[PATCH 0/3] a couple of read_key_without_echo() fixes

[PATCH 0/3] a couple of read_key_without_echo() fixes

[Phillip Wood via GitGitGadget]

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

[PATCH 1/3] terminal: pop signal handler when terminal is restored

[Phillip Wood via GitGitGadget]

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

[PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode

[Phillip Wood via GitGitGadget]

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

Re: [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode

[Junio C Hamano]

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

Re: [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode

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

[PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set

[Phillip Wood via GitGitGadget]

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

Re: [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set

[Junio C Hamano]

"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)

[PATCH v2 0/4] a couple of read_key_without_echo() fixes

[Phillip Wood via GitGitGadget]

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

[PATCH v2 1/4] terminal: always reset terminal when reading without echo

[Phillip Wood via GitGitGadget]

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

[PATCH v2 2/4] terminal: pop signal handler when terminal is restored

[Phillip Wood via GitGitGadget]

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

[PATCH v2 3/4] terminal: set VMIN and VTIME in non-canonical mode

[Phillip Wood via GitGitGadget]

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

[PATCH v2 4/4] add -p: disable stdin buffering when interactive.singlekey is set

[Phillip Wood via GitGitGadget]

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

Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes

[Junio C Hamano]

"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

Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes

[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

Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes

[Carlo Arenas]

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

Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes

[Junio C Hamano]

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.

Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes

[Johannes Schindelin]

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

Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes

[Phillip Wood]

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