{"thread":{"id":"56627","subject":"[RFC PATCH 0/2] protect git from a rogue editor","startedAt":"2021-10-02T15:37:17Z","lastAt":"2021-10-05T07:47:05Z","messageCount":14,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"437766","messageId":"20211002153654.52443-1-carenas@gmail.com","threadId":"56627","inReplyTo":null,"subject":"[RFC PATCH 0/2] protect git from a rogue editor","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-02T15:36:52Z","receivedAt":"2021-10-02T15:37:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"The following series, expands git's terminal support to allow for\nsaving/restoring its settings around an EDITOR call.\n\nThe rationale is better documented in multiple tickets for Windows\nusers that had found themselves with a difficult to read output after\ndoing a commit (either directly or while running a rebase), because\nthey were using Windows Terminal and vi (the default).\n\nIt is also useful in POSIX systems which before this series will have\nto reset their terminals, if (for example) vi got killed while editing\na commit message.\n\nPosted as an RFC, because I suspect it might need additional changes\nto support Windows < 10 and because I wanted to add a patch that at\nleast renames the \"hconin\" variable and maybe fix some spurious errors,\nor even add some logic to prevent the reset if the terminal wasn't\nmessed with.\n\nCarlo Marcelo Arenas Belón (2):\n  terminal: teach git how to save/restore its terminal settings\n  editor: save and reset terminal after calling EDITOR\n\n compat/terminal.c | 72 ++++++++++++++++++++++++++++++++++++++---------\n compat/terminal.h |  3 ++\n editor.c          |  4 +++\n 3 files changed, 66 insertions(+), 13 deletions(-)\n\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437767","messageId":"20211002153654.52443-2-carenas@gmail.com","threadId":"56627","inReplyTo":"20211002153654.52443-1-carenas@gmail.com","subject":"[RFC PATCH 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-02T15:36:53Z","receivedAt":"2021-10-02T15:37:19Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Add two functions to push/pop the terminal settings using either\nthe POSIX or Windows console functions, and refactor the current\ncode to use them.\n\nThis will allow for the state of the terminal to be saved and\nrestored around a child that might need to change them (ex vi)\nand might not restore them properly upon exit.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n compat/terminal.c | 72 ++++++++++++++++++++++++++++++++++++++---------\n compat/terminal.h |  3 ++\n 2 files changed, 62 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 43b73ddc75..d3f858fe94 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -25,25 +25,38 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n-static void restore_term(void)\n+void pop_term(void)\n {\n \tif (term_fd < 0)\n \t\treturn;\n \n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n+}\n+\n+static void restore_term(void)\n+{\n+\tpop_term();\n+\n \tclose(term_fd);\n \tterm_fd = -1;\n }\n \n+int push_term(int full_duplex)\n+{\n+\tif (term_fd < 0)\n+\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\n+\treturn (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);\n+}\n+\n static int disable_bits(tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tterm_fd = open(\"/dev/tty\", O_RDWR);\n-\tif (tcgetattr(term_fd, &t) < 0)\n+\tif (push_term(0) < 0)\n \t\tgoto error;\n \n-\told_term = t;\n+\tt = old_term;\n \tsigchain_push_common(restore_term_on_signal);\n \n \tt.c_lflag &= ~bits;\n@@ -75,7 +88,22 @@ static int enable_non_canonical(void)\n static int use_stty = 1;\n static struct string_list stty_restore = STRING_LIST_INIT_DUP;\n static HANDLE hconin = INVALID_HANDLE_VALUE;\n-static DWORD cmode;\n+static HANDLE hconout = INVALID_HANDLE_VALUE;\n+static DWORD cmode_in, cmode_out;\n+\n+void pop_term(void)\n+{\n+\tif (hconin == INVALID_HANDLE_VALUE)\n+\t\treturn;\n+\n+\tSetConsoleMode(hconin, cmode_in);\n+\tCloseHandle(hconin);\n+\tif (cmodeout) {\n+\t\tassert(hconout != INVALID_HANDLE_VALUE);\n+\t\tSetConsoleMode(hconout, cmode_out);\n+\t\tCloseHandle(hconout);\n+\t}\n+}\n \n static void restore_term(void)\n {\n@@ -94,12 +122,34 @@ static void restore_term(void)\n \t\treturn;\n \t}\n \n+\tpop_term();\n+\thconin = hconout = INVALID_HANDLE_VALUE;\n+}\n+\n+int push_term(int full_duplex)\n+{\n+\thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n+\t    FILE_SHARE_READ, NULL, OPEN_EXISTING,\n+\t    FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (hconin == INVALID_HANDLE_VALUE)\n-\t\treturn;\n+\t\treturn -1;\n+\n+\tif (full_duplex) {\n+\t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n+\t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n+\t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n+\t\tif (hconout == INVALID_HANDLE_VALUE)\n+\t\t\tgoto error;\n \n-\tSetConsoleMode(hconin, cmode);\n+\t\tGetConsoleMode(hconout, &cmode_out);\n+\t}\n+\n+\tGetConsoleMode(hconin, &cmode_in);\n+\treturn 0;\n+error:\n \tCloseHandle(hconin);\n \thconin = INVALID_HANDLE_VALUE;\n+\treturn -1;\n }\n \n static int disable_bits(DWORD bits)\n@@ -135,15 +185,11 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\thconin = CreateFile(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n-\t    FILE_SHARE_READ, NULL, OPEN_EXISTING,\n-\t    FILE_ATTRIBUTE_NORMAL, NULL);\n-\tif (hconin == INVALID_HANDLE_VALUE)\n+\tif (push_term(0) < 0)\n \t\treturn -1;\n \n-\tGetConsoleMode(hconin, &cmode);\n \tsigchain_push_common(restore_term_on_signal);\n-\tif (!SetConsoleMode(hconin, cmode & ~bits)) {\n+\tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n \t\tCloseHandle(hconin);\n \t\thconin = INVALID_HANDLE_VALUE;\n \t\treturn -1;\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex a9d52b8464..5cc19116b7 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,6 +1,9 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+int push_term(int full_duplex);\n+void pop_term(void);\n+\n char *git_terminal_prompt(const char *prompt, int echo);\n \n /* Read a single keystroke, without echoing it to the terminal */\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437768","messageId":"20211002153654.52443-3-carenas@gmail.com","threadId":"56627","inReplyTo":"20211002153654.52443-1-carenas@gmail.com","subject":"[RFC PATCH 2/2] editor: save and reset terminal after calling EDITOR","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-02T15:36:54Z","receivedAt":"2021-10-02T15:37:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When EDITOR is invoked to modify a commit message, it will likely\nchange the terminal settings, and if it misbehaves will leave the\nterminal output as shown in microsoft/terminal#9359.\n\nInstead use the recently added {push,pop}_term() functions to save\nthe terminal configuration and recover safely.\n\n[1] https://github.com/microsoft/terminal/issues/9359\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n editor.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/editor.c b/editor.c\nindex 6303ae0ab0..9095841642 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"compat/terminal.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -83,7 +84,9 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n+\t\tpush_term(1);\n \t\tif (start_command(&p) < 0) {\n+\t\t\tpop_term();\n \t\t\tstrbuf_release(&realpath);\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \t\t}\n@@ -91,6 +94,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n+\t\tpop_term();\n \t\tstrbuf_release(&realpath);\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437852","messageId":"20211004072600.74241-1-carenas@gmail.com","threadId":"56627","inReplyTo":"20211002153654.52443-1-carenas@gmail.com","subject":"[PATCH 0/2] protect git from a rogue editor","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-04T07:25:58Z","receivedAt":"2021-10-04T07:26:19Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"The following series, expands git's terminal support to allow for\nsaving/restoring its settings around an EDITOR call.\n\nThe reason why that might be useful has been documented[1] by Windows\nusers that had found themselves not able to read clearly the messages\nprinted by git after a commit (or a rebase) when the default EDITOR\nfailed to reset the terminal settings completely.\n\nIt can be useful also in POSIX systems and indeed could be tested there\nby forcefully killing vi while doing a commit, and that before this series\nwill require the user to reset their terminal.\n\nThe code has been improved since the RFC and simplified, and has been\ntested in Windows 7 x86, and Windows 10 and 11 (x86_64).\n\n[1] https://github.com/microsoft/terminal/issues/10152#issuecomment-932808573\n\nCarlo Marcelo Arenas Belón (2):\n  terminal: teach git how to save/restore its terminal settings\n  editor: save and reset terminal after calling EDITOR\n\n compat/terminal.c | 75 ++++++++++++++++++++++++++++++++++++++---------\n compat/terminal.h |  3 ++\n editor.c          |  8 +++++\n 3 files changed, 72 insertions(+), 14 deletions(-)\n\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437853","messageId":"20211004072600.74241-2-carenas@gmail.com","threadId":"56627","inReplyTo":"20211004072600.74241-1-carenas@gmail.com","subject":"[PATCH 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-04T07:25:59Z","receivedAt":"2021-10-04T07:26:19Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Currently, git will share its console with all its children (unless\nthey create their own), and is therefore possible that any of them\nthat might change settings of them could affect its operations once\ncompleted.\n\nRefactor the platform specific functionality to save the terminal\nsettings and expand it to also do so for the output handler.\n\nThis will allow for the state of the terminal to be saved and\nrestored around a child that might misbehave (ex vi) which will\nbe implemented next.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n compat/terminal.c | 75 ++++++++++++++++++++++++++++++++++++++---------\n compat/terminal.h |  3 ++\n 2 files changed, 64 insertions(+), 14 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 43b73ddc75..1fadbfd6b6 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -8,7 +8,7 @@\n \n #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n \n-static void restore_term(void);\n+void restore_term(void);\n \n static void restore_term_on_signal(int sig)\n {\n@@ -25,7 +25,7 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n-static void restore_term(void)\n+void restore_term(void)\n {\n \tif (term_fd < 0)\n \t\treturn;\n@@ -35,15 +35,22 @@ static void restore_term(void)\n \tterm_fd = -1;\n }\n \n+int save_term(int full_duplex)\n+{\n+\tif (term_fd < 0)\n+\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\n+\treturn (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);\n+}\n+\n static int disable_bits(tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tterm_fd = open(\"/dev/tty\", O_RDWR);\n-\tif (tcgetattr(term_fd, &t) < 0)\n+\tif (save_term(0) < 0)\n \t\tgoto error;\n \n-\told_term = t;\n+\tt = old_term;\n \tsigchain_push_common(restore_term_on_signal);\n \n \tt.c_lflag &= ~bits;\n@@ -75,9 +82,10 @@ static int enable_non_canonical(void)\n static int use_stty = 1;\n static struct string_list stty_restore = STRING_LIST_INIT_DUP;\n static HANDLE hconin = INVALID_HANDLE_VALUE;\n-static DWORD cmode;\n+static HANDLE hconout = INVALID_HANDLE_VALUE;\n+static DWORD cmode_in, cmode_out;\n \n-static void restore_term(void)\n+void restore_term(void)\n {\n \tif (use_stty) {\n \t\tint i;\n@@ -97,9 +105,42 @@ static void restore_term(void)\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn;\n \n-\tSetConsoleMode(hconin, cmode);\n+\tSetConsoleMode(hconin, cmode_in);\n+\tCloseHandle(hconin);\n+\tif (cmode_out) {\n+\t\tassert(hconout != INVALID_HANDLE_VALUE);\n+\t\tSetConsoleMode(hconout, cmode_out);\n+\t\tCloseHandle(hconout);\n+\t}\n+\n+\thconin = hconout = INVALID_HANDLE_VALUE;\n+}\n+\n+int save_term(int full_duplex)\n+{\n+\thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n+\t    FILE_SHARE_READ, NULL, OPEN_EXISTING,\n+\t    FILE_ATTRIBUTE_NORMAL, NULL);\n+\tif (hconin == INVALID_HANDLE_VALUE)\n+\t\treturn -1;\n+\n+\tif (full_duplex) {\n+\t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n+\t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n+\t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n+\t\tif (hconout == INVALID_HANDLE_VALUE)\n+\t\t\tgoto error;\n+\n+\t\tGetConsoleMode(hconout, &cmode_out);\n+\t}\n+\n+\tGetConsoleMode(hconin, &cmode_in);\n+\tuse_stty = 0;\n+\treturn 0;\n+error:\n \tCloseHandle(hconin);\n \thconin = INVALID_HANDLE_VALUE;\n+\treturn -1;\n }\n \n static int disable_bits(DWORD bits)\n@@ -135,15 +176,11 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\thconin = CreateFile(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n-\t    FILE_SHARE_READ, NULL, OPEN_EXISTING,\n-\t    FILE_ATTRIBUTE_NORMAL, NULL);\n-\tif (hconin == INVALID_HANDLE_VALUE)\n+\tif (save_term(0) < 0)\n \t\treturn -1;\n \n-\tGetConsoleMode(hconin, &cmode);\n \tsigchain_push_common(restore_term_on_signal);\n-\tif (!SetConsoleMode(hconin, cmode & ~bits)) {\n+\tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n \t\tCloseHandle(hconin);\n \t\thconin = INVALID_HANDLE_VALUE;\n \t\treturn -1;\n@@ -361,6 +398,16 @@ int read_key_without_echo(struct strbuf *buf)\n \n #else\n \n+int save_term(int full_duplex)\n+{\n+\t/* full_duplex == 1, but no support available */\n+\treturn -full_duplex;\n+}\n+\n+void restore_term(void)\n+{\n+}\n+\n char *git_terminal_prompt(const char *prompt, int echo)\n {\n \treturn getpass(prompt);\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex a9d52b8464..e1770c575b 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,6 +1,9 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+int save_term(int full_duplex);\n+void restore_term(void);\n+\n char *git_terminal_prompt(const char *prompt, int echo);\n \n /* Read a single keystroke, without echoing it to the terminal */\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437854","messageId":"20211004072600.74241-3-carenas@gmail.com","threadId":"56627","inReplyTo":"20211004072600.74241-1-carenas@gmail.com","subject":"[PATCH 2/2] editor: save and reset terminal after calling EDITOR","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-04T07:26:00Z","receivedAt":"2021-10-04T07:26:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When EDITOR is invoked to modify a commit message, it will likely\nchange the terminal settings, and if it misbehaves will leave the\nterminal output damaged as shown in a recent report from Windows\nTerminal[1]\n\nInstead use the functions provided by compat/terminal to save the\nsettings and recover safely.\n\n[1] https://github.com/microsoft/terminal/issues/9359\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n editor.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/editor.c b/editor.c\nindex 6303ae0ab0..be7441e7e0 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"compat/terminal.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -50,6 +51,8 @@ const char *git_sequence_editor(void)\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t   struct strbuf *buffer, const char *const *env)\n {\n+\tint term_fail;\n+\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n \n@@ -83,7 +86,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n+\t\tterm_fail = save_term(1);\n \t\tif (start_command(&p) < 0) {\n+\t\t\tif (!term_fail)\n+\t\t\t\trestore_term();\n \t\t\tstrbuf_release(&realpath);\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \t\t}\n@@ -91,6 +97,8 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n+\t\tif (!term_fail)\n+\t\t\trestore_term();\n \t\tstrbuf_release(&realpath);\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437910","messageId":"xmqq1r50ycgf.fsf@gitster.g","threadId":"56627","inReplyTo":"20211004072600.74241-2-carenas@gmail.com","subject":"Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-04T16:36:00Z","receivedAt":"2021-10-04T16:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 43b73ddc75..1fadbfd6b6 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -8,7 +8,7 @@\n>  \n>  #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n>  \n> -static void restore_term(void);\n> +void restore_term(void);\n\nCurious why you need this because (1) we do not have the same for\nsave_term() here, and (2) we include compat/terminal.h where these\ntwo are declared next to each other.\n"},{"id":"437911","messageId":"xmqqwnmswxs7.fsf@gitster.g","threadId":"56627","inReplyTo":"20211004072600.74241-1-carenas@gmail.com","subject":"Re: [PATCH 0/2] protect git from a rogue editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-04T16:38:16Z","receivedAt":"2021-10-04T16:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> The following series, expands git's terminal support to allow for\n> saving/restoring its settings around an EDITOR call.\n>\n> The reason why that might be useful has been documented[1] by Windows\n> users that had found themselves not able to read clearly the messages\n> printed by git after a commit (or a rebase) when the default EDITOR\n> failed to reset the terminal settings completely.\n\nSounds sensible.  I do not think it is limited to Windows---I think\nI've caused vi crash to get me into an non-echoing terminal myself\nnon some variant of UNIX before Linux era ;-)\n\n"},{"id":"437924","messageId":"CAPUEsph_Aqaveur4qZ+Z+85gw1z2CvPOkzbkqW9zt56RoHhUJw@mail.gmail.com","threadId":"56627","inReplyTo":"xmqq1r50ycgf.fsf@gitster.g","subject":"Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-04T17:27:06Z","receivedAt":"2021-10-04T17:27:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n> > diff --git a/compat/terminal.c b/compat/terminal.c\n> > index 43b73ddc75..1fadbfd6b6 100644\n> > --- a/compat/terminal.c\n> > +++ b/compat/terminal.c\n> > @@ -8,7 +8,7 @@\n> >\n> >  #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n> >\n> > -static void restore_term(void);\n> > +void restore_term(void);\n>\n> Curious why you need this because (1) we do not have the same for\n> save_term() here, and (2) we include compat/terminal.h where these\n> two are declared next to each other.\n\nIt is in preparation for the next patch where we will call these newly\npublic functions from editor.c\nI'll be reusing restore_term(), while save_term() is new, hence why\nonly one had this change.\n\nCarlo\n"},{"id":"437928","messageId":"xmqqlf38wthy.fsf@gitster.g","threadId":"56627","inReplyTo":"CAPUEsph_Aqaveur4qZ+Z+85gw1z2CvPOkzbkqW9zt56RoHhUJw@mail.gmail.com","subject":"Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-04T18:10:49Z","receivedAt":"2021-10-04T18:10:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n>> > diff --git a/compat/terminal.c b/compat/terminal.c\n>> > index 43b73ddc75..1fadbfd6b6 100644\n>> > --- a/compat/terminal.c\n>> > +++ b/compat/terminal.c\n>> > @@ -8,7 +8,7 @@\n>> >\n>> >  #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n>> >\n>> > -static void restore_term(void);\n>> > +void restore_term(void);\n>>\n>> Curious why you need this because (1) we do not have the same for\n>> save_term() here, and (2) we include compat/terminal.h where these\n>> two are declared next to each other.\n>\n> It is in preparation for the next patch where we will call these newly\n> public functions from editor.c\n> I'll be reusing restore_term(), while save_term() is new, hence why\n> only one had this change.\n\nI think I understand all that correctly.\n\nI was curious why the patch left a forward declaration, instead of\njust removing it, which it can do because now we have the necessary\ndeclaration in the header file it includes.\n\nWith only [1/2]:\n\n - we already have save_term() and restore_term() externally\n   declared, so even outside users can use them (which is a good\n   thing to do), as long as they include <compat/terminal.h>.\n\n - we include <compat/terminal.h> in compat/terminal.c; I do not see\n   why the patch needs to turn a static forward declaration into an\n   extern one in the hunk in question.\n\nThanks.\n"},{"id":"437930","messageId":"CAPUEspiQrGDyYrBUmeMh0C1uPDjUE5d--5zT4vZZUdNr+xtAxA@mail.gmail.com","threadId":"56627","inReplyTo":"xmqqlf38wthy.fsf@gitster.g","subject":"Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-04T18:33:07Z","receivedAt":"2021-10-04T18:33:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Oct 4, 2021 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Carlo Arenas <carenas@gmail.com> writes:\n> > On Mon, Oct 4, 2021 at 9:36 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n> >> > diff --git a/compat/terminal.c b/compat/terminal.c\n> >> > index 43b73ddc75..1fadbfd6b6 100644\n> >> > --- a/compat/terminal.c\n> >> > +++ b/compat/terminal.c\n> >> > @@ -8,7 +8,7 @@\n> >> >\n> >> >  #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n> >> >\n> >> > -static void restore_term(void);\n> >> > +void restore_term(void);\n> >>\n> >> Curious why you need this because (1) we do not have the same for\n> >> save_term() here, and (2) we include compat/terminal.h where these\n> >> two are declared next to each other.\n> >\n> > It is in preparation for the next patch where we will call these newly\n> > public functions from editor.c\n> > I'll be reusing restore_term(), while save_term() is new, hence why\n> > only one had this change.\n>\n> I think I understand all that correctly.\n>\n> I was curious why the patch left a forward declaration, instead of\n> just removing it, which it can do because now we have the necessary\n> declaration in the header file it includes.\n\nof course, just sloppy coding on my part; will remove in a reroll\n\nthanks,\n\nCarlo\n"},{"id":"437959","messageId":"20211005074648.86032-1-carenas@gmail.com","threadId":"56627","inReplyTo":"20211004072600.74241-1-carenas@gmail.com","subject":"[PATCH v2 0/2] protect git from a rogue editor","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-05T07:46:46Z","receivedAt":"2021-10-05T07:47:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This is a reroll of cm/save-restore-terminal that removes the unnecessary\nforward declaration in compat/terminal that was patiently reported[1] by\nJunio.\n\nAlso changed the wording used in the first paragraph of the commit message\nfor the first patch, which hopefully has better grammar.\n\nThe series implements a way for git to save and restore the terminal\nsettings around EDITOR calls to avoid having to deal with botched output\nif the editor misbehaves.\n\nCarlo Marcelo Arenas Belón (2):\n  terminal: teach git how to save/restore its terminal settings\n  editor: save and reset terminal after calling EDITOR\n\n compat/terminal.c | 75 +++++++++++++++++++++++++++++++++++++----------\n compat/terminal.h |  3 ++\n editor.c          |  8 +++++\n 3 files changed, 71 insertions(+), 15 deletions(-)\n\n[1] https://lore.kernel.org/git/CAPUEspiQrGDyYrBUmeMh0C1uPDjUE5d--5zT4vZZUdNr+xtAxA@mail.gmail.com/T/#u\n\nRange-diff against v1:\n1:  63cfeda4b7 ! 1:  cf8306816d terminal: teach git how to save/restore its terminal settings\n    @@ compat/terminal.c\n      #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n      \n     -static void restore_term(void);\n    -+void restore_term(void);\n    - \n    +-\n      static void restore_term_on_signal(int sig)\n      {\n    + \trestore_term();\n     @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n      static int term_fd = -1;\n      static struct termios old_term;\n2:  0f409b6d00 = 2:  d83cd3f712 editor: save and reset terminal after calling EDITOR\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437960","messageId":"20211005074648.86032-2-carenas@gmail.com","threadId":"56627","inReplyTo":"20211005074648.86032-1-carenas@gmail.com","subject":"[PATCH v2 1/2] terminal: teach git how to save/restore its terminal settings","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-05T07:46:47Z","receivedAt":"2021-10-05T07:47:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Currently, git will share its console with all its children (unless\nthey create their own), and is therefore possible that any of them\nthat might change the settings for it could affect its operations once\ncompleted.\n\nRefactor the platform specific functionality to save the terminal\nsettings and expand it to also do so for the output handler.\n\nThis will allow for the state of the terminal to be saved and\nrestored around a child that might misbehave (ex vi) which will\nbe implemented next.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nv2:\n- remove unnecessary forward declaration for restore_term per Junio\n- reword first paragraph with hopefully better grammar\n\n compat/terminal.c | 75 +++++++++++++++++++++++++++++++++++++----------\n compat/terminal.h |  3 ++\n 2 files changed, 63 insertions(+), 15 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 43b73ddc75..5b903e7c7e 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -8,8 +8,6 @@\n \n #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE)\n \n-static void restore_term(void);\n-\n static void restore_term_on_signal(int sig)\n {\n \trestore_term();\n@@ -25,7 +23,7 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n-static void restore_term(void)\n+void restore_term(void)\n {\n \tif (term_fd < 0)\n \t\treturn;\n@@ -35,15 +33,22 @@ static void restore_term(void)\n \tterm_fd = -1;\n }\n \n+int save_term(int full_duplex)\n+{\n+\tif (term_fd < 0)\n+\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\n+\treturn (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);\n+}\n+\n static int disable_bits(tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tterm_fd = open(\"/dev/tty\", O_RDWR);\n-\tif (tcgetattr(term_fd, &t) < 0)\n+\tif (save_term(0) < 0)\n \t\tgoto error;\n \n-\told_term = t;\n+\tt = old_term;\n \tsigchain_push_common(restore_term_on_signal);\n \n \tt.c_lflag &= ~bits;\n@@ -75,9 +80,10 @@ static int enable_non_canonical(void)\n static int use_stty = 1;\n static struct string_list stty_restore = STRING_LIST_INIT_DUP;\n static HANDLE hconin = INVALID_HANDLE_VALUE;\n-static DWORD cmode;\n+static HANDLE hconout = INVALID_HANDLE_VALUE;\n+static DWORD cmode_in, cmode_out;\n \n-static void restore_term(void)\n+void restore_term(void)\n {\n \tif (use_stty) {\n \t\tint i;\n@@ -97,9 +103,42 @@ static void restore_term(void)\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn;\n \n-\tSetConsoleMode(hconin, cmode);\n+\tSetConsoleMode(hconin, cmode_in);\n+\tCloseHandle(hconin);\n+\tif (cmode_out) {\n+\t\tassert(hconout != INVALID_HANDLE_VALUE);\n+\t\tSetConsoleMode(hconout, cmode_out);\n+\t\tCloseHandle(hconout);\n+\t}\n+\n+\thconin = hconout = INVALID_HANDLE_VALUE;\n+}\n+\n+int save_term(int full_duplex)\n+{\n+\thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n+\t    FILE_SHARE_READ, NULL, OPEN_EXISTING,\n+\t    FILE_ATTRIBUTE_NORMAL, NULL);\n+\tif (hconin == INVALID_HANDLE_VALUE)\n+\t\treturn -1;\n+\n+\tif (full_duplex) {\n+\t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n+\t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n+\t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n+\t\tif (hconout == INVALID_HANDLE_VALUE)\n+\t\t\tgoto error;\n+\n+\t\tGetConsoleMode(hconout, &cmode_out);\n+\t}\n+\n+\tGetConsoleMode(hconin, &cmode_in);\n+\tuse_stty = 0;\n+\treturn 0;\n+error:\n \tCloseHandle(hconin);\n \thconin = INVALID_HANDLE_VALUE;\n+\treturn -1;\n }\n \n static int disable_bits(DWORD bits)\n@@ -135,15 +174,11 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\thconin = CreateFile(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n-\t    FILE_SHARE_READ, NULL, OPEN_EXISTING,\n-\t    FILE_ATTRIBUTE_NORMAL, NULL);\n-\tif (hconin == INVALID_HANDLE_VALUE)\n+\tif (save_term(0) < 0)\n \t\treturn -1;\n \n-\tGetConsoleMode(hconin, &cmode);\n \tsigchain_push_common(restore_term_on_signal);\n-\tif (!SetConsoleMode(hconin, cmode & ~bits)) {\n+\tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n \t\tCloseHandle(hconin);\n \t\thconin = INVALID_HANDLE_VALUE;\n \t\treturn -1;\n@@ -361,6 +396,16 @@ int read_key_without_echo(struct strbuf *buf)\n \n #else\n \n+int save_term(int full_duplex)\n+{\n+\t/* full_duplex == 1, but no support available */\n+\treturn -full_duplex;\n+}\n+\n+void restore_term(void)\n+{\n+}\n+\n char *git_terminal_prompt(const char *prompt, int echo)\n {\n \treturn getpass(prompt);\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex a9d52b8464..e1770c575b 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,6 +1,9 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+int save_term(int full_duplex);\n+void restore_term(void);\n+\n char *git_terminal_prompt(const char *prompt, int echo);\n \n /* Read a single keystroke, without echoing it to the terminal */\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437961","messageId":"20211005074648.86032-3-carenas@gmail.com","threadId":"56627","inReplyTo":"20211005074648.86032-1-carenas@gmail.com","subject":"[PATCH v2 2/2] editor: save and reset terminal after calling EDITOR","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-05T07:46:48Z","receivedAt":"2021-10-05T07:47:05Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When EDITOR is invoked to modify a commit message, it will likely\nchange the terminal settings, and if it misbehaves will leave the\nterminal output damaged as shown in a recent report from Windows\nTerminal[1]\n\nInstead use the functions provided by compat/terminal to save the\nsettings and recover safely.\n\n[1] https://github.com/microsoft/terminal/issues/9359\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n editor.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/editor.c b/editor.c\nindex 6303ae0ab0..be7441e7e0 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"compat/terminal.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -50,6 +51,8 @@ const char *git_sequence_editor(void)\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t   struct strbuf *buffer, const char *const *env)\n {\n+\tint term_fail;\n+\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n \n@@ -83,7 +86,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n+\t\tterm_fail = save_term(1);\n \t\tif (start_command(&p) < 0) {\n+\t\t\tif (!term_fail)\n+\t\t\t\trestore_term();\n \t\t\tstrbuf_release(&realpath);\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \t\t}\n@@ -91,6 +97,8 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n+\t\tif (!term_fail)\n+\t\t\trestore_term();\n \t\tstrbuf_release(&realpath);\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n-- \n2.33.0.955.gee03ddbf0e\n\n"}]}