* [PATCH 0/3] a couple of read_key_without_echo() fixes @ 2022-02-16 10:54 Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 1/3] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-16 10:54 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood Fix a couple of bugs I noticed when using the builtin "add -p" with interactive.singlekey=true. The first patch is a general fix for the terminal save/restore functionality which forgot to call sigchain_pop() when it restored the terminal. The last two fix reading single keys in non-canonical mode by making sure we wait for an initial key press and only read one byte at a time from the underlying file descriptor. Note that these are untested on windows beyond our CI compiling them Phillip Wood (3): terminal: pop signal handler when terminal is restored terminal: set VMIN and VTIME in non-canonical mode add -p: disable stdin buffering when interactive.singlekey is set add-interactive.c | 2 ++ compat/terminal.c | 27 ++++++++++++++++++++++----- compat/terminal.h | 8 ++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) base-commit: b80121027d1247a0754b3cc46897fee75c050b44 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1146 -- gitgitgadget ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] terminal: pop signal handler when terminal is restored 2022-02-16 10:54 [PATCH 0/3] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget @ 2022-02-16 10:54 ` Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget ` (2 subsequent siblings) 3 siblings, 0 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-16 10:54 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When disable_bits() changes the terminal attributes it uses sigchain_push_common() to restore the terminal if a signal is received before restore_term() is called. However there is no corresponding call to sigchain_pop_common() when the settings are restored so the signal handler is left on the sigchain stack. This leaves the stack unbalanced so code such as sigchain_push_common(my_handler); ... read_key_without_echo(...); ... sigchain_pop_common(); pops the handler pushed by disable_bits() rather than the one it intended to. Additionally "git add -p" changes the terminal settings every time it reads a key press so the stack can grow significantly. In order to fix this save_term() now sets up the signal handler so restore_term() can unconditionally call sigchain_pop_common(). There are no callers of save_term() outside of terminal.c as the only external caller was removed by e3f7e01b50 ("Revert "editor: save and reset terminal after calling EDITOR"", 2021-11-22). Any future callers of save_term() should benefit from having the signal handler set up for them. For example if we reinstate the code to protect against an editor failing to restore the terminal attributes we would want that code to restore the terminal attributes on SIGINT. (As an aside run_command() installs a signal handler that waits for the child before re-raising the signal so we would be guaranteed to restore the settings after the child has exited.) Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- compat/terminal.c | 17 +++++++++++++---- compat/terminal.h | 8 ++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/compat/terminal.c b/compat/terminal.c index 5b903e7c7e3..20803badd03 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -11,7 +11,7 @@ static void restore_term_on_signal(int sig) { restore_term(); - sigchain_pop(sig); + /* restore_term calls sigchain_pop_common */ raise(sig); } @@ -31,14 +31,20 @@ void restore_term(void) tcsetattr(term_fd, TCSAFLUSH, &old_term); close(term_fd); term_fd = -1; + sigchain_pop_common(); } int save_term(int full_duplex) { if (term_fd < 0) term_fd = open("/dev/tty", O_RDWR); + if (term_fd < 0) + return -1; + if (tcgetattr(term_fd, &old_term) < 0) + return -1; + sigchain_push_common(restore_term_on_signal); - return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term); + return 0; } static int disable_bits(tcflag_t bits) @@ -49,12 +55,12 @@ static int disable_bits(tcflag_t bits) goto error; t = old_term; - sigchain_push_common(restore_term_on_signal); t.c_lflag &= ~bits; if (!tcsetattr(term_fd, TCSAFLUSH, &t)) return 0; + sigchain_pop_common(); error: close(term_fd); term_fd = -1; @@ -100,6 +106,8 @@ void restore_term(void) return; } + sigchain_pop_common(); + if (hconin == INVALID_HANDLE_VALUE) return; @@ -134,6 +142,7 @@ int save_term(int full_duplex) GetConsoleMode(hconin, &cmode_in); use_stty = 0; + sigchain_push_common(restore_term_on_signal); return 0; error: CloseHandle(hconin); @@ -177,10 +186,10 @@ static int disable_bits(DWORD bits) if (save_term(0) < 0) return -1; - sigchain_push_common(restore_term_on_signal); if (!SetConsoleMode(hconin, cmode_in & ~bits)) { CloseHandle(hconin); hconin = INVALID_HANDLE_VALUE; + sigchain_pop_common(); return -1; } diff --git a/compat/terminal.h b/compat/terminal.h index e1770c575b2..0fb9fa147c4 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -1,7 +1,15 @@ #ifndef COMPAT_TERMINAL_H #define COMPAT_TERMINAL_H +/* + * Save the terminal attributes so they can be restored later by a + * call to restore_term(). Note that every successful call to + * save_term() must be matched by a call to restore_term() even if the + * attributes have not been changed. Returns 0 on success, -1 on + * failure. + */ int save_term(int full_duplex); +/* Restore the terminal attributes that were saved with save_term() */ void restore_term(void); char *git_terminal_prompt(const char *prompt, int echo); -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode 2022-02-16 10:54 [PATCH 0/3] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 1/3] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget @ 2022-02-16 10:54 ` Phillip Wood via GitGitGadget 2022-02-16 21:40 ` Junio C Hamano 2022-02-16 10:54 ` [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 3 siblings, 1 reply; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-16 10:54 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If VMIN and VTIME are both set to zero then the terminal performs non-blocking reads which means that read_key_without_echo() returns EOF if there is no key press pending. This results in the user being unable to select anything when running "git add -p". Fix this by explicitly setting VMIN and VTIME when enabling non-canonical mode. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- compat/terminal.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index 20803badd03..d882dfa06e3 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits) t = old_term; t.c_lflag &= ~bits; + if (bits & ICANON) { + t.c_cc[VMIN] = 1; + t.c_cc[VTIME] = 0; + } if (!tcsetattr(term_fd, TCSAFLUSH, &t)) return 0; @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits) if (bits & ENABLE_LINE_INPUT) { string_list_append(&stty_restore, "icanon"); - strvec_push(&cp.args, "-icanon"); + /* + * POSIX allows VMIN and VTIME to overlap with VEOF and + * VEOL - let's hope that is not the case on windows. + */ + strvec_pushl(&cp.args, "-icanon", "min", "1", "time", "0", NULL); } if (bits & ENABLE_ECHO_INPUT) { -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode 2022-02-16 10:54 ` [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget @ 2022-02-16 21:40 ` Junio C Hamano 2022-02-17 10:59 ` Phillip Wood 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2022-02-16 21:40 UTC (permalink / raw) To: Phillip Wood via GitGitGadget Cc: git, Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > If VMIN and VTIME are both set to zero then the terminal performs > non-blocking reads which means that read_key_without_echo() returns > EOF if there is no key press pending. This results in the user being > unable to select anything when running "git add -p". Fix this by > explicitly setting VMIN and VTIME when enabling non-canonical mode. Makes sense. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > compat/terminal.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/compat/terminal.c b/compat/terminal.c > index 20803badd03..d882dfa06e3 100644 > --- a/compat/terminal.c > +++ b/compat/terminal.c > @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits) > t = old_term; > > t.c_lflag &= ~bits; > + if (bits & ICANON) { > + t.c_cc[VMIN] = 1; > + t.c_cc[VTIME] = 0; > + } Quite sensible. I wonder if we can simplify the "are we looking at an ESC that is the first byte of a multi-byte control sequence?" logic in the caller by using inter-character delay, but it is probably better to go one step at a time like this patch does. > if (!tcsetattr(term_fd, TCSAFLUSH, &t)) > return 0; > > @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits) > > if (bits & ENABLE_LINE_INPUT) { > string_list_append(&stty_restore, "icanon"); > - strvec_push(&cp.args, "-icanon"); > + /* > + * POSIX allows VMIN and VTIME to overlap with VEOF and > + * VEOL - let's hope that is not the case on windows. > + */ > + strvec_pushl(&cp.args, "-icanon", "min", "1", "time", "0", NULL); Interesting. So each call to read_key_without_echo() ends up being a run_command("stty -icanon min 1 time 0") followed by a read followed by another "stty". At least while in -icanon mode, VEOF and VEOL do not take effect, and the potential overlap would not matter. It really depends on what happens upon restore. Do we have similar "let's hope" on the tcsetattr() side, too? > } > > if (bits & ENABLE_ECHO_INPUT) { Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode 2022-02-16 21:40 ` Junio C Hamano @ 2022-02-17 10:59 ` Phillip Wood 0 siblings, 0 replies; 18+ messages in thread From: Phillip Wood @ 2022-02-17 10:59 UTC (permalink / raw) To: Junio C Hamano, Phillip Wood via GitGitGadget Cc: git, Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood On 16/02/2022 21:40, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> If VMIN and VTIME are both set to zero then the terminal performs >> non-blocking reads which means that read_key_without_echo() returns >> EOF if there is no key press pending. This results in the user being >> unable to select anything when running "git add -p". Fix this by >> explicitly setting VMIN and VTIME when enabling non-canonical mode. > > Makes sense. > >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> compat/terminal.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/compat/terminal.c b/compat/terminal.c >> index 20803badd03..d882dfa06e3 100644 >> --- a/compat/terminal.c >> +++ b/compat/terminal.c >> @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits) >> t = old_term; >> >> t.c_lflag &= ~bits; >> + if (bits & ICANON) { >> + t.c_cc[VMIN] = 1; >> + t.c_cc[VTIME] = 0; >> + } > > Quite sensible. I wonder if we can simplify the "are we looking at > an ESC that is the first byte of a multi-byte control sequence?" > logic in the caller by using inter-character delay, but it is > probably better to go one step at a time like this patch does. I wondered about that too, but we'd need to keep the poll() for windows I think (at least in the case where it is using the windows console rather that mintty) so I'm not sure that it ends up any simpler than special casing poll on macos. >> if (!tcsetattr(term_fd, TCSAFLUSH, &t)) >> return 0; >> >> @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits) >> >> if (bits & ENABLE_LINE_INPUT) { >> string_list_append(&stty_restore, "icanon"); >> - strvec_push(&cp.args, "-icanon"); >> + /* >> + * POSIX allows VMIN and VTIME to overlap with VEOF and >> + * VEOL - let's hope that is not the case on windows. >> + */ >> + strvec_pushl(&cp.args, "-icanon", "min", "1", "time", "0", NULL); > > Interesting. So each call to read_key_without_echo() ends up being > a run_command("stty -icanon min 1 time 0") followed by a read > followed by another "stty". > > At least while in -icanon mode, VEOF and VEOL do not take effect, > and the potential overlap would not matter. It really depends on > what happens upon restore. Indeed, we could run another stty process so we can remember the original VEOF and VEOL but that's more complexity and forking. > Do we have similar "let's hope" on the tcsetattr() side, too? We're OK there because we modify a copy of the original attributes when changing the settings and use the unmodified originals when restoring them. Best Wishes Phillip >> } >> >> if (bits & ENABLE_ECHO_INPUT) { > > Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set 2022-02-16 10:54 [PATCH 0/3] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 1/3] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget @ 2022-02-16 10:54 ` Phillip Wood via GitGitGadget 2022-02-16 21:43 ` Junio C Hamano 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 3 siblings, 1 reply; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-16 10:54 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The builtin "add -p" reads the key "F2" as three separate keys "^[", "O" and "Q". The "Q" causes it to quit which is probably not what the user was expecting. This is because it uses poll() to check for pending input when reading escape sequences but reads the input with getchar() which is buffered by default and so hoovers up all the pending input leading poll() think there isn't anything pending. Fix this by calling setbuf() to disable input buffering if interactive.singlekey is set. Looking at the comment above mingw_getchar() in terminal.c I wonder if that function is papering over this bug and could be removed. Unfortunately I don't have access to windows to test that. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- add-interactive.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 6498ae196f1..ad78774ca26 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) &s->interactive_diff_algorithm); git_config_get_bool("interactive.singlekey", &s->use_single_key); + if (s->use_single_key) + setbuf(stdin, NULL); } void clear_add_i_state(struct add_i_state *s) -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set 2022-02-16 10:54 ` [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget @ 2022-02-16 21:43 ` Junio C Hamano 0 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2022-02-16 21:43 UTC (permalink / raw) To: Phillip Wood via GitGitGadget Cc: git, Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/add-interactive.c b/add-interactive.c > index 6498ae196f1..ad78774ca26 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) > &s->interactive_diff_algorithm); > > git_config_get_bool("interactive.singlekey", &s->use_single_key); > + if (s->use_single_key) > + setbuf(stdin, NULL); OK. Will queue. Thanks. > } > > void clear_add_i_state(struct add_i_state *s) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-02-16 10:54 [PATCH 0/3] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget ` (2 preceding siblings ...) 2022-02-16 10:54 ` [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget @ 2022-02-22 18:53 ` Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 1/4] terminal: always reset terminal when reading without echo Phillip Wood via GitGitGadget ` (4 more replies) 3 siblings, 5 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-22 18:53 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood I have added a new patch to the beginning of the series that fixes a case where we did not call restore_term() when leaving read_key_without_echo(). I have also reworded the commit message to patch 2 as SIGINT is actually ignored while the editor is running (we should probably change that code to use wait_after_clean instead). Cover letter for V1: Fix a couple of bugs I noticed when using the builtin "add -p" with interactive.singlekey=true. The first patch is a general fix for the terminal save/restore functionality which forgot to call sigchain_pop() when it restored the terminal. The last two fix reading single keys in non-canonical mode by making sure we wait for an initial key press and only read one byte at a time from the underlying file descriptor. Note that these are untested on windows beyond our CI compiling them Phillip Wood (4): terminal: always reset terminal when reading without echo terminal: pop signal handler when terminal is restored terminal: set VMIN and VTIME in non-canonical mode add -p: disable stdin buffering when interactive.singlekey is set add-interactive.c | 2 ++ compat/terminal.c | 29 +++++++++++++++++++++++------ compat/terminal.h | 8 ++++++++ 3 files changed, 33 insertions(+), 6 deletions(-) base-commit: b80121027d1247a0754b3cc46897fee75c050b44 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1146 Range-diff vs v1: -: ----------- > 1: 45609d61afc terminal: always reset terminal when reading without echo 1: 9a3c2cea0f9 ! 2: 0020953f1cf terminal: pop signal handler when terminal is restored @@ Commit message external caller was removed by e3f7e01b50 ("Revert "editor: save and reset terminal after calling EDITOR"", 2021-11-22). Any future callers of save_term() should benefit from having the signal handler set up - for them. For example if we reinstate the code to protect against an - editor failing to restore the terminal attributes we would want that - code to restore the terminal attributes on SIGINT. (As an aside - run_command() installs a signal handler that waits for the child - before re-raising the signal so we would be guaranteed to restore the - settings after the child has exited.) + for them. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> 2: 02009172e08 = 3: 7ae9b236554 terminal: set VMIN and VTIME in non-canonical mode 3: 6d8423b6e1e = 4: 39b061a471b add -p: disable stdin buffering when interactive.singlekey is set -- gitgitgadget ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/4] terminal: always reset terminal when reading without echo 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget @ 2022-02-22 18:53 ` Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 2/4] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget ` (3 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-22 18:53 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Break out of the loop to ensure restore_term() is called before returning. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- compat/terminal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index 5b903e7c7e3..fb8c70a6251 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -385,7 +385,7 @@ int read_key_without_echo(struct strbuf *buf) ch = getchar(); if (ch == EOF) - return 0; + break; strbuf_addch(buf, ch); } } -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/4] terminal: pop signal handler when terminal is restored 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 1/4] terminal: always reset terminal when reading without echo Phillip Wood via GitGitGadget @ 2022-02-22 18:53 ` Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 3/4] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-22 18:53 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When disable_bits() changes the terminal attributes it uses sigchain_push_common() to restore the terminal if a signal is received before restore_term() is called. However there is no corresponding call to sigchain_pop_common() when the settings are restored so the signal handler is left on the sigchain stack. This leaves the stack unbalanced so code such as sigchain_push_common(my_handler); ... read_key_without_echo(...); ... sigchain_pop_common(); pops the handler pushed by disable_bits() rather than the one it intended to. Additionally "git add -p" changes the terminal settings every time it reads a key press so the stack can grow significantly. In order to fix this save_term() now sets up the signal handler so restore_term() can unconditionally call sigchain_pop_common(). There are no callers of save_term() outside of terminal.c as the only external caller was removed by e3f7e01b50 ("Revert "editor: save and reset terminal after calling EDITOR"", 2021-11-22). Any future callers of save_term() should benefit from having the signal handler set up for them. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- compat/terminal.c | 17 +++++++++++++---- compat/terminal.h | 8 ++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/compat/terminal.c b/compat/terminal.c index fb8c70a6251..11288cfe5c9 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -11,7 +11,7 @@ static void restore_term_on_signal(int sig) { restore_term(); - sigchain_pop(sig); + /* restore_term calls sigchain_pop_common */ raise(sig); } @@ -31,14 +31,20 @@ void restore_term(void) tcsetattr(term_fd, TCSAFLUSH, &old_term); close(term_fd); term_fd = -1; + sigchain_pop_common(); } int save_term(int full_duplex) { if (term_fd < 0) term_fd = open("/dev/tty", O_RDWR); + if (term_fd < 0) + return -1; + if (tcgetattr(term_fd, &old_term) < 0) + return -1; + sigchain_push_common(restore_term_on_signal); - return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term); + return 0; } static int disable_bits(tcflag_t bits) @@ -49,12 +55,12 @@ static int disable_bits(tcflag_t bits) goto error; t = old_term; - sigchain_push_common(restore_term_on_signal); t.c_lflag &= ~bits; if (!tcsetattr(term_fd, TCSAFLUSH, &t)) return 0; + sigchain_pop_common(); error: close(term_fd); term_fd = -1; @@ -100,6 +106,8 @@ void restore_term(void) return; } + sigchain_pop_common(); + if (hconin == INVALID_HANDLE_VALUE) return; @@ -134,6 +142,7 @@ int save_term(int full_duplex) GetConsoleMode(hconin, &cmode_in); use_stty = 0; + sigchain_push_common(restore_term_on_signal); return 0; error: CloseHandle(hconin); @@ -177,10 +186,10 @@ static int disable_bits(DWORD bits) if (save_term(0) < 0) return -1; - sigchain_push_common(restore_term_on_signal); if (!SetConsoleMode(hconin, cmode_in & ~bits)) { CloseHandle(hconin); hconin = INVALID_HANDLE_VALUE; + sigchain_pop_common(); return -1; } diff --git a/compat/terminal.h b/compat/terminal.h index e1770c575b2..0fb9fa147c4 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -1,7 +1,15 @@ #ifndef COMPAT_TERMINAL_H #define COMPAT_TERMINAL_H +/* + * Save the terminal attributes so they can be restored later by a + * call to restore_term(). Note that every successful call to + * save_term() must be matched by a call to restore_term() even if the + * attributes have not been changed. Returns 0 on success, -1 on + * failure. + */ int save_term(int full_duplex); +/* Restore the terminal attributes that were saved with save_term() */ void restore_term(void); char *git_terminal_prompt(const char *prompt, int echo); -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] terminal: set VMIN and VTIME in non-canonical mode 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 1/4] terminal: always reset terminal when reading without echo Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 2/4] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget @ 2022-02-22 18:53 ` Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 4/4] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget 2022-02-23 21:34 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Junio C Hamano 4 siblings, 0 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-22 18:53 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If VMIN and VTIME are both set to zero then the terminal performs non-blocking reads which means that read_key_without_echo() returns EOF if there is no key press pending. This results in the user being unable to select anything when running "git add -p". Fix this by explicitly setting VMIN and VTIME when enabling non-canonical mode. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- compat/terminal.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index 11288cfe5c9..3620184e790 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits) t = old_term; t.c_lflag &= ~bits; + if (bits & ICANON) { + t.c_cc[VMIN] = 1; + t.c_cc[VTIME] = 0; + } if (!tcsetattr(term_fd, TCSAFLUSH, &t)) return 0; @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits) if (bits & ENABLE_LINE_INPUT) { string_list_append(&stty_restore, "icanon"); - strvec_push(&cp.args, "-icanon"); + /* + * POSIX allows VMIN and VTIME to overlap with VEOF and + * VEOL - let's hope that is not the case on windows. + */ + strvec_pushl(&cp.args, "-icanon", "min", "1", "time", "0", NULL); } if (bits & ENABLE_ECHO_INPUT) { -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] add -p: disable stdin buffering when interactive.singlekey is set 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget ` (2 preceding siblings ...) 2022-02-22 18:53 ` [PATCH v2 3/4] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget @ 2022-02-22 18:53 ` Phillip Wood via GitGitGadget 2022-02-23 21:34 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Junio C Hamano 4 siblings, 0 replies; 18+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-02-22 18:53 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The builtin "add -p" reads the key "F2" as three separate keys "^[", "O" and "Q". The "Q" causes it to quit which is probably not what the user was expecting. This is because it uses poll() to check for pending input when reading escape sequences but reads the input with getchar() which is buffered by default and so hoovers up all the pending input leading poll() think there isn't anything pending. Fix this by calling setbuf() to disable input buffering if interactive.singlekey is set. Looking at the comment above mingw_getchar() in terminal.c I wonder if that function is papering over this bug and could be removed. Unfortunately I don't have access to windows to test that. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- add-interactive.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 6498ae196f1..ad78774ca26 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) &s->interactive_diff_algorithm); git_config_get_bool("interactive.singlekey", &s->use_single_key); + if (s->use_single_key) + setbuf(stdin, NULL); } void clear_add_i_state(struct add_i_state *s) -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget ` (3 preceding siblings ...) 2022-02-22 18:53 ` [PATCH v2 4/4] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget @ 2022-02-23 21:34 ` Junio C Hamano 2022-02-24 14:30 ` Phillip Wood 4 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2022-02-23 21:34 UTC (permalink / raw) To: Phillip Wood via GitGitGadget Cc: git, Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > I have added a new patch to the beginning of the series that fixes a case > where we did not call restore_term() when leaving read_key_without_echo(). I > have also reworded the commit message to patch 2 as SIGINT is actually > ignored while the editor is running (we should probably change that code to > use wait_after_clean instead). All patches looked good. Thanks. Let's mark it for 'next' and below soonish. > > Cover letter for V1: > > Fix a couple of bugs I noticed when using the builtin "add -p" with > interactive.singlekey=true. The first patch is a general fix for the > terminal save/restore functionality which forgot to call sigchain_pop() when > it restored the terminal. The last two fix reading single keys in > non-canonical mode by making sure we wait for an initial key press and only > read one byte at a time from the underlying file descriptor. > > Note that these are untested on windows beyond our CI compiling them > > Phillip Wood (4): > terminal: always reset terminal when reading without echo > terminal: pop signal handler when terminal is restored > terminal: set VMIN and VTIME in non-canonical mode > add -p: disable stdin buffering when interactive.singlekey is set > > add-interactive.c | 2 ++ > compat/terminal.c | 29 +++++++++++++++++++++++------ > compat/terminal.h | 8 ++++++++ > 3 files changed, 33 insertions(+), 6 deletions(-) > > > base-commit: b80121027d1247a0754b3cc46897fee75c050b44 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1146 > > Range-diff vs v1: > > -: ----------- > 1: 45609d61afc terminal: always reset terminal when reading without echo > 1: 9a3c2cea0f9 ! 2: 0020953f1cf terminal: pop signal handler when terminal is restored > @@ Commit message > external caller was removed by e3f7e01b50 ("Revert "editor: save and > reset terminal after calling EDITOR"", 2021-11-22). Any future callers > of save_term() should benefit from having the signal handler set up > - for them. For example if we reinstate the code to protect against an > - editor failing to restore the terminal attributes we would want that > - code to restore the terminal attributes on SIGINT. (As an aside > - run_command() installs a signal handler that waits for the child > - before re-raising the signal so we would be guaranteed to restore the > - settings after the child has exited.) > + for them. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > 2: 02009172e08 = 3: 7ae9b236554 terminal: set VMIN and VTIME in non-canonical mode > 3: 6d8423b6e1e = 4: 39b061a471b add -p: disable stdin buffering when interactive.singlekey is set ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-02-23 21:34 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Junio C Hamano @ 2022-02-24 14:30 ` Phillip Wood 2022-03-22 20:18 ` Carlo Arenas 0 siblings, 1 reply; 18+ messages in thread From: Phillip Wood @ 2022-02-24 14:30 UTC (permalink / raw) To: Junio C Hamano, Phillip Wood via GitGitGadget Cc: git, Johannes Schindelin, Carlo Marcelo Arenas Belón, Phillip Wood Hi Junio On 23/02/2022 21:34, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> I have added a new patch to the beginning of the series that fixes a case >> where we did not call restore_term() when leaving read_key_without_echo(). I >> have also reworded the commit message to patch 2 as SIGINT is actually >> ignored while the editor is running (we should probably change that code to >> use wait_after_clean instead). > > All patches looked good. Thanks. > > Let's mark it for 'next' and below soonish. That sounds good. I've got a couple more patches based on top of these which hopefully fix the remaining problems (notably the macos poll() bug). I'll polish and post them next week. Once those are in I hope we'll be able to enable the builtin "add -p" by default. Thanks Phillip >> >> Cover letter for V1: >> >> Fix a couple of bugs I noticed when using the builtin "add -p" with >> interactive.singlekey=true. The first patch is a general fix for the >> terminal save/restore functionality which forgot to call sigchain_pop() when >> it restored the terminal. The last two fix reading single keys in >> non-canonical mode by making sure we wait for an initial key press and only >> read one byte at a time from the underlying file descriptor. >> >> Note that these are untested on windows beyond our CI compiling them >> >> Phillip Wood (4): >> terminal: always reset terminal when reading without echo >> terminal: pop signal handler when terminal is restored >> terminal: set VMIN and VTIME in non-canonical mode >> add -p: disable stdin buffering when interactive.singlekey is set >> >> add-interactive.c | 2 ++ >> compat/terminal.c | 29 +++++++++++++++++++++++------ >> compat/terminal.h | 8 ++++++++ >> 3 files changed, 33 insertions(+), 6 deletions(-) >> >> >> base-commit: b80121027d1247a0754b3cc46897fee75c050b44 >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v2 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v2 >> Pull-Request: https://github.com/gitgitgadget/git/pull/1146 >> >> Range-diff vs v1: >> >> -: ----------- > 1: 45609d61afc terminal: always reset terminal when reading without echo >> 1: 9a3c2cea0f9 ! 2: 0020953f1cf terminal: pop signal handler when terminal is restored >> @@ Commit message >> external caller was removed by e3f7e01b50 ("Revert "editor: save and >> reset terminal after calling EDITOR"", 2021-11-22). Any future callers >> of save_term() should benefit from having the signal handler set up >> - for them. For example if we reinstate the code to protect against an >> - editor failing to restore the terminal attributes we would want that >> - code to restore the terminal attributes on SIGINT. (As an aside >> - run_command() installs a signal handler that waits for the child >> - before re-raising the signal so we would be guaranteed to restore the >> - settings after the child has exited.) >> + for them. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 2: 02009172e08 = 3: 7ae9b236554 terminal: set VMIN and VTIME in non-canonical mode >> 3: 6d8423b6e1e = 4: 39b061a471b add -p: disable stdin buffering when interactive.singlekey is set ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-02-24 14:30 ` Phillip Wood @ 2022-03-22 20:18 ` Carlo Arenas 2022-03-23 9:03 ` Junio C Hamano 2022-03-28 10:51 ` Phillip Wood 0 siblings, 2 replies; 18+ messages in thread From: Carlo Arenas @ 2022-03-22 20:18 UTC (permalink / raw) To: phillip.wood Cc: Junio C Hamano, Phillip Wood via GitGitGadget, git, Johannes Schindelin On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > That sounds good. I've got a couple more patches based on top of these > which hopefully fix the remaining problems (notably the macos poll() > bug). I'll polish and post them next week. Once those are in I hope > we'll be able to enable the builtin "add -p" by default. As this topic just hit master noticed (I apologize for not doing it sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed with 1/4) and therefore enabling the builtin by default as proposed originally by dscho could proceed without the additional series. Carlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-03-22 20:18 ` Carlo Arenas @ 2022-03-23 9:03 ` Junio C Hamano 2022-03-24 13:29 ` Johannes Schindelin 2022-03-28 10:51 ` Phillip Wood 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2022-03-23 9:03 UTC (permalink / raw) To: Carlo Arenas Cc: phillip.wood, Phillip Wood via GitGitGadget, git, Johannes Schindelin Carlo Arenas <carenas@gmail.com> writes: > On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> That sounds good. I've got a couple more patches based on top of these >> which hopefully fix the remaining problems (notably the macos poll() >> bug). I'll polish and post them next week. Once those are in I hope >> we'll be able to enable the builtin "add -p" by default. > > As this topic just hit master noticed (I apologize for not doing it > sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed > with 1/4) and therefore enabling the builtin by default as proposed > originally by dscho could proceed without the additional series. As long as it is a belated success report, nobody would mind. A belated failure report would be a cause of sorrow and grief, but even then, it is better to have it late than never. Thanks for testing. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-03-23 9:03 ` Junio C Hamano @ 2022-03-24 13:29 ` Johannes Schindelin 0 siblings, 0 replies; 18+ messages in thread From: Johannes Schindelin @ 2022-03-24 13:29 UTC (permalink / raw) To: Junio C Hamano Cc: Carlo Arenas, phillip.wood, Phillip Wood via GitGitGadget, git Hi, On Wed, 23 Mar 2022, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > > > On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> That sounds good. I've got a couple more patches based on top of these > >> which hopefully fix the remaining problems (notably the macos poll() > >> bug). I'll polish and post them next week. Once those are in I hope > >> we'll be able to enable the builtin "add -p" by default. > > > > As this topic just hit master noticed (I apologize for not doing it > > sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed > > with 1/4) and therefore enabling the builtin by default as proposed > > originally by dscho could proceed without the additional series. > > As long as it is a belated success report, nobody would mind. A > belated failure report would be a cause of sorrow and grief, but > even then, it is better to have it late than never. > > Thanks for testing. FWIW I tried to reproduce the reported issues, but since I do not have any hardware running macOS anymore, I only had the avenue to debug via `tmate` on one of GitHub Actions' macOS agents, but the problem did not reproduce for me via SSH. I am grateful for the great work put in by Carlo and Phillip to fix these issues. Thanks, Dscho ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes 2022-03-22 20:18 ` Carlo Arenas 2022-03-23 9:03 ` Junio C Hamano @ 2022-03-28 10:51 ` Phillip Wood 1 sibling, 0 replies; 18+ messages in thread From: Phillip Wood @ 2022-03-28 10:51 UTC (permalink / raw) To: Carlo Arenas, phillip.wood Cc: Junio C Hamano, Phillip Wood via GitGitGadget, git, Johannes Schindelin Hi Carlo On 22/03/2022 20:18, Carlo Arenas wrote: > On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> That sounds good. I've got a couple more patches based on top of these >> which hopefully fix the remaining problems (notably the macos poll() >> bug). I'll polish and post them next week. Once those are in I hope >> we'll be able to enable the builtin "add -p" by default. > > As this topic just hit master noticed (I apologize for not doing it > sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed > with 1/4) and therefore enabling the builtin by default as proposed > originally by dscho could proceed without the additional series. Thanks for testing this series Best Wishes Phillip ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-03-28 10:51 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-16 10:54 [PATCH 0/3] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 1/3] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget 2022-02-16 10:54 ` [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget 2022-02-16 21:40 ` Junio C Hamano 2022-02-17 10:59 ` Phillip Wood 2022-02-16 10:54 ` [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget 2022-02-16 21:43 ` Junio C Hamano 2022-02-22 18:53 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 1/4] terminal: always reset terminal when reading without echo Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 2/4] terminal: pop signal handler when terminal is restored Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 3/4] terminal: set VMIN and VTIME in non-canonical mode Phillip Wood via GitGitGadget 2022-02-22 18:53 ` [PATCH v2 4/4] add -p: disable stdin buffering when interactive.singlekey is set Phillip Wood via GitGitGadget 2022-02-23 21:34 ` [PATCH v2 0/4] a couple of read_key_without_echo() fixes Junio C Hamano 2022-02-24 14:30 ` Phillip Wood 2022-03-22 20:18 ` Carlo Arenas 2022-03-23 9:03 ` Junio C Hamano 2022-03-24 13:29 ` Johannes Schindelin 2022-03-28 10:51 ` Phillip Wood
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).