a couple of read_key_without_echo() fixes

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

From: Phillip Wood <[email protected]>

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

---
 compat/terminal.c | 17 +++++++++++++----
 compat/terminal.h |  8 ++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

index 5b903e7c7e3..20803badd03 100644
@@ -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;
 	}
 

index e1770c575b2..0fb9fa147c4 100644
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 compat/terminal.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 20803badd03..d882dfa06e3 100644
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 add-interactive.c | 2 ++
 1 file changed, 2 insertions(+)

index 6498ae196f1..ad78774ca26 100644
@@ -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

Makes sense.

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.

Indeed, we could run another stty process so we can remember the original VEOF and VEOL but that's more complexity and forking.

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

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.

 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

From: Phillip Wood <[email protected]>

Break out of the loop to ensure restore_term() is called before returning.

---
 compat/terminal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5b903e7c7e3..fb8c70a6251 100644
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 compat/terminal.c | 17 +++++++++++++----
 compat/terminal.h |  8 ++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

index fb8c70a6251..11288cfe5c9 100644
@@ -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;
 	}
 

index e1770c575b2..0fb9fa147c4 100644
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 compat/terminal.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 11288cfe5c9..3620184e790 100644
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 add-interactive.c | 2 ++
 1 file changed, 2 insertions(+)

index 6498ae196f1..ad78774ca26 100644
@@ -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

All patches looked good. Thanks.

Let's mark it for 'next' and below soonish.

Hi Junio

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

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

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.

Hi,

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

Hi Carlo

Thanks for testing this series

Best Wishes

Phillip