[RFC PATCH 0/2] protect git from a rogue editor

[RFC PATCH 0/2] protect git from a rogue editor

[Carlo Marcelo Arenas Belón]

The following series, expands git's terminal support to allow for
saving/restoring its settings around an EDITOR call.

The rationale is better documented in multiple tickets for Windows
users that had found themselves with a difficult to read output after
doing a commit (either directly or while running a rebase), because
they were using Windows Terminal and vi (the default).

It is also useful in POSIX systems which before this series will have
to reset their terminals, if (for example) vi got killed while editing
a commit message.

Posted as an RFC, because I suspect it might need additional changes
to support Windows < 10 and because I wanted to add a patch that at
least renames the "hconin" variable and maybe fix some spurious errors,
or even add some logic to prevent the reset if the terminal wasn't
messed with.

Carlo Marcelo Arenas Belón (2):
  terminal: teach git how to save/restore its terminal settings
  editor: save and reset terminal after calling EDITOR

 compat/terminal.c | 72 ++++++++++++++++++++++++++++++++++++++---------
 compat/terminal.h |  3 ++
 editor.c          |  4 +++
 3 files changed, 66 insertions(+), 13 deletions(-)

-- 
2.33.0.955.gee03ddbf0e

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

[Carlo Marcelo Arenas Belón]

Add two functions to push/pop the terminal settings using either
the POSIX or Windows console functions, and refactor the current
code to use them.

This will allow for the state of the terminal to be saved and
restored around a child that might need to change them (ex vi)
and might not restore them properly upon exit.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 compat/terminal.c | 72 ++++++++++++++++++++++++++++++++++++++---------
 compat/terminal.h |  3 ++
 2 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 43b73ddc75..d3f858fe94 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -25,25 +25,38 @@ static void restore_term_on_signal(int sig)
 static int term_fd = -1;
 static struct termios old_term;
 
-static void restore_term(void)
+void pop_term(void)
 {
 	if (term_fd < 0)
 		return;
 
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
+}
+
+static void restore_term(void)
+{
+	pop_term();
+
 	close(term_fd);
 	term_fd = -1;
 }
 
+int push_term(int full_duplex)
+{
+	if (term_fd < 0)
+		term_fd = open("/dev/tty", O_RDWR);
+
+	return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);
+}
+
 static int disable_bits(tcflag_t bits)
 {
 	struct termios t;
 
-	term_fd = open("/dev/tty", O_RDWR);
-	if (tcgetattr(term_fd, &t) < 0)
+	if (push_term(0) < 0)
 		goto error;
 
-	old_term = t;
+	t = old_term;
 	sigchain_push_common(restore_term_on_signal);
 
 	t.c_lflag &= ~bits;
@@ -75,7 +88,22 @@ static int enable_non_canonical(void)
 static int use_stty = 1;
 static struct string_list stty_restore = STRING_LIST_INIT_DUP;
 static HANDLE hconin = INVALID_HANDLE_VALUE;
-static DWORD cmode;
+static HANDLE hconout = INVALID_HANDLE_VALUE;
+static DWORD cmode_in, cmode_out;
+
+void pop_term(void)
+{
+	if (hconin == INVALID_HANDLE_VALUE)
+		return;
+
+	SetConsoleMode(hconin, cmode_in);
+	CloseHandle(hconin);
+	if (cmodeout) {
+		assert(hconout != INVALID_HANDLE_VALUE);
+		SetConsoleMode(hconout, cmode_out);
+		CloseHandle(hconout);
+	}
+}
 
 static void restore_term(void)
 {
@@ -94,12 +122,34 @@ static void restore_term(void)
 		return;
 	}
 
+	pop_term();
+	hconin = hconout = INVALID_HANDLE_VALUE;
+}
+
+int push_term(int full_duplex)
+{
+	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
+	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
+	    FILE_ATTRIBUTE_NORMAL, NULL);
 	if (hconin == INVALID_HANDLE_VALUE)
-		return;
+		return -1;
+
+	if (full_duplex) {
+		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
+			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
+			FILE_ATTRIBUTE_NORMAL, NULL);
+		if (hconout == INVALID_HANDLE_VALUE)
+			goto error;
 
-	SetConsoleMode(hconin, cmode);
+		GetConsoleMode(hconout, &cmode_out);
+	}
+
+	GetConsoleMode(hconin, &cmode_in);
+	return 0;
+error:
 	CloseHandle(hconin);
 	hconin = INVALID_HANDLE_VALUE;
+	return -1;
 }
 
 static int disable_bits(DWORD bits)
@@ -135,15 +185,11 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
-	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
-	    FILE_ATTRIBUTE_NORMAL, NULL);
-	if (hconin == INVALID_HANDLE_VALUE)
+	if (push_term(0) < 0)
 		return -1;
 
-	GetConsoleMode(hconin, &cmode);
 	sigchain_push_common(restore_term_on_signal);
-	if (!SetConsoleMode(hconin, cmode & ~bits)) {
+	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
 		CloseHandle(hconin);
 		hconin = INVALID_HANDLE_VALUE;
 		return -1;
diff --git a/compat/terminal.h b/compat/terminal.h
index a9d52b8464..5cc19116b7 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,6 +1,9 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+int push_term(int full_duplex);
+void pop_term(void);
+
 char *git_terminal_prompt(const char *prompt, int echo);
 
 /* Read a single keystroke, without echoing it to the terminal */
-- 
2.33.0.955.gee03ddbf0e

[RFC PATCH 2/2] editor: save and reset terminal after calling EDITOR

[Carlo Marcelo Arenas Belón]

When EDITOR is invoked to modify a commit message, it will likely
change the terminal settings, and if it misbehaves will leave the
terminal output as shown in microsoft/terminal#9359.

Instead use the recently added {push,pop}_term() functions to save
the terminal configuration and recover safely.

[1] https://github.com/microsoft/terminal/issues/9359

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 editor.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/editor.c b/editor.c
index 6303ae0ab0..9095841642 100644
--- a/editor.c
+++ b/editor.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "compat/terminal.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -83,7 +84,9 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
+		push_term(1);
 		if (start_command(&p) < 0) {
+			pop_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
 		}
@@ -91,6 +94,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		pop_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
-- 
2.33.0.955.gee03ddbf0e

[PATCH 0/2] protect git from a rogue editor

[Carlo Marcelo Arenas Belón]

The following series, expands git's terminal support to allow for
saving/restoring its settings around an EDITOR call.

The reason why that might be useful has been documented[1] by Windows
users that had found themselves not able to read clearly the messages
printed by git after a commit (or a rebase) when the default EDITOR
failed to reset the terminal settings completely.

It can be useful also in POSIX systems and indeed could be tested there
by forcefully killing vi while doing a commit, and that before this series
will require the user to reset their terminal.

The code has been improved since the RFC and simplified, and has been
tested in Windows 7 x86, and Windows 10 and 11 (x86_64).

[1] https://github.com/microsoft/terminal/issues/10152#issuecomment-932808573

Carlo Marcelo Arenas Belón (2):
  terminal: teach git how to save/restore its terminal settings
  editor: save and reset terminal after calling EDITOR

 compat/terminal.c | 75 ++++++++++++++++++++++++++++++++++++++---------
 compat/terminal.h |  3 ++
 editor.c          |  8 +++++
 3 files changed, 72 insertions(+), 14 deletions(-)

-- 
2.33.0.955.gee03ddbf0e

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

[Carlo Marcelo Arenas Belón]

Currently, git will share its console with all its children (unless
they create their own), and is therefore possible that any of them
that might change settings of them could affect its operations once
completed.

Refactor the platform specific functionality to save the terminal
settings and expand it to also do so for the output handler.

This will allow for the state of the terminal to be saved and
restored around a child that might misbehave (ex vi) which will
be implemented next.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 compat/terminal.c | 75 ++++++++++++++++++++++++++++++++++++++---------
 compat/terminal.h |  3 ++
 2 files changed, 64 insertions(+), 14 deletions(-)

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);
 
 static void restore_term_on_signal(int sig)
 {
@@ -25,7 +25,7 @@ static void restore_term_on_signal(int sig)
 static int term_fd = -1;
 static struct termios old_term;
 
-static void restore_term(void)
+void restore_term(void)
 {
 	if (term_fd < 0)
 		return;
@@ -35,15 +35,22 @@ static void restore_term(void)
 	term_fd = -1;
 }
 
+int save_term(int full_duplex)
+{
+	if (term_fd < 0)
+		term_fd = open("/dev/tty", O_RDWR);
+
+	return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);
+}
+
 static int disable_bits(tcflag_t bits)
 {
 	struct termios t;
 
-	term_fd = open("/dev/tty", O_RDWR);
-	if (tcgetattr(term_fd, &t) < 0)
+	if (save_term(0) < 0)
 		goto error;
 
-	old_term = t;
+	t = old_term;
 	sigchain_push_common(restore_term_on_signal);
 
 	t.c_lflag &= ~bits;
@@ -75,9 +82,10 @@ static int enable_non_canonical(void)
 static int use_stty = 1;
 static struct string_list stty_restore = STRING_LIST_INIT_DUP;
 static HANDLE hconin = INVALID_HANDLE_VALUE;
-static DWORD cmode;
+static HANDLE hconout = INVALID_HANDLE_VALUE;
+static DWORD cmode_in, cmode_out;
 
-static void restore_term(void)
+void restore_term(void)
 {
 	if (use_stty) {
 		int i;
@@ -97,9 +105,42 @@ static void restore_term(void)
 	if (hconin == INVALID_HANDLE_VALUE)
 		return;
 
-	SetConsoleMode(hconin, cmode);
+	SetConsoleMode(hconin, cmode_in);
+	CloseHandle(hconin);
+	if (cmode_out) {
+		assert(hconout != INVALID_HANDLE_VALUE);
+		SetConsoleMode(hconout, cmode_out);
+		CloseHandle(hconout);
+	}
+
+	hconin = hconout = INVALID_HANDLE_VALUE;
+}
+
+int save_term(int full_duplex)
+{
+	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
+	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
+	    FILE_ATTRIBUTE_NORMAL, NULL);
+	if (hconin == INVALID_HANDLE_VALUE)
+		return -1;
+
+	if (full_duplex) {
+		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
+			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
+			FILE_ATTRIBUTE_NORMAL, NULL);
+		if (hconout == INVALID_HANDLE_VALUE)
+			goto error;
+
+		GetConsoleMode(hconout, &cmode_out);
+	}
+
+	GetConsoleMode(hconin, &cmode_in);
+	use_stty = 0;
+	return 0;
+error:
 	CloseHandle(hconin);
 	hconin = INVALID_HANDLE_VALUE;
+	return -1;
 }
 
 static int disable_bits(DWORD bits)
@@ -135,15 +176,11 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
-	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
-	    FILE_ATTRIBUTE_NORMAL, NULL);
-	if (hconin == INVALID_HANDLE_VALUE)
+	if (save_term(0) < 0)
 		return -1;
 
-	GetConsoleMode(hconin, &cmode);
 	sigchain_push_common(restore_term_on_signal);
-	if (!SetConsoleMode(hconin, cmode & ~bits)) {
+	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
 		CloseHandle(hconin);
 		hconin = INVALID_HANDLE_VALUE;
 		return -1;
@@ -361,6 +398,16 @@ int read_key_without_echo(struct strbuf *buf)
 
 #else
 
+int save_term(int full_duplex)
+{
+	/* full_duplex == 1, but no support available */
+	return -full_duplex;
+}
+
+void restore_term(void)
+{
+}
+
 char *git_terminal_prompt(const char *prompt, int echo)
 {
 	return getpass(prompt);
diff --git a/compat/terminal.h b/compat/terminal.h
index a9d52b8464..e1770c575b 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,6 +1,9 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+int save_term(int full_duplex);
+void restore_term(void);
+
 char *git_terminal_prompt(const char *prompt, int echo);
 
 /* Read a single keystroke, without echoing it to the terminal */
-- 
2.33.0.955.gee03ddbf0e

[PATCH 2/2] editor: save and reset terminal after calling EDITOR

[Carlo Marcelo Arenas Belón]

When EDITOR is invoked to modify a commit message, it will likely
change the terminal settings, and if it misbehaves will leave the
terminal output damaged as shown in a recent report from Windows
Terminal[1]

Instead use the functions provided by compat/terminal to save the
settings and recover safely.

[1] https://github.com/microsoft/terminal/issues/9359

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 editor.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/editor.c b/editor.c
index 6303ae0ab0..be7441e7e0 100644
--- a/editor.c
+++ b/editor.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "compat/terminal.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -50,6 +51,8 @@ const char *git_sequence_editor(void)
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
+	int term_fail;
+
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
 
@@ -83,7 +86,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
+		term_fail = save_term(1);
 		if (start_command(&p) < 0) {
+			if (!term_fail)
+				restore_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
 		}
@@ -91,6 +97,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		if (!term_fail)
+			restore_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
-- 
2.33.0.955.gee03ddbf0e

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

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 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.

Re: [PATCH 0/2] protect git from a rogue editor

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> The following series, expands git's terminal support to allow for
> saving/restoring its settings around an EDITOR call.
>
> The reason why that might be useful has been documented[1] by Windows
> users that had found themselves not able to read clearly the messages
> printed by git after a commit (or a rebase) when the default EDITOR
> failed to reset the terminal settings completely.

Sounds sensible.  I do not think it is limited to Windows---I think
I've caused vi crash to get me into an non-echoing terminal myself
non some variant of UNIX before Linux era ;-)

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

[Carlo Arenas]

On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> > 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.

Carlo

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

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>> > 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.

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

[Carlo Arenas]

On Mon, Oct 4, 2021 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:
> Carlo Arenas <carenas@gmail.com> writes:
> > On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote:
> >> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> >> > 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

[PATCH v2 0/2] protect git from a rogue editor

[Carlo Marcelo Arenas Belón]

This is a reroll of cm/save-restore-terminal that removes the unnecessary
forward declaration in compat/terminal that was patiently reported[1] by
Junio.

Also changed the wording used in the first paragraph of the commit message
for the first patch, which hopefully has better grammar.

The series implements a way for git to save and restore the terminal
settings around EDITOR calls to avoid having to deal with botched output
if the editor misbehaves.

Carlo Marcelo Arenas Belón (2):
  terminal: teach git how to save/restore its terminal settings
  editor: save and reset terminal after calling EDITOR

 compat/terminal.c | 75 +++++++++++++++++++++++++++++++++++++----------
 compat/terminal.h |  3 ++
 editor.c          |  8 +++++
 3 files changed, 71 insertions(+), 15 deletions(-)

[1] https://lore.kernel.org/git/CAPUEspiQrGDyYrBUmeMh0C1uPDjUE5d--5zT4vZZUdNr+xtAxA@mail.gmail.com/T/#u

Range-diff against v1:
1:  63cfeda4b7 ! 1:  cf8306816d terminal: teach git how to save/restore its terminal settings
    @@ compat/terminal.c
      #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)
      
     -static void restore_term(void);
    -+void restore_term(void);
    - 
    +-
      static void restore_term_on_signal(int sig)
      {
    + 	restore_term();
     @@ compat/terminal.c: static void restore_term_on_signal(int sig)
      static int term_fd = -1;
      static struct termios old_term;
2:  0f409b6d00 = 2:  d83cd3f712 editor: save and reset terminal after calling EDITOR
-- 
2.33.0.955.gee03ddbf0e

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

[Carlo Marcelo Arenas Belón]

Currently, git will share its console with all its children (unless
they create their own), and is therefore possible that any of them
that might change the settings for it could affect its operations once
completed.

Refactor the platform specific functionality to save the terminal
settings and expand it to also do so for the output handler.

This will allow for the state of the terminal to be saved and
restored around a child that might misbehave (ex vi) which will
be implemented next.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
- remove unnecessary forward declaration for restore_term per Junio
- reword first paragraph with hopefully better grammar

 compat/terminal.c | 75 +++++++++++++++++++++++++++++++++++++----------
 compat/terminal.h |  3 ++
 2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 43b73ddc75..5b903e7c7e 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -8,8 +8,6 @@
 
 #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)
 
-static void restore_term(void);
-
 static void restore_term_on_signal(int sig)
 {
 	restore_term();
@@ -25,7 +23,7 @@ static void restore_term_on_signal(int sig)
 static int term_fd = -1;
 static struct termios old_term;
 
-static void restore_term(void)
+void restore_term(void)
 {
 	if (term_fd < 0)
 		return;
@@ -35,15 +33,22 @@ static void restore_term(void)
 	term_fd = -1;
 }
 
+int save_term(int full_duplex)
+{
+	if (term_fd < 0)
+		term_fd = open("/dev/tty", O_RDWR);
+
+	return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);
+}
+
 static int disable_bits(tcflag_t bits)
 {
 	struct termios t;
 
-	term_fd = open("/dev/tty", O_RDWR);
-	if (tcgetattr(term_fd, &t) < 0)
+	if (save_term(0) < 0)
 		goto error;
 
-	old_term = t;
+	t = old_term;
 	sigchain_push_common(restore_term_on_signal);
 
 	t.c_lflag &= ~bits;
@@ -75,9 +80,10 @@ static int enable_non_canonical(void)
 static int use_stty = 1;
 static struct string_list stty_restore = STRING_LIST_INIT_DUP;
 static HANDLE hconin = INVALID_HANDLE_VALUE;
-static DWORD cmode;
+static HANDLE hconout = INVALID_HANDLE_VALUE;
+static DWORD cmode_in, cmode_out;
 
-static void restore_term(void)
+void restore_term(void)
 {
 	if (use_stty) {
 		int i;
@@ -97,9 +103,42 @@ static void restore_term(void)
 	if (hconin == INVALID_HANDLE_VALUE)
 		return;
 
-	SetConsoleMode(hconin, cmode);
+	SetConsoleMode(hconin, cmode_in);
+	CloseHandle(hconin);
+	if (cmode_out) {
+		assert(hconout != INVALID_HANDLE_VALUE);
+		SetConsoleMode(hconout, cmode_out);
+		CloseHandle(hconout);
+	}
+
+	hconin = hconout = INVALID_HANDLE_VALUE;
+}
+
+int save_term(int full_duplex)
+{
+	hconin = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE,
+	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
+	    FILE_ATTRIBUTE_NORMAL, NULL);
+	if (hconin == INVALID_HANDLE_VALUE)
+		return -1;
+
+	if (full_duplex) {
+		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
+			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
+			FILE_ATTRIBUTE_NORMAL, NULL);
+		if (hconout == INVALID_HANDLE_VALUE)
+			goto error;
+
+		GetConsoleMode(hconout, &cmode_out);
+	}
+
+	GetConsoleMode(hconin, &cmode_in);
+	use_stty = 0;
+	return 0;
+error:
 	CloseHandle(hconin);
 	hconin = INVALID_HANDLE_VALUE;
+	return -1;
 }
 
 static int disable_bits(DWORD bits)
@@ -135,15 +174,11 @@ static int disable_bits(DWORD bits)
 		use_stty = 0;
 	}
 
-	hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
-	    FILE_SHARE_READ, NULL, OPEN_EXISTING,
-	    FILE_ATTRIBUTE_NORMAL, NULL);
-	if (hconin == INVALID_HANDLE_VALUE)
+	if (save_term(0) < 0)
 		return -1;
 
-	GetConsoleMode(hconin, &cmode);
 	sigchain_push_common(restore_term_on_signal);
-	if (!SetConsoleMode(hconin, cmode & ~bits)) {
+	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
 		CloseHandle(hconin);
 		hconin = INVALID_HANDLE_VALUE;
 		return -1;
@@ -361,6 +396,16 @@ int read_key_without_echo(struct strbuf *buf)
 
 #else
 
+int save_term(int full_duplex)
+{
+	/* full_duplex == 1, but no support available */
+	return -full_duplex;
+}
+
+void restore_term(void)
+{
+}
+
 char *git_terminal_prompt(const char *prompt, int echo)
 {
 	return getpass(prompt);
diff --git a/compat/terminal.h b/compat/terminal.h
index a9d52b8464..e1770c575b 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,6 +1,9 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+int save_term(int full_duplex);
+void restore_term(void);
+
 char *git_terminal_prompt(const char *prompt, int echo);
 
 /* Read a single keystroke, without echoing it to the terminal */
-- 
2.33.0.955.gee03ddbf0e

[PATCH v2 2/2] editor: save and reset terminal after calling EDITOR

[Carlo Marcelo Arenas Belón]

When EDITOR is invoked to modify a commit message, it will likely
change the terminal settings, and if it misbehaves will leave the
terminal output damaged as shown in a recent report from Windows
Terminal[1]

Instead use the functions provided by compat/terminal to save the
settings and recover safely.

[1] https://github.com/microsoft/terminal/issues/9359

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 editor.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/editor.c b/editor.c
index 6303ae0ab0..be7441e7e0 100644
--- a/editor.c
+++ b/editor.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "compat/terminal.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -50,6 +51,8 @@ const char *git_sequence_editor(void)
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
+	int term_fail;
+
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
 
@@ -83,7 +86,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
+		term_fail = save_term(1);
 		if (start_command(&p) < 0) {
+			if (!term_fail)
+				restore_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
 		}
@@ -91,6 +97,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		if (!term_fail)
+			restore_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
-- 
2.33.0.955.gee03ddbf0e