Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings

Carlo Arenas [off-list ref] writes:

On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano [off-list ref] wrote:

quoted

Carlo Marcelo Arenas Belón  [off-list ref] writes:

quoted

diff --git a/compat/terminal.c b/compat/terminal.c
index 43b73ddc75..1fadbfd6b6 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c

@@ -8,7 +8,7 @@

 #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)

-static void restore_term(void);
+void restore_term(void);

Curious why you need this because (1) we do not have the same for
save_term() here, and (2) we include compat/terminal.h where these
two are declared next to each other.

It is in preparation for the next patch where we will call these newly
public functions from editor.c
I'll be reusing restore_term(), while save_term() is new, hence why
only one had this change.

I think I understand all that correctly.

I was curious why the patch left a forward declaration, instead of
just removing it, which it can do because now we have the necessary
declaration in the header file it includes.

With only [1/2]:

 - we already have save_term() and restore_term() externally
   declared, so even outside users can use them (which is a good
   thing to do), as long as they include <compat/terminal.h>.

 - we include <compat/terminal.h> in compat/terminal.c; I do not see
   why the patch needs to turn a static forward declaration into an
   extern one in the hunk in question.

Thanks.

On Mon, Oct 4, 2021 at 11:10 AM Junio C Hamano [off-list ref] wrote:

Carlo Arenas [off-list ref] writes:

quoted

On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano [off-list ref] wrote:

quoted

Carlo Marcelo Arenas Belón  [off-list ref] writes:

quoted

diff --git a/compat/terminal.c b/compat/terminal.c
index 43b73ddc75..1fadbfd6b6 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c

@@ -8,7 +8,7 @@

 #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)

-static void restore_term(void);
+void restore_term(void);

Curious why you need this because (1) we do not have the same for
save_term() here, and (2) we include compat/terminal.h where these
two are declared next to each other.

It is in preparation for the next patch where we will call these newly
public functions from editor.c
I'll be reusing restore_term(), while save_term() is new, hence why
only one had this change.

I think I understand all that correctly.

I was curious why the patch left a forward declaration, instead of
just removing it, which it can do because now we have the necessary
declaration in the header file it includes.

of course, just sloppy coding on my part; will remove in a reroll

thanks,

Carlo