| Message ID | 02009172e081ab5c4de8e2476c1142f97b41698e.1645008873.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | a couple of read_key_without_echo() fixes | expand |
"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.
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.
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) {