* [RFC PATCH 0/2] protect git from a rogue editor
@ 2021-10-02 15:36 Carlo Marcelo Arenas Belón
2021-10-02 15:36 ` [RFC PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2021-10-02 15:36 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply [flat|nested] 14+ messages in thread* [RFC PATCH 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-02 15:36 [RFC PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón @ 2021-10-02 15:36 ` Carlo Marcelo Arenas Belón 2021-10-02 15:36 ` [RFC PATCH 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2 siblings, 0 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-02 15:36 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 2/2] editor: save and reset terminal after calling EDITOR 2021-10-02 15:36 [RFC PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2021-10-02 15:36 ` [RFC PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón @ 2021-10-02 15:36 ` Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2 siblings, 0 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-02 15:36 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 0/2] protect git from a rogue editor 2021-10-02 15:36 [RFC PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2021-10-02 15:36 ` [RFC PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón 2021-10-02 15:36 ` [RFC PATCH 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón @ 2021-10-04 7:25 ` Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón ` (3 more replies) 2 siblings, 4 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-04 7:25 UTC (permalink / raw) To: git; +Cc: johannes.schindelin, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón @ 2021-10-04 7:25 ` Carlo Marcelo Arenas Belón 2021-10-04 16:36 ` Junio C Hamano 2021-10-04 7:26 ` [PATCH 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-04 7:25 UTC (permalink / raw) To: git; +Cc: johannes.schindelin, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-04 7:25 ` [PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón @ 2021-10-04 16:36 ` Junio C Hamano 2021-10-04 17:27 ` Carlo Arenas 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2021-10-04 16:36 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, johannes.schindelin 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-04 16:36 ` Junio C Hamano @ 2021-10-04 17:27 ` Carlo Arenas 2021-10-04 18:10 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Carlo Arenas @ 2021-10-04 17:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-04 17:27 ` Carlo Arenas @ 2021-10-04 18:10 ` Junio C Hamano 2021-10-04 18:33 ` Carlo Arenas 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2021-10-04 18:10 UTC (permalink / raw) To: Carlo Arenas; +Cc: git, johannes.schindelin 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-04 18:10 ` Junio C Hamano @ 2021-10-04 18:33 ` Carlo Arenas 0 siblings, 0 replies; 14+ messages in thread From: Carlo Arenas @ 2021-10-04 18:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] editor: save and reset terminal after calling EDITOR 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón @ 2021-10-04 7:26 ` Carlo Marcelo Arenas Belón 2021-10-04 16:38 ` [PATCH 0/2] protect git from a rogue editor Junio C Hamano 2021-10-05 7:46 ` [PATCH v2 " Carlo Marcelo Arenas Belón 3 siblings, 0 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-04 7:26 UTC (permalink / raw) To: git; +Cc: johannes.schindelin, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] protect git from a rogue editor 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón 2021-10-04 7:26 ` [PATCH 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón @ 2021-10-04 16:38 ` Junio C Hamano 2021-10-05 7:46 ` [PATCH v2 " Carlo Marcelo Arenas Belón 3 siblings, 0 replies; 14+ messages in thread From: Junio C Hamano @ 2021-10-04 16:38 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, johannes.schindelin 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 ;-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 0/2] protect git from a rogue editor 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón ` (2 preceding siblings ...) 2021-10-04 16:38 ` [PATCH 0/2] protect git from a rogue editor Junio C Hamano @ 2021-10-05 7:46 ` Carlo Marcelo Arenas Belón 2021-10-05 7:46 ` [PATCH v2 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón 2021-10-05 7:46 ` [PATCH v2 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón 3 siblings, 2 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-05 7:46 UTC (permalink / raw) To: git; +Cc: johannes.schindelin, gitster, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] terminal: teach git how to save/restore its terminal settings 2021-10-05 7:46 ` [PATCH v2 " Carlo Marcelo Arenas Belón @ 2021-10-05 7:46 ` Carlo Marcelo Arenas Belón 2021-10-05 7:46 ` [PATCH v2 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón 1 sibling, 0 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-05 7:46 UTC (permalink / raw) To: git; +Cc: johannes.schindelin, gitster, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] editor: save and reset terminal after calling EDITOR 2021-10-05 7:46 ` [PATCH v2 " Carlo Marcelo Arenas Belón 2021-10-05 7:46 ` [PATCH v2 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón @ 2021-10-05 7:46 ` Carlo Marcelo Arenas Belón 1 sibling, 0 replies; 14+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-10-05 7:46 UTC (permalink / raw) To: git; +Cc: johannes.schindelin, gitster, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-10-05 7:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-02 15:36 [RFC PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2021-10-02 15:36 ` [RFC PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón 2021-10-02 15:36 ` [RFC PATCH 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 0/2] protect git from a rogue editor Carlo Marcelo Arenas Belón 2021-10-04 7:25 ` [PATCH 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón 2021-10-04 16:36 ` Junio C Hamano 2021-10-04 17:27 ` Carlo Arenas 2021-10-04 18:10 ` Junio C Hamano 2021-10-04 18:33 ` Carlo Arenas 2021-10-04 7:26 ` [PATCH 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón 2021-10-04 16:38 ` [PATCH 0/2] protect git from a rogue editor Junio C Hamano 2021-10-05 7:46 ` [PATCH v2 " Carlo Marcelo Arenas Belón 2021-10-05 7:46 ` [PATCH v2 1/2] terminal: teach git how to save/restore its terminal settings Carlo Marcelo Arenas Belón 2021-10-05 7:46 ` [PATCH v2 2/2] editor: save and reset terminal after calling EDITOR Carlo Marcelo Arenas Belón
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).