{"thread":{"id":"57509","subject":"[PATCH 0/4] builtin add -p: hopefully final readkey fixes","startedAt":"2022-03-04T13:11:34Z","lastAt":"2022-03-16T18:54:25Z","messageCount":53,"participants":["Phillip Wood","Ramsay Jones","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano","Carlo Arenas","rsbecker@nexbridge.com"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"450389","messageId":"20220304131126.8293-1-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":null,"subject":"[PATCH 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-04T13:11:22Z","receivedAt":"2022-03-04T13:11:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nFix the remaining issues that I'm aware of when using the built in\n\"add -p\" with interactive.singlekey that are stopping it from being\nmerged to master. The first three patches make sure that we call\ntcsetattr() and the same file descriptor that we use for read() and\nwork around poll() being broken when reading from terminals on\nmacos. The final patch is more of an improvement rather than a bug fix\n(the same issue already exists in the perl version) and could proceed\nseparately.\n\nUnfortunately these patches conflict with\n'cb/save-term-across-editor-invocation' as well as the textual\nconflicts there is a semantic conflict as the argument to save_term()\nis changed so the code in editor.c will need updating.\n\nThese patches are based on 'pw/single-key-interactive'\n\nPhillip Wood (4):\n terminal: use flags for save_term()\n terminal: don't assume stdin is /dev/tty\n terminal: work around macos poll() bug\n terminal: restore settings on SIGTSTP\n\n compat/terminal.c | 205 +++++++++++++++++++++++++++++++++++++++-------\n compat/terminal.h | 6 +-\n 2 files changed, 182 insertions(+), 29 deletions(-)\n\n-- \n2.35.1\n\n"},{"id":"450390","messageId":"20220304131126.8293-2-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-04T13:11:23Z","receivedAt":"2022-03-04T13:11:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe next commit will add another flag in addition to the existing\nfull_duplex so change the function signature to take an unsigned flags\nargument. Also alter the functions that call save_term() so that they\ncan pass flags down to it.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 41 +++++++++++++++++++++--------------------\n compat/terminal.h | 5 ++++-\n 2 files changed, 25 insertions(+), 21 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex d882dfa06e..bad8e04cd8 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -34,7 +34,7 @@ void restore_term(void)\n \tsigchain_pop_common();\n }\n \n-int save_term(int full_duplex)\n+int save_term(unsigned flags)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n@@ -47,11 +47,11 @@ int save_term(int full_duplex)\n \treturn 0;\n }\n \n-static int disable_bits(tcflag_t bits)\n+static int disable_bits(unsigned flags, tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\tgoto error;\n \n \tt = old_term;\n@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)\n \treturn -1;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(unsigned flags)\n {\n-\treturn disable_bits(ECHO);\n+\treturn disable_bits(flags, ECHO);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(unsigned flags)\n {\n-\treturn disable_bits(ICANON | ECHO);\n+\treturn disable_bits(flags, ICANON | ECHO);\n }\n \n #elif defined(GIT_WINDOWS_NATIVE)\n@@ -126,15 +126,15 @@ void restore_term(void)\n \thconin = hconout = INVALID_HANDLE_VALUE;\n }\n \n-int save_term(int full_duplex)\n+int save_term(unsigned flags)\n {\n \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n \t FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn -1;\n \n-\tif (full_duplex) {\n+\tif (flags & SAVE_TERM_DUPLEX) {\n \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n@@ -154,7 +154,7 @@ int save_term(int full_duplex)\n \treturn -1;\n }\n \n-static int disable_bits(DWORD bits)\n+static int disable_bits(unsigned flags, DWORD bits)\n {\n \tif (use_stty) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\treturn -1;\n \n \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)\n \treturn 0;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(unsigned flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT);\n+\treturn disable_bits(ENABLE_ECHO_INPUT, flags);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(unsigned flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n+\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,\n+\t\t\t flags);\n }\n \n /*\n@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n \t\treturn NULL;\n \t}\n \n-\tif (!echo && disable_echo()) {\n+\tif (!echo && disable_echo(0)) {\n \t\tfclose(input_fh);\n \t\tfclose(output_fh);\n \t\treturn NULL;\n@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical() < 0) {\n+\tif (warning_displayed || enable_non_canonical(0) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\n@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)\n \n #else\n \n-int save_term(int full_duplex)\n+int save_term(unsigned flags)\n {\n-\t/* full_duplex == 1, but no support available */\n-\treturn -full_duplex;\n+\t/* no duplex support available */\n+\treturn -!!(flags & SAVE_TERM_DUPLEX);\n }\n \n void restore_term(void)\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex 0fb9fa147c..f24b91390d 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,14 +1,17 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+/* Save input and output settings */\n+#define SAVE_TERM_DUPLEX (1u << 0)\n+\n /*\n * Save the terminal attributes so they can be restored later by a\n * call to restore_term(). Note that every successful call to\n * save_term() must be matched by a call to restore_term() even if the\n * attributes have not been changed. Returns 0 on success, -1 on\n * failure.\n */\n-int save_term(int full_duplex);\n+int save_term(unsigned flags);\n /* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n-- \n2.35.1\n\n"},{"id":"450391","messageId":"20220304131126.8293-4-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH 3/4] terminal: work around macos poll() bug","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-04T13:11:25Z","receivedAt":"2022-03-04T13:11:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nOn macos the builtin \"add -p\" does not handle keys that generate\nescape sequences because poll() does not work with terminals\nthere. Switch to using select() on non-windows platforms to work\naround this.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 43 +++++++++++++++++++++++++++++++++++++------\n 1 file changed, 37 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 249836e78f..5d516ff546 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -82,6 +82,32 @@ static int enable_non_canonical(unsigned flags)\n \treturn disable_bits(flags, ICANON | ECHO);\n }\n \n+/*\n+ * On macos it is not possible to use poll() with a terminal so use select\n+ * instead.\n+ */\n+#include \n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct timeval tv, *tvp = NULL;\n+\tfd_set readfds;\n+\tint res;\n+\n+\tif (timeout >= 0) {\n+\t\ttv.tv_sec = timeout / 1000;\n+\t\ttv.tv_usec = (timeout % 1000) * 1000;\n+\t\ttvp = &tv;\n+\t}\n+\n+\tFD_ZERO(&readfds);\n+\tFD_SET(0, &readfds);\n+\tres = select(1, &readfds, NULL, NULL, tvp);\n+\tif (res < 0)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #elif defined(GIT_WINDOWS_NATIVE)\n \n #define INPUT_PATH \"CONIN$\"\n@@ -247,6 +273,16 @@ static int mingw_getchar(void)\n }\n #define getchar mingw_getchar\n \n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n+\n+\tif (poll(&pfd, 1, timeout) < 1)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #endif\n \n #ifndef FORCE_TEXT\n@@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)\n \t\t * half a second when we know that the sequence is complete.\n \t\t */\n \t\twhile (!is_known_escape_sequence(buf->buf)) {\n-\t\t\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n-\n-\t\t\tif (poll(&pfd, 1, 500) < 1)\n-\t\t\t\tbreak;\n-\n-\t\t\tch = getchar();\n+\t\t\tch = getchar_with_timeout(500);\n \t\t\tif (ch == EOF)\n \t\t\t\treturn 0;\n \t\t\tstrbuf_addch(buf, ch);\n-- \n2.35.1\n\n"},{"id":"450392","messageId":"20220304131126.8293-3-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-04T13:11:24Z","receivedAt":"2022-03-04T13:11:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nread_key_without_echo() reads from stdin but uses /dev/tty when it\ndisables echo. This is unfortunate as there no guarantee that stdin is\nthe same device as /dev/tty. The perl version of \"add -p\" uses stdin\nwhen it sets the terminal mode, this commit does the same for the\nbuiltin version. There is still a difference between the perl and\nbuiltin versions though - the perl version will ignore any errors when\nsetting the terminal mode[1] and will still read single bytes when\nstdin is not a terminal. The builtin version displays a warning if\nsetting the terminal mode fails and switches to reading a line at a\ntime.\n\n[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 5 +++--\n compat/terminal.h | 3 ++-\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex bad8e04cd8..249836e78f 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -37,7 +37,8 @@ void restore_term(void)\n int save_term(unsigned flags)\n {\n \tif (term_fd < 0)\n-\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n+\t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n \tif (term_fd < 0)\n \t\treturn -1;\n \tif (tcgetattr(term_fd, &old_term) < 0)\n@@ -362,7 +363,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical(0) < 0) {\n+\tif (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex f24b91390d..f5655d0d0b 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -3,7 +3,8 @@\n \n /* Save input and output settings */\n #define SAVE_TERM_DUPLEX (1u << 0)\n-\n+/* Save stdin rather than /dev/tty (fails is stdin is not a terminal) */\n+#define SAVE_TERM_STDIN (1u << 1)\n /*\n * Save the terminal attributes so they can be restored later by a\n * call to restore_term(). Note that every successful call to\n-- \n2.35.1\n\n"},{"id":"450393","messageId":"20220304131126.8293-5-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-04T13:11:26Z","receivedAt":"2022-03-04T13:11:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf the user suspends git while it is waiting for a keypress reset the\nterminal before stopping and restore the settings when git resumes. If\nthe user tries to resume in the background print an error\nmessage (taking care to use async safe functions) before stopping\nagain. Ideally we would reprint the prompt for the user when git\nresumes but this patch just restarts the read().\n\nThe signal handler is established with sigaction() rather than using\nsigchain_push() as this allows us to control the signal mask when the\nhandler is invoked and ensure SA_RESTART is used to restart the\nread() when resuming.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 124 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 120 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 5d516ff546..79ab54c2f8 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -1,4 +1,4 @@\n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"compat/terminal.h\"\n #include \"sigchain.h\"\n #include \"strbuf.h\"\n@@ -23,6 +23,89 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n+static char *background_resume_msg;\n+static char *restore_error_msg;\n+static volatile sig_atomic_t ttou_received;\n+\n+static void write_msg(const char *msg)\n+{\n+\twrite_in_full(2, msg, strlen(msg));\n+\twrite_in_full(2, \"\\n\", 1);\n+}\n+\n+static void print_background_resume_msg(int signo)\n+{\n+\tint saved_errno = errno;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\n+\tttou_received = 1;\n+\twrite_msg(background_resume_msg);\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\terrno = saved_errno;\n+}\n+\n+static void restore_terminal_on_suspend(int signo)\n+{\n+\tint saved_errno = errno;\n+\tint res;\n+\tstruct termios t;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\tint can_restore = 1;\n+\n+\tif (tcgetattr(term_fd, &t) < 0)\n+\t\tcan_restore = 0;\n+\n+\tif (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)\n+\t\twrite_msg(restore_error_msg);\n+\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\tif (!can_restore) {\n+\t\twrite_msg(restore_error_msg);\n+\t\tgoto out;\n+\t}\n+\t/*\n+\t * If we resume in the background then we receive SIGTTOU when calling\n+\t * tcsetattr() below. Set up a handler to print an error message in that\n+\t * case.\n+\t */\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, SIGTTOU);\n+\tsa.sa_mask = old_sa.sa_mask;\n+\tsa.sa_handler = print_background_resume_msg;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGTTOU, &sa, &old_sa);\n+ again:\n+\tttou_received = 0;\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\tres = tcsetattr(term_fd, TCSAFLUSH, &t);\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tif (ttou_received)\n+\t\tgoto again;\n+\telse if (res < 0)\n+\t\twrite_msg(restore_error_msg);\n+\tsigaction(SIGTTOU, &old_sa, NULL);\n+ out:\n+\terrno = saved_errno;\n+}\n+\n void restore_term(void)\n {\n \tif (term_fd < 0)\n@@ -32,10 +115,19 @@ void restore_term(void)\n \tclose(term_fd);\n \tterm_fd = -1;\n \tsigchain_pop_common();\n+\tif (restore_error_msg) {\n+\t\tsignal(SIGTTIN, SIG_DFL);\n+\t\tsignal(SIGTTOU, SIG_DFL);\n+\t\tsignal(SIGTSTP, SIG_DFL);\n+\t\tFREE_AND_NULL(restore_error_msg);\n+\t\tFREE_AND_NULL(background_resume_msg);\n+\t}\n }\n \n int save_term(unsigned flags)\n {\n+\tstruct sigaction sa;\n+\n \tif (term_fd < 0)\n \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n@@ -44,6 +136,26 @@ int save_term(unsigned flags)\n \tif (tcgetattr(term_fd, &old_term) < 0)\n \t\treturn -1;\n \tsigchain_push_common(restore_term_on_signal);\n+\t/*\n+\t * If job control is disabled then the shell will have set the\n+\t * disposition of SIGTSTP to SIG_IGN.\n+\t */\n+\tsigaction(SIGTSTP, NULL, &sa);\n+\tif (sa.sa_handler == SIG_IGN)\n+\t\treturn 0;\n+\n+\t/* avoid calling gettext() from signal handler */\n+\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n+\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n+\tsa.sa_handler = restore_terminal_on_suspend;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigemptyset(&sa.sa_mask);\n+\tsigaddset(&sa.sa_mask, SIGTSTP);\n+\tsigaddset(&sa.sa_mask, SIGTTIN);\n+\tsigaddset(&sa.sa_mask, SIGTTOU);\n+\tsigaction(SIGTSTP, &sa, NULL);\n+\tsigaction(SIGTTIN, &sa, NULL);\n+\tsigaction(SIGTTOU, &sa, NULL);\n \n \treturn 0;\n }\n@@ -93,6 +205,7 @@ static int getchar_with_timeout(int timeout)\n \tfd_set readfds;\n \tint res;\n \n+ again:\n \tif (timeout >= 0) {\n \t\ttv.tv_sec = timeout / 1000;\n \t\ttv.tv_usec = (timeout % 1000) * 1000;\n@@ -102,9 +215,12 @@ static int getchar_with_timeout(int timeout)\n \tFD_ZERO(&readfds);\n \tFD_SET(0, &readfds);\n \tres = select(1, &readfds, NULL, NULL, tvp);\n-\tif (res < 0)\n-\t\treturn EOF;\n-\n+\tif (res < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tgoto again;\n+\t\telse\n+\t\t\treturn EOF;\n+\t}\n \treturn getchar();\n }\n \n-- \n2.35.1\n\n"},{"id":"450438","messageId":"0a8b4a1d-f6e7-dc8f-3496-c2c96a25bcda@ramsayjones.plus.com","threadId":"57509","inReplyTo":"20220304131126.8293-3-phillip.wood123@gmail.com","subject":"Re: [PATCH 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2022-03-04T20:42:40Z","receivedAt":"2022-03-04T20:42:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/03/2022 13:11, Phillip Wood wrote:\n> From: Phillip Wood \n> \n[snip]\n> diff --git a/compat/terminal.h b/compat/terminal.h\n> index f24b91390d..f5655d0d0b 100644\n> --- a/compat/terminal.h\n> +++ b/compat/terminal.h\n> @@ -3,7 +3,8 @@\n> \n> /* Save input and output settings */\n> #define SAVE_TERM_DUPLEX (1u << 0)\n> -\n> +/* Save stdin rather than /dev/tty (fails is stdin is not a terminal) */\n\ns/fails is/fails if/ ?\n\nATB,\nRamsay Jones\n\n> +#define SAVE_TERM_STDIN (1u << 1)\n> /*\n> * Save the terminal attributes so they can be restored later by a\n> * call to restore_term(). Note that every successful call to\n"},{"id":"450439","messageId":"95cbca4c-ec26-853e-243f-461a365d9f71@ramsayjones.plus.com","threadId":"57509","inReplyTo":"20220304131126.8293-2-phillip.wood123@gmail.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2022-03-04T20:40:36Z","receivedAt":"2022-03-04T20:43:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Phillip,\n\nI have not studied/applied your patches, they are just floating\npast my inbox, so please ignore me if I have misunderstood ...\n\nOn 04/03/2022 13:11, Phillip Wood wrote:\n> From: Phillip Wood \n> \n> The next commit will add another flag in addition to the existing\n> full_duplex so change the function signature to take an unsigned flags\n> argument. Also alter the functions that call save_term() so that they\n> can pass flags down to it.\n> \n> Signed-off-by: Phillip Wood \n> ---\n> compat/terminal.c | 41 +++++++++++++++++++++--------------------\n> compat/terminal.h | 5 ++++-\n> 2 files changed, 25 insertions(+), 21 deletions(-)\n> \n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index d882dfa06e..bad8e04cd8 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -34,7 +34,7 @@ void restore_term(void)\n> \tsigchain_pop_common();\n> }\n> \n> -int save_term(int full_duplex)\n> +int save_term(unsigned flags)\n> {\n> \tif (term_fd < 0)\n> \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n> @@ -47,11 +47,11 @@ int save_term(int full_duplex)\n> \treturn 0;\n> }\n> \n> -static int disable_bits(tcflag_t bits)\n> +static int disable_bits(unsigned flags, tcflag_t bits)\n\n.. you add the 'flags' as the new first parameter ...\n\n> {\n> \tstruct termios t;\n> \n> -\tif (save_term(0) < 0)\n> +\tif (save_term(flags) < 0)\n> \t\tgoto error;\n> \n> \tt = old_term;\n> @@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)\n> \treturn -1;\n> }\n> \n> -static int disable_echo(void)\n> +static int disable_echo(unsigned flags)\n> {\n> -\treturn disable_bits(ECHO);\n> +\treturn disable_bits(flags, ECHO);\n\n.. and pass it as first parameter, good, and ...\n\n> }\n> \n> -static int enable_non_canonical(void)\n> +static int enable_non_canonical(unsigned flags)\n> {\n> -\treturn disable_bits(ICANON | ECHO);\n> +\treturn disable_bits(flags, ICANON | ECHO);\n\n.. here as well, good, and ...\n\n> }\n> \n> #elif defined(GIT_WINDOWS_NATIVE)\n> @@ -126,15 +126,15 @@ void restore_term(void)\n> \thconin = hconout = INVALID_HANDLE_VALUE;\n> }\n> \n> -int save_term(int full_duplex)\n> +int save_term(unsigned flags)\n> {\n> \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n> \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n> \t FILE_ATTRIBUTE_NORMAL, NULL);\n> \tif (hconin == INVALID_HANDLE_VALUE)\n> \t\treturn -1;\n> \n> -\tif (full_duplex) {\n> +\tif (flags & SAVE_TERM_DUPLEX) {\n> \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n> \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n> \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n> @@ -154,7 +154,7 @@ int save_term(int full_duplex)\n> \treturn -1;\n> }\n> \n> -static int disable_bits(DWORD bits)\n> +static int disable_bits(unsigned flags, DWORD bits)\n\n.. Huh? Ah, the DWORD suggests this is in an #ifdef'd windows\npart of the file, OK. ...\n\n> {\n> \tif (use_stty) {\n> \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> @@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)\n> \t\tuse_stty = 0;\n> \t}\n> \n> -\tif (save_term(0) < 0)\n> +\tif (save_term(flags) < 0)\n> \t\treturn -1;\n> \n> \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n> @@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)\n> \treturn 0;\n> }\n> \n> -static int disable_echo(void)\n> +static int disable_echo(unsigned flags)\n> {\n> -\treturn disable_bits(ENABLE_ECHO_INPUT);\n> +\treturn disable_bits(ENABLE_ECHO_INPUT, flags);\n\n.. but here, you pass the flags as the second parameter. ;-)\n\n> }\n> \n> -static int enable_non_canonical(void)\n> +static int enable_non_canonical(unsigned flags)\n> {\n> -\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n> +\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,\n> +\t\t\t flags);\n\n.. ditto here.\n\nATB,\nRamsay Jones\n\n\n> }\n> \n> /*\n> @@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n> \t\treturn NULL;\n> \t}\n> \n> -\tif (!echo && disable_echo()) {\n> +\tif (!echo && disable_echo(0)) {\n> \t\tfclose(input_fh);\n> \t\tfclose(output_fh);\n> \t\treturn NULL;\n> @@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)\n> \tstatic int warning_displayed;\n> \tint ch;\n> \n> -\tif (warning_displayed || enable_non_canonical() < 0) {\n> +\tif (warning_displayed || enable_non_canonical(0) < 0) {\n> \t\tif (!warning_displayed) {\n> \t\t\twarning(\"reading single keystrokes not supported on \"\n> \t\t\t\t\"this platform; reading line instead\");\n> @@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)\n> \n> #else\n> \n> -int save_term(int full_duplex)\n> +int save_term(unsigned flags)\n> {\n> -\t/* full_duplex == 1, but no support available */\n> -\treturn -full_duplex;\n> +\t/* no duplex support available */\n> +\treturn -!!(flags & SAVE_TERM_DUPLEX);\n> }\n> \n> void restore_term(void)\n> diff --git a/compat/terminal.h b/compat/terminal.h\n> index 0fb9fa147c..f24b91390d 100644\n> --- a/compat/terminal.h\n> +++ b/compat/terminal.h\n> @@ -1,14 +1,17 @@\n> #ifndef COMPAT_TERMINAL_H\n> #define COMPAT_TERMINAL_H\n> \n> +/* Save input and output settings */\n> +#define SAVE_TERM_DUPLEX (1u << 0)\n> +\n> /*\n> * Save the terminal attributes so they can be restored later by a\n> * call to restore_term(). Note that every successful call to\n> * save_term() must be matched by a call to restore_term() even if the\n> * attributes have not been changed. Returns 0 on success, -1 on\n> * failure.\n> */\n> -int save_term(int full_duplex);\n> +int save_term(unsigned flags);\n> /* Restore the terminal attributes that were saved with save_term() */\n> void restore_term(void);\n> \n"},{"id":"450491","messageId":"220305.86bkyk4hwc.gmgdl@evledraar.gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-5-phillip.wood123@gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-05T13:59:33Z","receivedAt":"2022-03-05T14:02:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 04 2022, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> If the user suspends git while it is waiting for a keypress reset the\n> terminal before stopping and restore the settings when git resumes. If\n> the user tries to resume in the background print an error\n> message (taking care to use async safe functions) before stopping\n> again. Ideally we would reprint the prompt for the user when git\n> resumes but this patch just restarts the read().\n>\n> The signal handler is established with sigaction() rather than using\n> sigchain_push() as this allows us to control the signal mask when the\n> handler is invoked and ensure SA_RESTART is used to restart the\n> read() when resuming.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> compat/terminal.c | 124 ++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 120 insertions(+), 4 deletions(-)\n>\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 5d516ff546..79ab54c2f8 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -1,4 +1,4 @@\n> -#include \"git-compat-util.h\"\n> +#include \"cache.h\"\n> #include \"compat/terminal.h\"\n> #include \"sigchain.h\"\n> #include \"strbuf.h\"\n> @@ -23,6 +23,89 @@ static void restore_term_on_signal(int sig)\n> static int term_fd = -1;\n> static struct termios old_term;\n> \n> +static char *background_resume_msg;\n> +static char *restore_error_msg;\n> +static volatile sig_atomic_t ttou_received;\n> +\n> +static void write_msg(const char *msg)\n> +{\n> +\twrite_in_full(2, msg, strlen(msg));\n> +\twrite_in_full(2, \"\\n\", 1);\n> +}\n> +\n> +static void print_background_resume_msg(int signo)\n> +{\n> +\tint saved_errno = errno;\n> +\tsigset_t mask;\n> +\tstruct sigaction old_sa;\n> +\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n> +\n> +\tttou_received = 1;\n> +\twrite_msg(background_resume_msg);\n> +\tsigaction(signo, &sa, &old_sa);\n> +\traise(signo);\n> +\tsigemptyset(&mask);\n> +\tsigaddset(&mask, signo);\n> +\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n> +\t/* Stopped here */\n> +\tsigprocmask(SIG_BLOCK, &mask, NULL);\n> +\tsigaction(signo, &old_sa, NULL);\n> +\terrno = saved_errno;\n> +}\n> +\n> +static void restore_terminal_on_suspend(int signo)\n> +{\n> +\tint saved_errno = errno;\n> +\tint res;\n> +\tstruct termios t;\n> +\tsigset_t mask;\n> +\tstruct sigaction old_sa;\n> +\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n> +\tint can_restore = 1;\n> +\n> +\tif (tcgetattr(term_fd, &t) < 0)\n> +\t\tcan_restore = 0;\n> +\n> +\tif (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)\n> +\t\twrite_msg(restore_error_msg);\n> +\n> +\tsigaction(signo, &sa, &old_sa);\n> +\traise(signo);\n> +\tsigemptyset(&mask);\n> +\tsigaddset(&mask, signo);\n> +\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n> +\t/* Stopped here */\n> +\tsigprocmask(SIG_BLOCK, &mask, NULL);\n> +\tsigaction(signo, &old_sa, NULL);\n> +\tif (!can_restore) {\n> +\t\twrite_msg(restore_error_msg);\n> +\t\tgoto out;\n> +\t}\n> +\t/*\n> +\t * If we resume in the background then we receive SIGTTOU when calling\n> +\t * tcsetattr() below. Set up a handler to print an error message in that\n> +\t * case.\n> +\t */\n> +\tsigemptyset(&mask);\n> +\tsigaddset(&mask, SIGTTOU);\n> +\tsa.sa_mask = old_sa.sa_mask;\n> +\tsa.sa_handler = print_background_resume_msg;\n> +\tsa.sa_flags = SA_RESTART;\n> +\tsigaction(SIGTTOU, &sa, &old_sa);\n> + again:\n> +\tttou_received = 0;\n> +\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n> +\tres = tcsetattr(term_fd, TCSAFLUSH, &t);\n> +\tsigprocmask(SIG_BLOCK, &mask, NULL);\n> +\tif (ttou_received)\n> +\t\tgoto again;\n> +\telse if (res < 0)\n> +\t\twrite_msg(restore_error_msg);\n> +\tsigaction(SIGTTOU, &old_sa, NULL);\n> + out:\n> +\terrno = saved_errno;\n> +}\n> +\n> void restore_term(void)\n> {\n> \tif (term_fd < 0)\n> @@ -32,10 +115,19 @@ void restore_term(void)\n> \tclose(term_fd);\n> \tterm_fd = -1;\n> \tsigchain_pop_common();\n> +\tif (restore_error_msg) {\n> +\t\tsignal(SIGTTIN, SIG_DFL);\n> +\t\tsignal(SIGTTOU, SIG_DFL);\n> +\t\tsignal(SIGTSTP, SIG_DFL);\n> +\t\tFREE_AND_NULL(restore_error_msg);\n> +\t\tFREE_AND_NULL(background_resume_msg);\n> +\t}\n> }\n> \n> int save_term(unsigned flags)\n> {\n> +\tstruct sigaction sa;\n> +\n> \tif (term_fd < 0)\n> \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n> \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n> @@ -44,6 +136,26 @@ int save_term(unsigned flags)\n> \tif (tcgetattr(term_fd, &old_term) < 0)\n> \t\treturn -1;\n> \tsigchain_push_common(restore_term_on_signal);\n> +\t/*\n> +\t * If job control is disabled then the shell will have set the\n> +\t * disposition of SIGTSTP to SIG_IGN.\n> +\t */\n> +\tsigaction(SIGTSTP, NULL, &sa);\n> +\tif (sa.sa_handler == SIG_IGN)\n> +\t\treturn 0;\n> +\n> +\t/* avoid calling gettext() from signal handler */\n> +\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n> +\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n\nYou don't need to xstrdup() the return values of gettext() (here _()),\nyou'll get a pointer to static storage that's safe to hold on to for the\nduration of the program.\n\nIn this case I think it would make sense to skip \"error: \" from the\nmessage itself.\n\nEventually we'll get to making usage.c have that prefix translated, and\ncan have some utility function exposed there (I have WIP patches for\nthis already since a while ago).\n\nTo translators it'll look like the same thing, and avoid churn when we\nmake the \"error: \" prefix translatable.\n\nAside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above\nadvice...) doesn't it make sense to add the \"\\n\" here, so you'll have\none write_in_full() above?\n"},{"id":"450492","messageId":"220305.867d984hsk.gmgdl@evledraar.gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-2-phillip.wood123@gmail.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-05T14:02:56Z","receivedAt":"2022-03-05T14:04:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 04 2022, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> The next commit will add another flag in addition to the existing\n> full_duplex so change the function signature to take an unsigned flags\n> argument. Also alter the functions that call save_term() so that they\n> can pass flags down to it.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> compat/terminal.c | 41 +++++++++++++++++++++--------------------\n> compat/terminal.h | 5 ++++-\n> 2 files changed, 25 insertions(+), 21 deletions(-)\n>\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index d882dfa06e..bad8e04cd8 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -34,7 +34,7 @@ void restore_term(void)\n> \tsigchain_pop_common();\n> }\n> \n> -int save_term(int full_duplex)\n> +int save_term(unsigned flags)\n\nDoing e.g. ...\n\n> void restore_term(void)\n> diff --git a/compat/terminal.h b/compat/terminal.h\n> index 0fb9fa147c..f24b91390d 100644\n> --- a/compat/terminal.h\n> +++ b/compat/terminal.h\n> @@ -1,14 +1,17 @@\n> #ifndef COMPAT_TERMINAL_H\n> #define COMPAT_TERMINAL_H\n> \n> +/* Save input and output settings */\n> +#define SAVE_TERM_DUPLEX (1u << 0)\n\t\n\tenum save_terminal_flags {\n\t\tSAVE_TERMINAL_FLAGS = 1 << 0,\n\t};\n\t\nHere would be better IMO. See 3f9ab7ccdea (parse-options.[ch]:\nconsistently use \"enum parse_opt_flags\", 2021-10-08) for how it makes\ndebugging better.\n"},{"id":"450519","messageId":"1351bb93-673e-817f-6911-d321217032ce@gmail.com","threadId":"57509","inReplyTo":"220305.867d984hsk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-07T10:45:04Z","receivedAt":"2022-03-07T11:19:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/03/2022 14:02, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Mar 04 2022, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> The next commit will add another flag in addition to the existing\n>> full_duplex so change the function signature to take an unsigned flags\n>> argument. Also alter the functions that call save_term() so that they\n>> can pass flags down to it.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>> compat/terminal.c | 41 +++++++++++++++++++++--------------------\n>> compat/terminal.h | 5 ++++-\n>> 2 files changed, 25 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/compat/terminal.c b/compat/terminal.c\n>> index d882dfa06e..bad8e04cd8 100644\n>> --- a/compat/terminal.c\n>> +++ b/compat/terminal.c\n>> @@ -34,7 +34,7 @@ void restore_term(void)\n>> \tsigchain_pop_common();\n>> }\n>> \n>> -int save_term(int full_duplex)\n>> +int save_term(unsigned flags)\n> \n> Doing e.g. ...\n> \n>> void restore_term(void)\n>> diff --git a/compat/terminal.h b/compat/terminal.h\n>> index 0fb9fa147c..f24b91390d 100644\n>> --- a/compat/terminal.h\n>> +++ b/compat/terminal.h\n>> @@ -1,14 +1,17 @@\n>> #ifndef COMPAT_TERMINAL_H\n>> #define COMPAT_TERMINAL_H\n>> \n>> +/* Save input and output settings */\n>> +#define SAVE_TERM_DUPLEX (1u << 0)\n> \t\n> \tenum save_terminal_flags {\n> \t\tSAVE_TERMINAL_FLAGS = 1 << 0,\n> \t};\n> \t\n> Here would be better IMO. See 3f9ab7ccdea (parse-options.[ch]:\n> consistently use \"enum parse_opt_flags\", 2021-10-08) for how it makes\n> debugging better.\n\nI'd remembered Junio objecting to enums for bit flags in the discussion \nof that patch but looking at the whole thread it seems like the debugger \nsupport lead him to change his mind. I'll update.\n\nBest Wishes\n\nPhillip\n"},{"id":"450521","messageId":"8aa11144-c9ce-46aa-2edd-15e8fa1298dc@gmail.com","threadId":"57509","inReplyTo":"220305.86bkyk4hwc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-07T10:53:34Z","receivedAt":"2022-03-07T11:24:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:\n> [...] \n>> int save_term(unsigned flags)\n>> {\n>> +\tstruct sigaction sa;\n>> +\n>> \tif (term_fd < 0)\n>> \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n>> \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n>> @@ -44,6 +136,26 @@ int save_term(unsigned flags)\n>> \tif (tcgetattr(term_fd, &old_term) < 0)\n>> \t\treturn -1;\n>> \tsigchain_push_common(restore_term_on_signal);\n>> +\t/*\n>> +\t * If job control is disabled then the shell will have set the\n>> +\t * disposition of SIGTSTP to SIG_IGN.\n>> +\t */\n>> +\tsigaction(SIGTSTP, NULL, &sa);\n>> +\tif (sa.sa_handler == SIG_IGN)\n>> +\t\treturn 0;\n>> +\n>> +\t/* avoid calling gettext() from signal handler */\n>> +\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n>> +\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n> \n> You don't need to xstrdup() the return values of gettext() (here _()),\n> you'll get a pointer to static storage that's safe to hold on to for the\n> duration of the program.\n\nI had a look at the documentation and could not see anything about the \nlifetime of the returned string, all it says is \"don't alter it\"\n\n> In this case I think it would make sense to skip \"error: \" from the\n> message itself.\n> \n> Eventually we'll get to making usage.c have that prefix translated, and\n> can have some utility function exposed there (I have WIP patches for\n> this already since a while ago).\n> \n> To translators it'll look like the same thing, and avoid churn when we\n> make the \"error: \" prefix translatable.\n\nUnless we add a function that returns a string rather than printing the \nmessage I don't see how it avoids churn in the future. Having the whole \nstring with the \"error: \" prefix translated here does not add any extra \nburden to translators - it is still the same number of strings to translate.\n\n> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above\n> advice...) doesn't it make sense to add the \"\\n\" here, so you'll have\n> one write_in_full() above?\n\nI decided to keep the translated string simpler by omitting the newline, \ncalling write_in_full() twice isn't a bit deal (I don't think the output \ncan be split by a write from another thread or signal handler in between).\n\nBest Wishes\n\nPhillip\n\n"},{"id":"450523","messageId":"a2d91895-d933-9909-2f1b-5addd38974f3@gmail.com","threadId":"57509","inReplyTo":"95cbca4c-ec26-853e-243f-461a365d9f71@ramsayjones.plus.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-07T11:11:14Z","receivedAt":"2022-03-07T11:28:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ramsay\n\nOn 04/03/2022 20:40, Ramsay Jones wrote:\n> Hi Phillip,\n> \n> I have not studied/applied your patches, they are just floating\n> past my inbox, so please ignore me if I have misunderstood ...\n> \n> On 04/03/2022 13:11, Phillip Wood wrote:\n>> From: Phillip Wood \n>>\n>> The next commit will add another flag in addition to the existing\n>> full_duplex so change the function signature to take an unsigned flags\n>> argument. Also alter the functions that call save_term() so that they\n>> can pass flags down to it.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>> compat/terminal.c | 41 +++++++++++++++++++++--------------------\n>> compat/terminal.h | 5 ++++-\n>> 2 files changed, 25 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/compat/terminal.c b/compat/terminal.c\n>> index d882dfa06e..bad8e04cd8 100644\n>> --- a/compat/terminal.c\n>> +++ b/compat/terminal.c\n>> @@ -34,7 +34,7 @@ void restore_term(void)\n>> \tsigchain_pop_common();\n>> }\n>> \n>> -int save_term(int full_duplex)\n>> +int save_term(unsigned flags)\n>> {\n>> \tif (term_fd < 0)\n>> \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n>> @@ -47,11 +47,11 @@ int save_term(int full_duplex)\n>> \treturn 0;\n>> }\n>> \n>> -static int disable_bits(tcflag_t bits)\n>> +static int disable_bits(unsigned flags, tcflag_t bits)\n> \n> .. you add the 'flags' as the new first parameter ...\n> \n>> {\n>> \tstruct termios t;\n>> \n>> -\tif (save_term(0) < 0)\n>> +\tif (save_term(flags) < 0)\n>> \t\tgoto error;\n>> \n>> \tt = old_term;\n>> @@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)\n>> \treturn -1;\n>> }\n>> \n>> -static int disable_echo(void)\n>> +static int disable_echo(unsigned flags)\n>> {\n>> -\treturn disable_bits(ECHO);\n>> +\treturn disable_bits(flags, ECHO);\n> \n> .. and pass it as first parameter, good, and ...\n> \n>> }\n>> \n>> -static int enable_non_canonical(void)\n>> +static int enable_non_canonical(unsigned flags)\n>> {\n>> -\treturn disable_bits(ICANON | ECHO);\n>> +\treturn disable_bits(flags, ICANON | ECHO);\n> \n> .. here as well, good, and ...\n> \n>> }\n>> \n>> #elif defined(GIT_WINDOWS_NATIVE)\n>> @@ -126,15 +126,15 @@ void restore_term(void)\n>> \thconin = hconout = INVALID_HANDLE_VALUE;\n>> }\n>> \n>> -int save_term(int full_duplex)\n>> +int save_term(unsigned flags)\n>> {\n>> \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n>> \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n>> \t FILE_ATTRIBUTE_NORMAL, NULL);\n>> \tif (hconin == INVALID_HANDLE_VALUE)\n>> \t\treturn -1;\n>> \n>> -\tif (full_duplex) {\n>> +\tif (flags & SAVE_TERM_DUPLEX) {\n>> \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n>> \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n>> \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n>> @@ -154,7 +154,7 @@ int save_term(int full_duplex)\n>> \treturn -1;\n>> }\n>> \n>> -static int disable_bits(DWORD bits)\n>> +static int disable_bits(unsigned flags, DWORD bits)\n> \n> .. Huh? Ah, the DWORD suggests this is in an #ifdef'd windows\n> part of the file, OK. ...\n> \n>> {\n>> \tif (use_stty) {\n>> \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>> @@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)\n>> \t\tuse_stty = 0;\n>> \t}\n>> \n>> -\tif (save_term(0) < 0)\n>> +\tif (save_term(flags) < 0)\n>> \t\treturn -1;\n>> \n>> \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n>> @@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)\n>> \treturn 0;\n>> }\n>> \n>> -static int disable_echo(void)\n>> +static int disable_echo(unsigned flags)\n>> {\n>> -\treturn disable_bits(ENABLE_ECHO_INPUT);\n>> +\treturn disable_bits(ENABLE_ECHO_INPUT, flags);\n> \n> .. but here, you pass the flags as the second parameter. ;-)\n\nOh dear that's embarrassing, thanks for your careful review.\n\nAre patches 3 & 4 OK for non-stop platforms?\n\nBest Wishes\n\nPhillip\n\n>> }\n>> \n>> -static int enable_non_canonical(void)\n>> +static int enable_non_canonical(unsigned flags)\n>> {\n>> -\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n>> +\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,\n>> +\t\t\t flags);\n> \n> .. ditto here.\n> \n> ATB,\n> Ramsay Jones\n> \n> \n>> }\n>> \n>> /*\n>> @@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n>> \t\treturn NULL;\n>> \t}\n>> \n>> -\tif (!echo && disable_echo()) {\n>> +\tif (!echo && disable_echo(0)) {\n>> \t\tfclose(input_fh);\n>> \t\tfclose(output_fh);\n>> \t\treturn NULL;\n>> @@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)\n>> \tstatic int warning_displayed;\n>> \tint ch;\n>> \n>> -\tif (warning_displayed || enable_non_canonical() < 0) {\n>> +\tif (warning_displayed || enable_non_canonical(0) < 0) {\n>> \t\tif (!warning_displayed) {\n>> \t\t\twarning(\"reading single keystrokes not supported on \"\n>> \t\t\t\t\"this platform; reading line instead\");\n>> @@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)\n>> \n>> #else\n>> \n>> -int save_term(int full_duplex)\n>> +int save_term(unsigned flags)\n>> {\n>> -\t/* full_duplex == 1, but no support available */\n>> -\treturn -full_duplex;\n>> +\t/* no duplex support available */\n>> +\treturn -!!(flags & SAVE_TERM_DUPLEX);\n>> }\n>> \n>> void restore_term(void)\n>> diff --git a/compat/terminal.h b/compat/terminal.h\n>> index 0fb9fa147c..f24b91390d 100644\n>> --- a/compat/terminal.h\n>> +++ b/compat/terminal.h\n>> @@ -1,14 +1,17 @@\n>> #ifndef COMPAT_TERMINAL_H\n>> #define COMPAT_TERMINAL_H\n>> \n>> +/* Save input and output settings */\n>> +#define SAVE_TERM_DUPLEX (1u << 0)\n>> +\n>> /*\n>> * Save the terminal attributes so they can be restored later by a\n>> * call to restore_term(). Note that every successful call to\n>> * save_term() must be matched by a call to restore_term() even if the\n>> * attributes have not been changed. Returns 0 on success, -1 on\n>> * failure.\n>> */\n>> -int save_term(int full_duplex);\n>> +int save_term(unsigned flags);\n>> /* Restore the terminal attributes that were saved with save_term() */\n>> void restore_term(void);\n>> \n\n"},{"id":"450533","messageId":"220307.86ilsq0xxr.gmgdl@evledraar.gmail.com","threadId":"57509","inReplyTo":"8aa11144-c9ce-46aa-2edd-15e8fa1298dc@gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-07T11:49:37Z","receivedAt":"2022-03-07T12:06:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 07 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:\n>> [...] \n>>> int save_term(unsigned flags)\n>>> {\n>>> +\tstruct sigaction sa;\n>>> +\n>>> \tif (term_fd < 0)\n>>> \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n>>> \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n>>> @@ -44,6 +136,26 @@ int save_term(unsigned flags)\n>>> \tif (tcgetattr(term_fd, &old_term) < 0)\n>>> \t\treturn -1;\n>>> \tsigchain_push_common(restore_term_on_signal);\n>>> +\t/*\n>>> +\t * If job control is disabled then the shell will have set the\n>>> +\t * disposition of SIGTSTP to SIG_IGN.\n>>> +\t */\n>>> +\tsigaction(SIGTSTP, NULL, &sa);\n>>> +\tif (sa.sa_handler == SIG_IGN)\n>>> +\t\treturn 0;\n>>> +\n>>> +\t/* avoid calling gettext() from signal handler */\n>>> +\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n>>> +\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n>> You don't need to xstrdup() the return values of gettext() (here\n>> _()),\n>> you'll get a pointer to static storage that's safe to hold on to for the\n>> duration of the program.\n>\n> I had a look at the documentation and could not see anything about the\n> lifetime of the returned string, all it says is \"don't alter it\"\n\nI think this is overed in \"11.2.7 Optimization of the *gettext\nfunctions\", a pedantic reading might suggest not, but what's meant with\nthe combination of that API documentation & the description of how MO\nfiles work is that you're just getting pointers into the already-loaded\ntranslation catalog, so it's safe to hold onto the pointer and re-use it\nlater.\n\nIn any case, if we're going to be paranoid about gettext() it would make\nsense to propose that as some general change to how we use it, we rely\non this assumption holding in a lot of our use of the API:\n\n git grep '= _\\('\n\nRather than sneak that partcular new assumption in here in this already\ntricky code...\n\n>> In this case I think it would make sense to skip \"error: \" from the\n>> message itself.\n>> Eventually we'll get to making usage.c have that prefix translated,\n>> and\n>> can have some utility function exposed there (I have WIP patches for\n>> this already since a while ago).\n>> To translators it'll look like the same thing, and avoid churn when\n>> we\n>> make the \"error: \" prefix translatable.\n>\n> Unless we add a function that returns a string rather than printing\n> the message I don't see how it avoids churn in the future. Having the\n> whole string with the \"error: \" prefix translated here does not add\n> any extra burden to translators - it is still the same number of\n> strings to translate.\n\nBecause translators translate \"we failed\" for most errors, not \"error:\nwe failed\".\n\nIf and when we convert it from \"error: we failed\" to \"we failed\" they'll\nneed to translate it again (although to be fair, the translation cache\nwill help).\n\nAnd even then it'll be one of very few exceptions where the \"error: \"\ncurrently that *is* translated.\n\n>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above\n>> advice...) doesn't it make sense to add the \"\\n\" here, so you'll have\n>> one write_in_full() above?\n>\n> I decided to keep the translated string simpler by omitting the\n> newline, calling write_in_full() twice isn't a bit deal (I don't think\n> the output can be split by a write from another thread or signal\n> handler in between).\n\nMakes sense.\n\nFWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you\ncould do:\n\n xstrfmt(\"error: %s\\n\", _(\"the error\"))\n\nAnd then do one call to write_in_full().\n\nBut I think just:\n\n msg = _(\"the error\");\n\nAnd then:\n\n\tconst char *const = pfx = \"error: \";\n const size_t len = strlen(pfx);\n\n\twrite_in_full(2, pfx, len);\n write_in_full(2, msg, strlen(msg));\n\twrite_in_full(2, \"\\n\", 1);\n\nMakes more sense :)\n"},{"id":"450534","messageId":"220307.86ee3e0xv6.gmgdl@evledraar.gmail.com","threadId":"57509","inReplyTo":"1351bb93-673e-817f-6911-d321217032ce@gmail.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-07T12:06:54Z","receivedAt":"2022-03-07T12:07:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 07 2022, Phillip Wood wrote:\n\n> On 05/03/2022 14:02, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Mar 04 2022, Phillip Wood wrote:\n>> \n>>> From: Phillip Wood \n>>>\n>>> The next commit will add another flag in addition to the existing\n>>> full_duplex so change the function signature to take an unsigned flags\n>>> argument. Also alter the functions that call save_term() so that they\n>>> can pass flags down to it.\n>>>\n>>> Signed-off-by: Phillip Wood \n>>> ---\n>>> compat/terminal.c | 41 +++++++++++++++++++++--------------------\n>>> compat/terminal.h | 5 ++++-\n>>> 2 files changed, 25 insertions(+), 21 deletions(-)\n>>>\n>>> diff --git a/compat/terminal.c b/compat/terminal.c\n>>> index d882dfa06e..bad8e04cd8 100644\n>>> --- a/compat/terminal.c\n>>> +++ b/compat/terminal.c\n>>> @@ -34,7 +34,7 @@ void restore_term(void)\n>>> \tsigchain_pop_common();\n>>> }\n>>> -int save_term(int full_duplex)\n>>> +int save_term(unsigned flags)\n>> Doing e.g. ...\n>> \n>>> void restore_term(void)\n>>> diff --git a/compat/terminal.h b/compat/terminal.h\n>>> index 0fb9fa147c..f24b91390d 100644\n>>> --- a/compat/terminal.h\n>>> +++ b/compat/terminal.h\n>>> @@ -1,14 +1,17 @@\n>>> #ifndef COMPAT_TERMINAL_H\n>>> #define COMPAT_TERMINAL_H\n>>> +/* Save input and output settings */\n>>> +#define SAVE_TERM_DUPLEX (1u << 0)\n>> \t\n>> \tenum save_terminal_flags {\n>> \t\tSAVE_TERMINAL_FLAGS = 1 << 0,\n>> \t};\n>> \t\n>> Here would be better IMO. See 3f9ab7ccdea (parse-options.[ch]:\n>> consistently use \"enum parse_opt_flags\", 2021-10-08) for how it makes\n>> debugging better.\n>\n> I'd remembered Junio objecting to enums for bit flags in the\n> discussion of that patch but looking at the whole thread it seems like\n> the debugger support lead him to change his mind. I'll update.\n\nYeah, aside from that I think part of that was whether it was worth it\nto refactor it for existing code, but since this is new code & we tend\nto use that pattern liberally (which pre-dates any recent changes I did\nby quite a bit...), ....\n"},{"id":"450574","messageId":"a02dea22-7403-7302-cf20-9aff75a10d14@gmail.com","threadId":"57509","inReplyTo":"220307.86ilsq0xxr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-07T13:49:51Z","receivedAt":"2022-03-07T13:50:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Mar 07 2022, Phillip Wood wrote:\n> \n>> Hi Ævar\n>>\n>> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:\n>>> [...]\n>>>> int save_term(unsigned flags)\n>>>> {\n>>>> +\tstruct sigaction sa;\n>>>> +\n>>>> \tif (term_fd < 0)\n>>>> \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n>>>> \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n>>>> @@ -44,6 +136,26 @@ int save_term(unsigned flags)\n>>>> \tif (tcgetattr(term_fd, &old_term) < 0)\n>>>> \t\treturn -1;\n>>>> \tsigchain_push_common(restore_term_on_signal);\n>>>> +\t/*\n>>>> +\t * If job control is disabled then the shell will have set the\n>>>> +\t * disposition of SIGTSTP to SIG_IGN.\n>>>> +\t */\n>>>> +\tsigaction(SIGTSTP, NULL, &sa);\n>>>> +\tif (sa.sa_handler == SIG_IGN)\n>>>> +\t\treturn 0;\n>>>> +\n>>>> +\t/* avoid calling gettext() from signal handler */\n>>>> +\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n>>>> +\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n>>> You don't need to xstrdup() the return values of gettext() (here\n>>> _()),\n>>> you'll get a pointer to static storage that's safe to hold on to for the\n>>> duration of the program.\n>>\n>> I had a look at the documentation and could not see anything about the\n>> lifetime of the returned string, all it says is \"don't alter it\"\n> \n> I think this is overed in \"11.2.7 Optimization of the *gettext\n> functions\", a pedantic reading might suggest not, but what's meant with\n> the combination of that API documentation & the description of how MO\n> files work is that you're just getting pointers into the already-loaded\n> translation catalog, so it's safe to hold onto the pointer and re-use it\n> later.\n\nStrictly that section only shows it is safe if there are no other calls \nto gettext() before the returned string is used. I agree the \nimplementation is likely to be just returning static strings but I can't \nfind anywhere that says another implementation (e.g. on macos/*bsd) has \nto do that.\n\n> In any case, if we're going to be paranoid about gettext() it would make\n> sense to propose that as some general change to how we use it, we rely\n> on this assumption holding in a lot of our use of the API:\n> \n> git grep '= _\\('\n> \n> Rather than sneak that partcular new assumption in here in this already\n> tricky code...\n\nThe ones I looked at are mostly not calling gettext() again before using \nthe translated string (there is one exception in builtin/remote.c).\n\nIn restore_term() I'm checking if the messages are NULL to see if job \ncontrol is enabled, I could use a flag but I'm inclined to just keep \ncoping the strings.\n\n> \n>>> In this case I think it would make sense to skip \"error: \" from the\n>>> message itself.\n>>> Eventually we'll get to making usage.c have that prefix translated,\n>>> and\n>>> can have some utility function exposed there (I have WIP patches for\n>>> this already since a while ago).\n>>> To translators it'll look like the same thing, and avoid churn when\n>>> we\n>>> make the \"error: \" prefix translatable.\n>>\n>> Unless we add a function that returns a string rather than printing\n>> the message I don't see how it avoids churn in the future. Having the\n>> whole string with the \"error: \" prefix translated here does not add\n>> any extra burden to translators - it is still the same number of\n>> strings to translate.\n> \n> Because translators translate \"we failed\" for most errors, not \"error:\n> we failed\".\n> \n> If and when we convert it from \"error: we failed\" to \"we failed\" they'll\n> need to translate it again (although to be fair, the translation cache\n> will help).\n> \n> And even then it'll be one of very few exceptions where the \"error: \"\n> currently that *is* translated.\n> \n>>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above\n>>> advice...) doesn't it make sense to add the \"\\n\" here, so you'll have\n>>> one write_in_full() above?\n>>\n>> I decided to keep the translated string simpler by omitting the\n>> newline, calling write_in_full() twice isn't a bit deal (I don't think\n>> the output can be split by a write from another thread or signal\n>> handler in between).\n> \n> Makes sense.\n> \n> FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you\n> could do:\n> \n> xstrfmt(\"error: %s\\n\", _(\"the error\"))\n> \n> And then do one call to write_in_full().\n> \n> But I think just:\n> \n> msg = _(\"the error\");\n> \n> And then:\n> \n> \tconst char *const = pfx = \"error: \";\n> const size_t len = strlen(pfx);\n> \n> \twrite_in_full(2, pfx, len);\n> write_in_full(2, msg, strlen(msg));\n> \twrite_in_full(2, \"\\n\", 1);\n> \n> Makes more sense :)\n\nAgreed, I'll change that.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"450582","messageId":"220307.86o82hzucl.gmgdl@evledraar.gmail.com","threadId":"57509","inReplyTo":"a02dea22-7403-7302-cf20-9aff75a10d14@gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-07T14:45:51Z","receivedAt":"2022-03-07T14:54:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 07 2022, Phillip Wood wrote:\n\n> On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Mar 07 2022, Phillip Wood wrote:\n>> \n>>> Hi Ævar\n>>>\n>>> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:\n>>>> [...]\n>>>>> int save_term(unsigned flags)\n>>>>> {\n>>>>> +\tstruct sigaction sa;\n>>>>> +\n>>>>> \tif (term_fd < 0)\n>>>>> \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n>>>>> \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n>>>>> @@ -44,6 +136,26 @@ int save_term(unsigned flags)\n>>>>> \tif (tcgetattr(term_fd, &old_term) < 0)\n>>>>> \t\treturn -1;\n>>>>> \tsigchain_push_common(restore_term_on_signal);\n>>>>> +\t/*\n>>>>> +\t * If job control is disabled then the shell will have set the\n>>>>> +\t * disposition of SIGTSTP to SIG_IGN.\n>>>>> +\t */\n>>>>> +\tsigaction(SIGTSTP, NULL, &sa);\n>>>>> +\tif (sa.sa_handler == SIG_IGN)\n>>>>> +\t\treturn 0;\n>>>>> +\n>>>>> +\t/* avoid calling gettext() from signal handler */\n>>>>> +\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n>>>>> +\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n>>>> You don't need to xstrdup() the return values of gettext() (here\n>>>> _()),\n>>>> you'll get a pointer to static storage that's safe to hold on to for the\n>>>> duration of the program.\n>>>\n>>> I had a look at the documentation and could not see anything about the\n>>> lifetime of the returned string, all it says is \"don't alter it\"\n>> I think this is overed in \"11.2.7 Optimization of the *gettext\n>> functions\", a pedantic reading might suggest not, but what's meant with\n>> the combination of that API documentation & the description of how MO\n>> files work is that you're just getting pointers into the already-loaded\n>> translation catalog, so it's safe to hold onto the pointer and re-use it\n>> later.\n>\n> Strictly that section only shows it is safe if there are no other\n> calls to gettext() before the returned string is used. I agree the \n> implementation is likely to be just returning static strings but I\n> can't find anywhere that says another implementation (e.g. on\n> macos/*bsd) has to do that.\n\nI agree. I'm 99.99% sure this is safe & portable use of the API, but I'm\nhaving some trouble finding documentation for that...\n\n>> In any case, if we're going to be paranoid about gettext() it would make\n>> sense to propose that as some general change to how we use it, we rely\n>> on this assumption holding in a lot of our use of the API:\n>> git grep '= _\\('\n>> Rather than sneak that partcular new assumption in here in this\n>> already\n>> tricky code...\n>\n> The ones I looked at are mostly not calling gettext() again before\n> using the translated string (there is one exception in\n> builtin/remote.c).\n\nDoesn't validate_encoding() in convert.c, process_entry() in\nmerge-ort.c, setup_unpack_trees_porcelain() in unpack-trees.c cmd_mv()\nin builtin/mv.c etc. qualify?\n\nI.e. for a hypothetical gettext() that always returned the same pointer\nand just overwrote it with the latest message those would all emit bad\noutput, wouldn't they?\n\n> In restore_term() I'm checking if the messages are NULL to see if job\n> control is enabled, I could use a flag but I'm inclined to just keep \n> coping the strings.\n\nChecking if they're NULL is orthagonal to whether we xstrdup()\nthem. I.e. you'd just skip the xstrdup() and replace the FREE_AND_NULL\nwith a \"= NULL\" assignment, no?\n\nAnyway, *if* I'm right that it's not new general paranoia with how\ngettext() is used I still think splitting up that part of the change\nwould make sense, just for future readers etc. who'd wonder why it is\nthat this already tricky signal handling etc. code needs that particular\nbit of special behavior.\n\n>>>> In this case I think it would make sense to skip \"error: \" from the\n>>>> message itself.\n>>>> Eventually we'll get to making usage.c have that prefix translated,\n>>>> and\n>>>> can have some utility function exposed there (I have WIP patches for\n>>>> this already since a while ago).\n>>>> To translators it'll look like the same thing, and avoid churn when\n>>>> we\n>>>> make the \"error: \" prefix translatable.\n>>>\n>>> Unless we add a function that returns a string rather than printing\n>>> the message I don't see how it avoids churn in the future. Having the\n>>> whole string with the \"error: \" prefix translated here does not add\n>>> any extra burden to translators - it is still the same number of\n>>> strings to translate.\n>> Because translators translate \"we failed\" for most errors, not\n>> \"error:\n>> we failed\".\n>> If and when we convert it from \"error: we failed\" to \"we failed\"\n>> they'll\n>> need to translate it again (although to be fair, the translation cache\n>> will help).\n>> And even then it'll be one of very few exceptions where the \"error:\n>> \"\n>> currently that *is* translated.\n>> \n>>>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above\n>>>> advice...) doesn't it make sense to add the \"\\n\" here, so you'll have\n>>>> one write_in_full() above?\n>>>\n>>> I decided to keep the translated string simpler by omitting the\n>>> newline, calling write_in_full() twice isn't a bit deal (I don't think\n>>> the output can be split by a write from another thread or signal\n>>> handler in between).\n>> Makes sense.\n>> FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you\n>> could do:\n>> xstrfmt(\"error: %s\\n\", _(\"the error\"))\n>> And then do one call to write_in_full().\n>> But I think just:\n>> msg = _(\"the error\");\n>> And then:\n>> \tconst char *const = pfx = \"error: \";\n>> const size_t len = strlen(pfx);\n>> \twrite_in_full(2, pfx, len);\n>> write_in_full(2, msg, strlen(msg));\n>> \twrite_in_full(2, \"\\n\", 1);\n>> Makes more sense :)\n>\n> Agreed, I'll change that.\n>\n> Best Wishes\n>\n> Phillip\n\n"},{"id":"450643","messageId":"e16ee84f-4fa5-6cc8-c96d-3b728e3c8b2a@ramsayjones.plus.com","threadId":"57509","inReplyTo":"a2d91895-d933-9909-2f1b-5addd38974f3@gmail.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2022-03-07T20:21:14Z","receivedAt":"2022-03-07T20:24:34Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Phillip,\n\nOn 07/03/2022 11:11, Phillip Wood wrote:\n> Hi Ramsay\n[snip]\n>> .. but here, you pass the flags as the second parameter. ;-)\n> \n> Oh dear that's embarrassing, thanks for your careful review.\n> \n> Are patches 3 & 4 OK for non-stop platforms?\n\nErr... I didn't notice any problems with patches 3 & 4, but, as\nfar as the non-stop platform is concerned, I wouldn't have a clue! ;-)\n\n(Perhaps you were thinking of Randall?)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"450720","messageId":"dbd48cc5-1f2a-12d7-e3aa-d5f885ef9ad1@gmail.com","threadId":"57509","inReplyTo":"e16ee84f-4fa5-6cc8-c96d-3b728e3c8b2a@ramsayjones.plus.com","subject":"Re: [PATCH 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-08T10:41:09Z","receivedAt":"2022-03-08T10:41:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ramsay\n\nOn 07/03/2022 20:21, Ramsay Jones wrote:\n> Hi Phillip,\n> \n> On 07/03/2022 11:11, Phillip Wood wrote:\n>> Hi Ramsay\n> [snip]\n>>> .. but here, you pass the flags as the second parameter. ;-)\n>>\n>> Oh dear that's embarrassing, thanks for your careful review.\n>>\n>> Are patches 3 & 4 OK for non-stop platforms?\n> \n> Err... I didn't notice any problems with patches 3 & 4, but, as\n> far as the non-stop platform is concerned, I wouldn't have a clue! ;-)\n> \n> (Perhaps you were thinking of Randall?)\n\nSorry, yes I had confused you with Randall\n\nApologies\n\nPhillip\n\n"},{"id":"450721","messageId":"09a0bb3f-1e03-5692-f54e-2ff080c21cc8@gmail.com","threadId":"57509","inReplyTo":"220307.86o82hzucl.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-08T10:54:59Z","receivedAt":"2022-03-08T10:55:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/03/2022 14:45, Ævar Arnfjörð Bjarmason wrote:\n>[...] \n> On Mon, Mar 07 2022, Phillip Wood wrote:\n> \n>> On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:\n>>> On Mon, Mar 07 2022, Phillip Wood wrote:\n>>>\n>>>> Hi Ævar\n>>>>\n>>>> On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:\n>>>>> [...]\n>>>>>> int save_term(unsigned flags)\n>>>>>> {\n>>>>>> +\tstruct sigaction sa;\n>>>>>> +\n>>>>>> \tif (term_fd < 0)\n>>>>>> \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n>>>>>> \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n>>>>>> @@ -44,6 +136,26 @@ int save_term(unsigned flags)\n>>>>>> \tif (tcgetattr(term_fd, &old_term) < 0)\n>>>>>> \t\treturn -1;\n>>>>>> \tsigchain_push_common(restore_term_on_signal);\n>>>>>> +\t/*\n>>>>>> +\t * If job control is disabled then the shell will have set the\n>>>>>> +\t * disposition of SIGTSTP to SIG_IGN.\n>>>>>> +\t */\n>>>>>> +\tsigaction(SIGTSTP, NULL, &sa);\n>>>>>> +\tif (sa.sa_handler == SIG_IGN)\n>>>>>> +\t\treturn 0;\n>>>>>> +\n>>>>>> +\t/* avoid calling gettext() from signal handler */\n>>>>>> +\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n>>>>>> +\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n>>>>> You don't need to xstrdup() the return values of gettext() (here\n>>>>> _()),\n>>>>> you'll get a pointer to static storage that's safe to hold on to for the\n>>>>> duration of the program.\n>>>>\n>>>> I had a look at the documentation and could not see anything about the\n>>>> lifetime of the returned string, all it says is \"don't alter it\"\n>>> I think this is overed in \"11.2.7 Optimization of the *gettext\n>>> functions\", a pedantic reading might suggest not, but what's meant with\n>>> the combination of that API documentation & the description of how MO\n>>> files work is that you're just getting pointers into the already-loaded\n>>> translation catalog, so it's safe to hold onto the pointer and re-use it\n>>> later.\n>>\n>> Strictly that section only shows it is safe if there are no other\n>> calls to gettext() before the returned string is used. I agree the\n>> implementation is likely to be just returning static strings but I\n>> can't find anywhere that says another implementation (e.g. on\n>> macos/*bsd) has to do that.\n> \n> I agree. I'm 99.99% sure this is safe & portable use of the API, but I'm\n> having some trouble finding documentation for that...\n> \n>>> In any case, if we're going to be paranoid about gettext() it would make\n>>> sense to propose that as some general change to how we use it, we rely\n>>> on this assumption holding in a lot of our use of the API:\n>>> git grep '= _\\('\n>>> Rather than sneak that partcular new assumption in here in this\n>>> already\n>>> tricky code...\n>>\n>> The ones I looked at are mostly not calling gettext() again before\n>> using the translated string (there is one exception in\n>> builtin/remote.c).\n> \n> Doesn't validate_encoding() in convert.c, process_entry() in\n> merge-ort.c, setup_unpack_trees_porcelain() in unpack-trees.c cmd_mv()\n> in builtin/mv.c etc. qualify?\n\nI only checked a few, cmd_mv() always assigns to the same variable so \nthe previous value is overwritten anyway, some of the others such as \nunpack_trees are assuming the return value is valid after a subsequent \ncall to gettext(). I found[1] which states\n\n The string returned must not be modified by the program and can\n be invalidated by a subsequent call to bind_textdomain_codeset()\n or setlocale(3C).\n\nso I think we can drop the copying.\n\n> I.e. for a hypothetical gettext() that always returned the same pointer\n> and just overwrote it with the latest message those would all emit bad\n> output, wouldn't they?\n> \n>> In restore_term() I'm checking if the messages are NULL to see if job\n>> control is enabled, I could use a flag but I'm inclined to just keep\n>> coping the strings.\n> \n> Checking if they're NULL is orthagonal to whether we xstrdup()\n> them. I.e. you'd just skip the xstrdup() and replace the FREE_AND_NULL\n> with a \"= NULL\" assignment, no?\n\nYes, I'm not sure what I was thinking when I wrote that.\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://docs.oracle.com/cd/E88353_01/html/E37843/gettext-3c.html#REFMAN3Agettext-3c\n\n"},{"id":"450824","messageId":"20220309110325.36917-1-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-09T11:03:21Z","receivedAt":"2022-03-09T11:04:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThanks to Ramsay and Ævar for their comments on V1.\nChanges since V1:\n * Patch 1\n - use an enum for save_term()'s flags (suggested by Ævar)\n - fixed argument order in the windows code (thanks to Ramsay)\n * Patch 2\n - fixed a typo in a comment (thanks to Ramsay)\n * Patch 4\n - stopped duplicating the strings returned by gettext() (suggested by\n Ævar)\n - reworked error message handling in the signal handler to add an\n \"error: \" prefix (suggested by Ævar)\n - tweaked the background resume error message\n\nV1 Cover Letter:\n\nFix the remaining issues that I'm aware of when using the built in\n\"add -p\" with interactive.singlekey that are stopping it from being\nmerged to master. The first three patches make sure that we call\ntcsetattr() and the same file descriptor that we use for read() and\nwork around poll() being broken when reading from terminals on\nmacos. The final patch is more of an improvement rather than a bug fix\n(the same issue already exists in the perl version) and could proceed\nseparately.\n\nUnfortunately these patches conflict with\n'cb/save-term-across-editor-invocation' as well as the textual\nconflicts there is a semantic conflict as the argument to save_term()\nis changed so the code in editor.c will need updating.\n\nThese patches are based on 'pw/single-key-interactive'\n\nPhillip Wood (4):\n terminal: use flags for save_term()\n terminal: don't assume stdin is /dev/tty\n terminal: work around macos poll() bug\n terminal: restore settings on SIGTSTP\n\n compat/terminal.c | 206 +++++++++++++++++++++++++++++++++++++++-------\n compat/terminal.h | 9 +-\n 2 files changed, 186 insertions(+), 29 deletions(-)\n\nRange-diff against v1:\n1: 6961a4e0dc ! 1: fac5a0f5b7 terminal: use flags for save_term()\n @@ Commit message\n terminal: use flags for save_term()\n \n The next commit will add another flag in addition to the existing\n - full_duplex so change the function signature to take an unsigned flags\n + full_duplex so change the function signature to take a flags\n argument. Also alter the functions that call save_term() so that they\n can pass flags down to it.\n \n + The choice to use an enum for tho bitwise flags is because gdb will\n + display the symbolic names of all the flags that are set rather than\n + the integer value.\n +\n Signed-off-by: Phillip Wood \n \n ## compat/terminal.c ##\n @@ compat/terminal.c: void restore_term(void)\n }\n \n -int save_term(int full_duplex)\n -+int save_term(unsigned flags)\n ++int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n @@ compat/terminal.c: int save_term(int full_duplex)\n }\n \n -static int disable_bits(tcflag_t bits)\n -+static int disable_bits(unsigned flags, tcflag_t bits)\n ++static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n {\n \tstruct termios t;\n \n @@ compat/terminal.c: static int disable_bits(tcflag_t bits)\n }\n \n -static int disable_echo(void)\n -+static int disable_echo(unsigned flags)\n ++static int disable_echo(enum save_term_flags flags)\n {\n -\treturn disable_bits(ECHO);\n +\treturn disable_bits(flags, ECHO);\n }\n \n -static int enable_non_canonical(void)\n -+static int enable_non_canonical(unsigned flags)\n ++static int enable_non_canonical(enum save_term_flags flags)\n {\n -\treturn disable_bits(ICANON | ECHO);\n +\treturn disable_bits(flags, ICANON | ECHO);\n @@ compat/terminal.c: void restore_term(void)\n }\n \n -int save_term(int full_duplex)\n -+int save_term(unsigned flags)\n ++int save_term(enum save_term_flags flags)\n {\n \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n @@ compat/terminal.c: int save_term(int full_duplex)\n }\n \n -static int disable_bits(DWORD bits)\n -+static int disable_bits(unsigned flags, DWORD bits)\n ++static int disable_bits(enum save_term_flags flags, DWORD bits)\n {\n \tif (use_stty) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n @@ compat/terminal.c: static int disable_bits(DWORD bits)\n }\n \n -static int disable_echo(void)\n -+static int disable_echo(unsigned flags)\n ++static int disable_echo(enum save_term_flags flags)\n {\n -\treturn disable_bits(ENABLE_ECHO_INPUT);\n -+\treturn disable_bits(ENABLE_ECHO_INPUT, flags);\n ++\treturn disable_bits(flags, ENABLE_ECHO_INPUT);\n }\n \n -static int enable_non_canonical(void)\n -+static int enable_non_canonical(unsigned flags)\n ++static int enable_non_canonical(enum save_term_flags flags)\n {\n -\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n -+\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT,\n -+\t\t\t flags);\n ++\treturn disable_bits(flags,\n ++\t\t\t ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n }\n \n /*\n @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)\n #else\n \n -int save_term(int full_duplex)\n -+int save_term(unsigned flags)\n ++int save_term(enum save_term_flags flags)\n {\n -\t/* full_duplex == 1, but no support available */\n -\treturn -full_duplex;\n @@ compat/terminal.h\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n -+/* Save input and output settings */\n -+#define SAVE_TERM_DUPLEX (1u << 0)\n ++enum save_term_flags {\n ++\t/* Save input and output settings */\n ++\tSAVE_TERM_DUPLEX = 1 << 0,\n ++};\n +\n /*\n * Save the terminal attributes so they can be restored later by a\n2: 45315087d4 ! 2: bf29460ec6 terminal: don't assume stdin is /dev/tty\n @@ Commit message\n \n ## compat/terminal.c ##\n @@ compat/terminal.c: void restore_term(void)\n - int save_term(unsigned flags)\n + int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n -\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)\n \n ## compat/terminal.h ##\n @@\n + enum save_term_flags {\n + \t/* Save input and output settings */\n + \tSAVE_TERM_DUPLEX = 1 << 0,\n ++\t/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */\n ++\tSAVE_TERM_STDIN = 1 << 1,\n + };\n \n - /* Save input and output settings */\n - #define SAVE_TERM_DUPLEX (1u << 0)\n --\n -+/* Save stdin rather than /dev/tty (fails is stdin is not a terminal) */\n -+#define SAVE_TERM_STDIN (1u << 1)\n /*\n - * Save the terminal attributes so they can be restored later by a\n - * call to restore_term(). Note that every successful call to\n4: 62703be209 ! 4: dd0e1fabb1 terminal: restore settings on SIGTSTP\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n -+static char *background_resume_msg;\n -+static char *restore_error_msg;\n ++static const char *background_resume_msg;\n ++static const char *restore_error_msg;\n +static volatile sig_atomic_t ttou_received;\n +\n -+static void write_msg(const char *msg)\n ++static void write_err(const char *msg)\n +{\n ++\twrite_in_full(2, \"error: \", strlen(\"error: \"));\n +\twrite_in_full(2, msg, strlen(msg));\n +\twrite_in_full(2, \"\\n\", 1);\n +}\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n +\n +\tttou_received = 1;\n -+\twrite_msg(background_resume_msg);\n ++\twrite_err(background_resume_msg);\n +\tsigaction(signo, &sa, &old_sa);\n +\traise(signo);\n +\tsigemptyset(&mask);\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +\t\tcan_restore = 0;\n +\n +\tif (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)\n -+\t\twrite_msg(restore_error_msg);\n ++\t\twrite_err(restore_error_msg);\n +\n +\tsigaction(signo, &sa, &old_sa);\n +\traise(signo);\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +\tsigprocmask(SIG_BLOCK, &mask, NULL);\n +\tsigaction(signo, &old_sa, NULL);\n +\tif (!can_restore) {\n -+\t\twrite_msg(restore_error_msg);\n ++\t\twrite_err(restore_error_msg);\n +\t\tgoto out;\n +\t}\n +\t/*\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +\tif (ttou_received)\n +\t\tgoto again;\n +\telse if (res < 0)\n -+\t\twrite_msg(restore_error_msg);\n ++\t\twrite_err(restore_error_msg);\n +\tsigaction(SIGTTOU, &old_sa, NULL);\n + out:\n +\terrno = saved_errno;\n @@ compat/terminal.c: void restore_term(void)\n +\t\tsignal(SIGTTIN, SIG_DFL);\n +\t\tsignal(SIGTTOU, SIG_DFL);\n +\t\tsignal(SIGTSTP, SIG_DFL);\n -+\t\tFREE_AND_NULL(restore_error_msg);\n -+\t\tFREE_AND_NULL(background_resume_msg);\n ++\t\trestore_error_msg = NULL;\n ++\t\tbackground_resume_msg = NULL;\n +\t}\n }\n \n - int save_term(unsigned flags)\n + int save_term(enum save_term_flags flags)\n {\n +\tstruct sigaction sa;\n +\n \tif (term_fd < 0)\n \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n -@@ compat/terminal.c: int save_term(unsigned flags)\n +@@ compat/terminal.c: int save_term(enum save_term_flags flags)\n \tif (tcgetattr(term_fd, &old_term) < 0)\n \t\treturn -1;\n \tsigchain_push_common(restore_term_on_signal);\n @@ compat/terminal.c: int save_term(unsigned flags)\n +\t\treturn 0;\n +\n +\t/* avoid calling gettext() from signal handler */\n -+\tbackground_resume_msg = xstrdup(_(\"error: cannot resume in the background\"));\n -+\trestore_error_msg = xstrdup(_(\"error: cannot restore terminal settings\"));\n ++\tbackground_resume_msg = _(\"cannot resume in the background, please use 'fg' to resume\");\n ++\trestore_error_msg = _(\"cannot restore terminal settings\");\n +\tsa.sa_handler = restore_terminal_on_suspend;\n +\tsa.sa_flags = SA_RESTART;\n +\tsigemptyset(&sa.sa_mask);\n-- \n2.35.1\n\n"},{"id":"450825","messageId":"20220309110325.36917-2-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220309110325.36917-1-phillip.wood123@gmail.com","subject":"[PATCH v2 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-09T11:03:22Z","receivedAt":"2022-03-09T11:04:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe next commit will add another flag in addition to the existing\nfull_duplex so change the function signature to take a flags\nargument. Also alter the functions that call save_term() so that they\ncan pass flags down to it.\n\nThe choice to use an enum for tho bitwise flags is because gdb will\ndisplay the symbolic names of all the flags that are set rather than\nthe integer value.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 41 +++++++++++++++++++++--------------------\n compat/terminal.h | 7 ++++++-\n 2 files changed, 27 insertions(+), 21 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex d882dfa06e..9392b068b1 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -34,7 +34,7 @@ void restore_term(void)\n \tsigchain_pop_common();\n }\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n@@ -47,11 +47,11 @@ int save_term(int full_duplex)\n \treturn 0;\n }\n \n-static int disable_bits(tcflag_t bits)\n+static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\tgoto error;\n \n \tt = old_term;\n@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)\n \treturn -1;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(enum save_term_flags flags)\n {\n-\treturn disable_bits(ECHO);\n+\treturn disable_bits(flags, ECHO);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(enum save_term_flags flags)\n {\n-\treturn disable_bits(ICANON | ECHO);\n+\treturn disable_bits(flags, ICANON | ECHO);\n }\n \n #elif defined(GIT_WINDOWS_NATIVE)\n@@ -126,15 +126,15 @@ void restore_term(void)\n \thconin = hconout = INVALID_HANDLE_VALUE;\n }\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n \t FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn -1;\n \n-\tif (full_duplex) {\n+\tif (flags & SAVE_TERM_DUPLEX) {\n \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n@@ -154,7 +154,7 @@ int save_term(int full_duplex)\n \treturn -1;\n }\n \n-static int disable_bits(DWORD bits)\n+static int disable_bits(enum save_term_flags flags, DWORD bits)\n {\n \tif (use_stty) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\treturn -1;\n \n \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)\n \treturn 0;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(enum save_term_flags flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT);\n+\treturn disable_bits(flags, ENABLE_ECHO_INPUT);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(enum save_term_flags flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n+\treturn disable_bits(flags,\n+\t\t\t ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n }\n \n /*\n@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n \t\treturn NULL;\n \t}\n \n-\tif (!echo && disable_echo()) {\n+\tif (!echo && disable_echo(0)) {\n \t\tfclose(input_fh);\n \t\tfclose(output_fh);\n \t\treturn NULL;\n@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical() < 0) {\n+\tif (warning_displayed || enable_non_canonical(0) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\n@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)\n \n #else\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n-\t/* full_duplex == 1, but no support available */\n-\treturn -full_duplex;\n+\t/* no duplex support available */\n+\treturn -!!(flags & SAVE_TERM_DUPLEX);\n }\n \n void restore_term(void)\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex 0fb9fa147c..24c4df4c0e 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,14 +1,19 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+enum save_term_flags {\n+\t/* Save input and output settings */\n+\tSAVE_TERM_DUPLEX = 1 << 0,\n+};\n+\n /*\n * Save the terminal attributes so they can be restored later by a\n * call to restore_term(). Note that every successful call to\n * save_term() must be matched by a call to restore_term() even if the\n * attributes have not been changed. Returns 0 on success, -1 on\n * failure.\n */\n-int save_term(int full_duplex);\n+int save_term(unsigned flags);\n /* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n-- \n2.35.1\n\n"},{"id":"450826","messageId":"20220309110325.36917-3-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220309110325.36917-1-phillip.wood123@gmail.com","subject":"[PATCH v2 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-09T11:03:23Z","receivedAt":"2022-03-09T11:04:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nread_key_without_echo() reads from stdin but uses /dev/tty when it\ndisables echo. This is unfortunate as there no guarantee that stdin is\nthe same device as /dev/tty. The perl version of \"add -p\" uses stdin\nwhen it sets the terminal mode, this commit does the same for the\nbuiltin version. There is still a difference between the perl and\nbuiltin versions though - the perl version will ignore any errors when\nsetting the terminal mode[1] and will still read single bytes when\nstdin is not a terminal. The builtin version displays a warning if\nsetting the terminal mode fails and switches to reading a line at a\ntime.\n\n[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 5 +++--\n compat/terminal.h | 2 ++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 9392b068b1..cb653419c3 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -37,7 +37,8 @@ void restore_term(void)\n int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n-\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n+\t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n \tif (term_fd < 0)\n \t\treturn -1;\n \tif (tcgetattr(term_fd, &old_term) < 0)\n@@ -362,7 +363,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical(0) < 0) {\n+\tif (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex 24c4df4c0e..3217611b08 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -4,6 +4,8 @@\n enum save_term_flags {\n \t/* Save input and output settings */\n \tSAVE_TERM_DUPLEX = 1 << 0,\n+\t/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */\n+\tSAVE_TERM_STDIN = 1 << 1,\n };\n \n /*\n-- \n2.35.1\n\n"},{"id":"450827","messageId":"20220309110325.36917-4-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220309110325.36917-1-phillip.wood123@gmail.com","subject":"[PATCH v2 3/4] terminal: work around macos poll() bug","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-09T11:03:24Z","receivedAt":"2022-03-09T11:04:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nOn macos the builtin \"add -p\" does not handle keys that generate\nescape sequences because poll() does not work with terminals\nthere. Switch to using select() on non-windows platforms to work\naround this.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 43 +++++++++++++++++++++++++++++++++++++------\n 1 file changed, 37 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex cb653419c3..4189cbb12c 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -82,6 +82,32 @@ static int enable_non_canonical(enum save_term_flags flags)\n \treturn disable_bits(flags, ICANON | ECHO);\n }\n \n+/*\n+ * On macos it is not possible to use poll() with a terminal so use select\n+ * instead.\n+ */\n+#include \n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct timeval tv, *tvp = NULL;\n+\tfd_set readfds;\n+\tint res;\n+\n+\tif (timeout >= 0) {\n+\t\ttv.tv_sec = timeout / 1000;\n+\t\ttv.tv_usec = (timeout % 1000) * 1000;\n+\t\ttvp = &tv;\n+\t}\n+\n+\tFD_ZERO(&readfds);\n+\tFD_SET(0, &readfds);\n+\tres = select(1, &readfds, NULL, NULL, tvp);\n+\tif (res < 0)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #elif defined(GIT_WINDOWS_NATIVE)\n \n #define INPUT_PATH \"CONIN$\"\n@@ -247,6 +273,16 @@ static int mingw_getchar(void)\n }\n #define getchar mingw_getchar\n \n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n+\n+\tif (poll(&pfd, 1, timeout) < 1)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #endif\n \n #ifndef FORCE_TEXT\n@@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)\n \t\t * half a second when we know that the sequence is complete.\n \t\t */\n \t\twhile (!is_known_escape_sequence(buf->buf)) {\n-\t\t\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n-\n-\t\t\tif (poll(&pfd, 1, 500) < 1)\n-\t\t\t\tbreak;\n-\n-\t\t\tch = getchar();\n+\t\t\tch = getchar_with_timeout(500);\n \t\t\tif (ch == EOF)\n \t\t\t\treturn 0;\n \t\t\tstrbuf_addch(buf, ch);\n-- \n2.35.1\n\n"},{"id":"450828","messageId":"20220309110325.36917-5-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220309110325.36917-1-phillip.wood123@gmail.com","subject":"[PATCH v2 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-09T11:03:25Z","receivedAt":"2022-03-09T11:04:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf the user suspends git while it is waiting for a keypress reset the\nterminal before stopping and restore the settings when git resumes. If\nthe user tries to resume in the background print an error\nmessage (taking care to use async safe functions) before stopping\nagain. Ideally we would reprint the prompt for the user when git\nresumes but this patch just restarts the read().\n\nThe signal handler is established with sigaction() rather than using\nsigchain_push() as this allows us to control the signal mask when the\nhandler is invoked and ensure SA_RESTART is used to restart the\nread() when resuming.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 125 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 121 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 4189cbb12c..f425f7b339 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -1,4 +1,4 @@\n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"compat/terminal.h\"\n #include \"sigchain.h\"\n #include \"strbuf.h\"\n@@ -23,6 +23,90 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n+static const char *background_resume_msg;\n+static const char *restore_error_msg;\n+static volatile sig_atomic_t ttou_received;\n+\n+static void write_err(const char *msg)\n+{\n+\twrite_in_full(2, \"error: \", strlen(\"error: \"));\n+\twrite_in_full(2, msg, strlen(msg));\n+\twrite_in_full(2, \"\\n\", 1);\n+}\n+\n+static void print_background_resume_msg(int signo)\n+{\n+\tint saved_errno = errno;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\n+\tttou_received = 1;\n+\twrite_err(background_resume_msg);\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\terrno = saved_errno;\n+}\n+\n+static void restore_terminal_on_suspend(int signo)\n+{\n+\tint saved_errno = errno;\n+\tint res;\n+\tstruct termios t;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\tint can_restore = 1;\n+\n+\tif (tcgetattr(term_fd, &t) < 0)\n+\t\tcan_restore = 0;\n+\n+\tif (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)\n+\t\twrite_err(restore_error_msg);\n+\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\tif (!can_restore) {\n+\t\twrite_err(restore_error_msg);\n+\t\tgoto out;\n+\t}\n+\t/*\n+\t * If we resume in the background then we receive SIGTTOU when calling\n+\t * tcsetattr() below. Set up a handler to print an error message in that\n+\t * case.\n+\t */\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, SIGTTOU);\n+\tsa.sa_mask = old_sa.sa_mask;\n+\tsa.sa_handler = print_background_resume_msg;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGTTOU, &sa, &old_sa);\n+ again:\n+\tttou_received = 0;\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\tres = tcsetattr(term_fd, TCSAFLUSH, &t);\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tif (ttou_received)\n+\t\tgoto again;\n+\telse if (res < 0)\n+\t\twrite_err(restore_error_msg);\n+\tsigaction(SIGTTOU, &old_sa, NULL);\n+ out:\n+\terrno = saved_errno;\n+}\n+\n void restore_term(void)\n {\n \tif (term_fd < 0)\n@@ -32,10 +116,19 @@ void restore_term(void)\n \tclose(term_fd);\n \tterm_fd = -1;\n \tsigchain_pop_common();\n+\tif (restore_error_msg) {\n+\t\tsignal(SIGTTIN, SIG_DFL);\n+\t\tsignal(SIGTTOU, SIG_DFL);\n+\t\tsignal(SIGTSTP, SIG_DFL);\n+\t\trestore_error_msg = NULL;\n+\t\tbackground_resume_msg = NULL;\n+\t}\n }\n \n int save_term(enum save_term_flags flags)\n {\n+\tstruct sigaction sa;\n+\n \tif (term_fd < 0)\n \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n@@ -44,6 +137,26 @@ int save_term(enum save_term_flags flags)\n \tif (tcgetattr(term_fd, &old_term) < 0)\n \t\treturn -1;\n \tsigchain_push_common(restore_term_on_signal);\n+\t/*\n+\t * If job control is disabled then the shell will have set the\n+\t * disposition of SIGTSTP to SIG_IGN.\n+\t */\n+\tsigaction(SIGTSTP, NULL, &sa);\n+\tif (sa.sa_handler == SIG_IGN)\n+\t\treturn 0;\n+\n+\t/* avoid calling gettext() from signal handler */\n+\tbackground_resume_msg = _(\"cannot resume in the background, please use 'fg' to resume\");\n+\trestore_error_msg = _(\"cannot restore terminal settings\");\n+\tsa.sa_handler = restore_terminal_on_suspend;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigemptyset(&sa.sa_mask);\n+\tsigaddset(&sa.sa_mask, SIGTSTP);\n+\tsigaddset(&sa.sa_mask, SIGTTIN);\n+\tsigaddset(&sa.sa_mask, SIGTTOU);\n+\tsigaction(SIGTSTP, &sa, NULL);\n+\tsigaction(SIGTTIN, &sa, NULL);\n+\tsigaction(SIGTTOU, &sa, NULL);\n \n \treturn 0;\n }\n@@ -93,6 +206,7 @@ static int getchar_with_timeout(int timeout)\n \tfd_set readfds;\n \tint res;\n \n+ again:\n \tif (timeout >= 0) {\n \t\ttv.tv_sec = timeout / 1000;\n \t\ttv.tv_usec = (timeout % 1000) * 1000;\n@@ -102,9 +216,12 @@ static int getchar_with_timeout(int timeout)\n \tFD_ZERO(&readfds);\n \tFD_SET(0, &readfds);\n \tres = select(1, &readfds, NULL, NULL, tvp);\n-\tif (res < 0)\n-\t\treturn EOF;\n-\n+\tif (res < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tgoto again;\n+\t\telse\n+\t\t\treturn EOF;\n+\t}\n \treturn getchar();\n }\n \n-- \n2.35.1\n\n"},{"id":"450834","messageId":"nycvar.QRO.7.76.6.2203091310080.357@tvgsbejvaqbjf.bet","threadId":"57509","inReplyTo":"20220304131126.8293-5-phillip.wood123@gmail.com","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-09T12:19:32Z","receivedAt":"2022-03-09T12:19:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 4 Mar 2022, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> If the user suspends git while it is waiting for a keypress reset the\n> terminal before stopping and restore the settings when git resumes. If\n> the user tries to resume in the background print an error\n> message (taking care to use async safe functions) before stopping\n> again. Ideally we would reprint the prompt for the user when git\n> resumes but this patch just restarts the read().\n>\n> The signal handler is established with sigaction() rather than using\n> sigchain_push() as this allows us to control the signal mask when the\n> handler is invoked and ensure SA_RESTART is used to restart the\n> read() when resuming.\n\nThis description makes sense. From my understanding of signals, the code\nalso does make sense, but it is unfortunate that it has to be so much code\nto implement something as straight-forward as suspend/resume.\n\nFWIW I tested the `add -p` command with these patches on Windows and it\nstill works as well as when I had developed it.\n\nThank you,\nDscho\n"},{"id":"450955","messageId":"xmqqilsmlo31.fsf@gitster.g","threadId":"57509","inReplyTo":"20220309110325.36917-1-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-09T23:10:10Z","receivedAt":"2022-03-09T23:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> V1 Cover Letter:\n>\n> Fix the remaining issues that I'm aware of when using the built in\n> \"add -p\" with interactive.singlekey that are stopping it from being\n> merged to master. The first three patches make sure that we call\n> tcsetattr() and the same file descriptor that we use for read() and\n> work around poll() being broken when reading from terminals on\n> macos. The final patch is more of an improvement rather than a bug fix\n> (the same issue already exists in the perl version) and could proceed\n> separately.\n>\n> Unfortunately these patches conflict with\n> 'cb/save-term-across-editor-invocation' as well as the textual\n> conflicts there is a semantic conflict as the argument to save_term()\n> is changed so the code in editor.c will need updating.\n>\n> These patches are based on 'pw/single-key-interactive'\n\nIs it still true, or does the base only apply to v1?\n\n$ git checkout --detach pw/single-key-interactive\nHEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set\n$ git am -s ./+pw4-v2-add-p-single\nApplying: terminal: use flags for save_term()\nApplying: terminal: don't assume stdin is /dev/tty\nApplying: terminal: work around macos poll() bug\nerror: patch failed: compat/terminal.c:397\nerror: compat/terminal.c: patch does not apply\nPatch failed at 0003 terminal: work around macos poll() bug\nhint: Use 'git am --show-current-patch=diff' to see the failed patch\nWhen you have resolved this problem, run \"git am --continue\".\nIf you prefer to skip this patch, run \"git am --skip\" instead.\nTo restore the original branch and stop patching, run \"git am --abort\".\n$ exit\n"},{"id":"450957","messageId":"xmqqzglyk89e.fsf@gitster.g","threadId":"57509","inReplyTo":"xmqqilsmlo31.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-09T23:37:17Z","receivedAt":"2022-03-09T23:37:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> These patches are based on 'pw/single-key-interactive'\n>\n> Is it still true, or does the base only apply to v1?\n>\n> $ git checkout --detach pw/single-key-interactive\n> HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set\n> $ git am -s ./+pw4-v2-add-p-single\n> Applying: terminal: use flags for save_term()\n> Applying: terminal: don't assume stdin is /dev/tty\n> Applying: terminal: work around macos poll() bug\n> error: patch failed: compat/terminal.c:397\n> error: compat/terminal.c: patch does not apply\n> Patch failed at 0003 terminal: work around macos poll() bug\n> hint: Use 'git am --show-current-patch=diff' to see the failed patch\n> When you have resolved this problem, run \"git am --continue\".\n> If you prefer to skip this patch, run \"git am --skip\" instead.\n> To restore the original branch and stop patching, run \"git am --abort\".\n> $ exit\n\nI think I figured it out. A merge of pw/single-key-interactive into\na recent tip of 'master' wants the \"return 0\" in the preimage below\nto be \"break\" in compat/terminal.c\n\n\n> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)\n> \t\t * half a second when we know that the sequence is complete.\n> \t\t */\n> \t\twhile (!is_known_escape_sequence(buf->buf)) {\n> -\t\t\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n> -\n> -\t\t\tif (poll(&pfd, 1, 500) < 1)\n> -\t\t\t\tbreak;\n> -\n> -\t\t\tch = getchar();\n> +\t\t\tch = getchar_with_timeout(500);\n> \t\t\tif (ch == EOF)\n> \t\t\t\treturn 0;\n> \t\t\tstrbuf_addch(buf, ch);\n\nI'll wiggle it in to see how well it fares with other topics in\nflight.\n\nThanks.\n"},{"id":"450996","messageId":"nycvar.QRO.7.76.6.2203101422500.357@tvgsbejvaqbjf.bet","threadId":"57509","inReplyTo":"20220309110325.36917-1-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-10T13:25:00Z","receivedAt":"2022-03-10T13:25:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 9 Mar 2022, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> Thanks to Ramsay and Ævar for their comments on V1.\n> Changes since V1:\n> * Patch 1\n> - use an enum for save_term()'s flags (suggested by Ævar)\n> - fixed argument order in the windows code (thanks to Ramsay)\n> * Patch 2\n> - fixed a typo in a comment (thanks to Ramsay)\n> * Patch 4\n> - stopped duplicating the strings returned by gettext() (suggested by\n> Ævar)\n> - reworked error message handling in the signal handler to add an\n> \"error: \" prefix (suggested by Ævar)\n> - tweaked the background resume error message\n\nWhile I did not ask for any of these changes, they look good to me. I had\na look over the range-diff and found it reasonable.\n\nOnwards to a bright built-in `add -p` future!\n\nThanks,\nDscho\n"},{"id":"450998","messageId":"93d197db-c52c-101b-bdb0-3b4c9b073705@gmail.com","threadId":"57509","inReplyTo":"xmqqzglyk89e.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-10T13:28:05Z","receivedAt":"2022-03-10T13:28:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 09/03/2022 23:37, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>>> These patches are based on 'pw/single-key-interactive'\n>>\n>> Is it still true, or does the base only apply to v1?\n\nThe base is unchanged but does not seem to match\npw/single-key-interactive. I'm not sure what happened there. They are\nbased on 300db53b37 (\"add -p: disable stdin buffering when\ninteractive.singlekey is set\", 2022-02-16) which is the second parent\nof e53fb7aa3f (\"Merge branch 'pw/single-key-interactive' into seen\",\n2022-02-20)\n\n>> $ git checkout --detach pw/single-key-interactive\n>> HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set\n>> $ git am -s ./+pw4-v2-add-p-single\n>> Applying: terminal: use flags for save_term()\n>> Applying: terminal: don't assume stdin is /dev/tty\n>> Applying: terminal: work around macos poll() bug\n>> error: patch failed: compat/terminal.c:397\n>> error: compat/terminal.c: patch does not apply\n>> Patch failed at 0003 terminal: work around macos poll() bug\n>> hint: Use 'git am --show-current-patch=diff' to see the failed patch\n>> When you have resolved this problem, run \"git am --continue\".\n>> If you prefer to skip this patch, run \"git am --skip\" instead.\n>> To restore the original branch and stop patching, run \"git am --abort\".\n>> $ exit\n> \n> I think I figured it out. A merge of pw/single-key-interactive into\n> a recent tip of 'master' wants the \"return 0\" in the preimage below\n> to be \"break\" in compat/terminal.c\n> \n> \n>> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)\n>> \t\t * half a second when we know that the sequence is complete.\n>> \t\t */\n>> \t\twhile (!is_known_escape_sequence(buf->buf)) {\n>> -\t\t\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n>> -\n>> -\t\t\tif (poll(&pfd, 1, 500) < 1)\n>> -\t\t\t\tbreak;\n>> -\n>> -\t\t\tch = getchar();\n>> +\t\t\tch = getchar_with_timeout(500);\n>> \t\t\tif (ch == EOF)\n>> \t\t\t\treturn 0;\n>> \t\t\tstrbuf_addch(buf, ch);\n\nThat looks good to me. However unfortunately there are some semantic\nconflicts as well. The patch below is based on 6b1f77214c (\"Merge branch\n'pw/add-p-single-key' into seen\", 2022-03-09), hopefully Thunderbird wont\nmangle it. Whilst preparing the fixup I realized I need to reroll to\nfix a closing stdin in patch 2 and resetting the job signals on error in\npatch 4. What's the best base to use when rerolling?\n\nBest Wishes\n\nPhillip\n\n---- >8 ----\n\nFrom: Phillip Wood \nDate: Thu, 10 Mar 2022 11:05:26 +0000\nSubject: [PATCH] fixup! Merge branch 'pw/add-p-single-key' into seen\n\n---\n compat/terminal.c | 3 ++-\n editor.c | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 3172f4f408..d1ed5c07dc 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -141,7 +141,8 @@ int save_term(enum save_term_flags flags)\n if (term_fd < 0)\n return -1;\n if ((flags & SAVE_TERM_DUPLEX) && !is_controlling_terminal(term_fd)) {\n- close(term_fd);\n+ if (term_fd) /* avoid closing stdin */\n+ close(term_fd);\n term_fd = -1;\n return -1;\n }\ndiff --git a/editor.c b/editor.c\nindex 6c5c95e6a2..192d6ea75d 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -55,7 +55,7 @@ static int prepare_term(const char *editor)\n \n git_config_get_bool(\"editor.stty\", &need_saverestore);\n if (need_saverestore)\n- return save_term(1);\n+ return save_term(SAVE_TERM_DUPLEX);\n return 0;\n }\n \n-- \n2.33.0.342.g580a6b0edd\n"},{"id":"451005","messageId":"220310.86v8wmj5c8.gmgdl@evledraar.gmail.com","threadId":"57509","inReplyTo":"20220309110325.36917-4-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 3/4] terminal: work around macos poll() bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-10T13:35:00Z","receivedAt":"2022-03-10T13:38:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 09 2022, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> On macos the builtin \"add -p\" does not handle keys that generate\n> escape sequences because poll() does not work with terminals\n> there. Switch to using select() on non-windows platforms to work\n> around this.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> compat/terminal.c | 43 +++++++++++++++++++++++++++++++++++++------\n> 1 file changed, 37 insertions(+), 6 deletions(-)\n>\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index cb653419c3..4189cbb12c 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -82,6 +82,32 @@ static int enable_non_canonical(enum save_term_flags flags)\n> \treturn disable_bits(flags, ICANON | ECHO);\n> }\n> \n> +/*\n> + * On macos it is not possible to use poll() with a terminal so use select\n> + * instead.\n> + */\n> +#include \n\nI don't think this breaks anything in practice due to the platforms we\ndefine HAVE_DEV_TTY and NO_SYS_SELECT_H on, but it does look redundant &\nconfusing, and will break if the current users of HAVE_DEV_TTY and\nNO_SYS_SELECT_H change.\n\nI.e. isn't sys/select.h already pulled in by the relevant include in\ngit-compat-util.h? Why is it needed again here?\n"},{"id":"451033","messageId":"45b98ac0-4d33-ee9a-e7f2-3e8137fa8a1a@gmail.com","threadId":"57509","inReplyTo":"220310.86v8wmj5c8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/4] terminal: work around macos poll() bug","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-10T16:02:43Z","receivedAt":"2022-03-10T16:02:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 10/03/2022 13:35, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>> \n>> +/*\n>> + * On macos it is not possible to use poll() with a terminal so use select\n>> + * instead.\n>> + */\n>> +#include \n> \n> I don't think this breaks anything in practice due to the platforms we\n> define HAVE_DEV_TTY and NO_SYS_SELECT_H on, but it does look redundant &\n> confusing, and will break if the current users of HAVE_DEV_TTY and\n> NO_SYS_SELECT_H change.\n> \n> I.e. isn't sys/select.h already pulled in by the relevant include in\n> git-compat-util.h? Why is it needed again here?\n\nI didn't realize that git-compat-util.h already included that header, \nI'll remove it.\n\nThanks\n\nPhillip\n"},{"id":"451034","messageId":"7fff6841-968d-b741-2589-fa737499113d@gmail.com","threadId":"57509","inReplyTo":"nycvar.QRO.7.76.6.2203091310080.357@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-10T16:06:29Z","receivedAt":"2022-03-10T16:06:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 09/03/2022 12:19, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Fri, 4 Mar 2022, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> If the user suspends git while it is waiting for a keypress reset the\n>> terminal before stopping and restore the settings when git resumes. If\n>> the user tries to resume in the background print an error\n>> message (taking care to use async safe functions) before stopping\n>> again. Ideally we would reprint the prompt for the user when git\n>> resumes but this patch just restarts the read().\n>>\n>> The signal handler is established with sigaction() rather than using\n>> sigchain_push() as this allows us to control the signal mask when the\n>> handler is invoked and ensure SA_RESTART is used to restart the\n>> read() when resuming.\n> \n> This description makes sense. From my understanding of signals, the code\n> also does make sense, but it is unfortunate that it has to be so much code\n> to implement something as straight-forward as suspend/resume.\n\nYes it is a lot of code. It would be a bit simpler if we omitted the \nwarning about resuming in the background but I think that is worth \nhaving. There's also a lot of changing signal masks to avoid stopping \ntwice if the user presses ^Z a second time while the signal handler is \nactive.\n\n> FWIW I tested the `add -p` command with these patches on Windows and it\n> still works as well as when I had developed it.\n\nThanks for testing this on Windows, I don't think we have any meaningful \ntest coverage for interactive.singlekey and it is probably tricky to add \nbecause it relies on having a tty.\n\n\nBest Wishes\n\nPhillip\n> Thank you,\n> Dscho\n"},{"id":"451035","messageId":"e22e2b8b-55b5-5392-5772-e8a5ff0ee245@gmail.com","threadId":"57509","inReplyTo":"nycvar.QRO.7.76.6.2203101422500.357@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-10T16:08:23Z","receivedAt":"2022-03-10T16:08:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 10/03/2022 13:25, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Wed, 9 Mar 2022, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> Thanks to Ramsay and Ævar for their comments on V1.\n>> Changes since V1:\n>> * Patch 1\n>> - use an enum for save_term()'s flags (suggested by Ævar)\n>> - fixed argument order in the windows code (thanks to Ramsay)\n>> * Patch 2\n>> - fixed a typo in a comment (thanks to Ramsay)\n>> * Patch 4\n>> - stopped duplicating the strings returned by gettext() (suggested by\n>> Ævar)\n>> - reworked error message handling in the signal handler to add an\n>> \"error: \" prefix (suggested by Ævar)\n>> - tweaked the background resume error message\n> \n> While I did not ask for any of these changes, they look good to me. I had\n> a look over the range-diff and found it reasonable.\n> \n> Onwards to a bright built-in `add -p` future!\n\nHopefully! I spotted a couple more small issues earlier so there will be \na v3 soonish\n\nBest Wishes\n\nPhillip\n"},{"id":"451052","messageId":"xmqqh785fzys.fsf@gitster.g","threadId":"57509","inReplyTo":"45b98ac0-4d33-ee9a-e7f2-3e8137fa8a1a@gmail.com","subject":"Re: [PATCH v2 3/4] terminal: work around macos poll() bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-10T18:02:19Z","receivedAt":"2022-03-10T18:02:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Ævar\n>\n> On 10/03/2022 13:35, Ævar Arnfjörð Bjarmason wrote:\n>> [...]\n>>> +/*\n>>> + * On macos it is not possible to use poll() with a terminal so use select\n>>> + * instead.\n>>> + */\n>>> +#include \n>> I don't think this breaks anything in practice due to the platforms\n>> we\n>> define HAVE_DEV_TTY and NO_SYS_SELECT_H on, but it does look redundant &\n>> confusing, and will break if the current users of HAVE_DEV_TTY and\n>> NO_SYS_SELECT_H change.\n>> I.e. isn't sys/select.h already pulled in by the relevant include in\n>> git-compat-util.h? Why is it needed again here?\n>\n> I didn't realize that git-compat-util.h already included that header,\n> I'll remove it.\n\nThanks, both. The removal of that line is very much appreciated.\n"},{"id":"451054","messageId":"a1e51433-71b1-b32d-0475-192b83bac43a@gmail.com","threadId":"57509","inReplyTo":"93d197db-c52c-101b-bdb0-3b4c9b073705@gmail.com","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-10T18:18:31Z","receivedAt":"2022-03-10T18:18:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/03/2022 13:28, Phillip Wood wrote:\n> Hi Junio\n> \n> On 09/03/2022 23:37, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> $ git checkout --detach pw/single-key-interactive\n>>> HEAD is now at ac618c418e add -p: disable stdin buffering when \n>>> interactive.singlekey is set\n>>> $ git am -s ./+pw4-v2-add-p-single\n>>> Applying: terminal: use flags for save_term()\n>>> Applying: terminal: don't assume stdin is /dev/tty\n>>> Applying: terminal: work around macos poll() bug\n>>> error: patch failed: compat/terminal.c:397\n>>> error: compat/terminal.c: patch does not apply\n>>> Patch failed at 0003 terminal: work around macos poll() bug\n>>> hint: Use 'git am --show-current-patch=diff' to see the failed patch\n>>> When you have resolved this problem, run \"git am --continue\".\n>>> If you prefer to skip this patch, run \"git am --skip\" instead.\n>>> To restore the original branch and stop patching, run \"git am --abort\".\n>>> $ exit\n>>\n>> I think I figured it out.  A merge of pw/single-key-interactive into\n>> a recent tip of 'master' wants the \"return 0\" in the preimage below\n>> to be \"break\" in compat/terminal.c\n>>\n>>\n>>> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)\n>>>            * half a second when we know that the sequence is complete.\n>>>            */\n>>>           while (!is_known_escape_sequence(buf->buf)) {\n>>> -            struct pollfd pfd = { .fd = 0, .events = POLLIN };\n>>> -\n>>> -            if (poll(&pfd, 1, 500) < 1)\n>>> -                break;\n>>> -\n>>> -            ch = getchar();\n>>> +            ch = getchar_with_timeout(500);\n>>>               if (ch == EOF)\n>>>                   return 0;\n\nLooking more closely that should be \"break\" not \"return 0\". I think what \nhas happened is that I accidentally based these on an old version of \npw/single-key-interactive which did not contain 24d7ce383a (\"terminal: \nalways reset terminal when reading without echo\", 2022-02-22)\n\nSorry for the confusion\n\nPhillip\n"},{"id":"451062","messageId":"xmqqo82dej0k.fsf@gitster.g","threadId":"57509","inReplyTo":"a1e51433-71b1-b32d-0475-192b83bac43a@gmail.com","subject":"Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-10T18:53:47Z","receivedAt":"2022-03-10T18:53:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Looking more closely that should be \"break\" not \"return 0\". I think\n> what has happened is that I accidentally based these on an old version\n> of pw/single-key-interactive which did not contain 24d7ce383a\n> (\"terminal: always reset terminal when reading without echo\",\n> 2022-02-22)\n\nI thought the base pw/single-key-interactive topic is solid enough\nand ready for 'next'? It probably is a good idea to rebase these\nfollow-on patches on top of a merge of the base topic into 'master'.\n\nI'll keep v2 in 'seen' but mark the topic as \"Expecting a reroll\"\nin the draft of the next issue of the \"What's cooking\" report.\n\nThanks.\n"},{"id":"451152","messageId":"CAPUEsph4Gzd4S_YNeCF+uHntGWXdcA2wY9XL=-+MZtJ0YZb-Sg@mail.gmail.com","threadId":"57509","inReplyTo":"20220309110325.36917-2-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 1/4] terminal: use flags for save_term()","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-03-11T16:52:10Z","receivedAt":"2022-03-11T16:52:25Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Mar 9, 2022 at 3:04 AM Phillip Wood wrote:\n> diff --git a/compat/terminal.h b/compat/terminal.h\n> index 0fb9fa147c..24c4df4c0e 100644\n> --- a/compat/terminal.h\n> +++ b/compat/terminal.h\n> @@ -1,14 +1,19 @@\n> #ifndef COMPAT_TERMINAL_H\n> #define COMPAT_TERMINAL_H\n>\n> +enum save_term_flags {\n> + /* Save input and output settings */\n> + SAVE_TERM_DUPLEX = 1 << 0,\n> +};\n> +\n> /*\n> * Save the terminal attributes so they can be restored later by a\n> * call to restore_term(). Note that every successful call to\n> * save_term() must be matched by a call to restore_term() even if the\n> * attributes have not been changed. Returns 0 on success, -1 on\n> * failure.\n> */\n> -int save_term(int full_duplex);\n> +int save_term(unsigned flags);\n\ns/unsigned/enum save_term_flags/\n\nCarlo\n"},{"id":"451301","messageId":"92a80462-c00e-acf7-7cce-095891f42226@gmail.com","threadId":"57509","inReplyTo":"CAPUEsph4Gzd4S_YNeCF+uHntGWXdcA2wY9XL=-+MZtJ0YZb-Sg@mail.gmail.com","subject":"Re: [PATCH v2 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-14T10:49:08Z","receivedAt":"2022-03-14T10:49:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/03/2022 16:52, Carlo Arenas wrote:\n> On Wed, Mar 9, 2022 at 3:04 AM Phillip Wood wrote:\n>> diff --git a/compat/terminal.h b/compat/terminal.h\n>> index 0fb9fa147c..24c4df4c0e 100644\n>> --- a/compat/terminal.h\n>> +++ b/compat/terminal.h\n>> @@ -1,14 +1,19 @@\n>> #ifndef COMPAT_TERMINAL_H\n>> #define COMPAT_TERMINAL_H\n>>\n>> +enum save_term_flags {\n>> + /* Save input and output settings */\n>> + SAVE_TERM_DUPLEX = 1 << 0,\n>> +};\n>> +\n>> /*\n>> * Save the terminal attributes so they can be restored later by a\n>> * call to restore_term(). Note that every successful call to\n>> * save_term() must be matched by a call to restore_term() even if the\n>> * attributes have not been changed. Returns 0 on success, -1 on\n>> * failure.\n>> */\n>> -int save_term(int full_duplex);\n>> +int save_term(unsigned flags);\n> \n> s/unsigned/enum save_term_flags/\n\nWell spotted!\n\nThanks\n\nPhillip\n\n> Carlo\n\n"},{"id":"451384","messageId":"20220315105723.19398-3-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220315105723.19398-1-phillip.wood123@gmail.com","subject":"[PATCH v3 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-15T10:57:21Z","receivedAt":"2022-03-15T10:58:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nread_key_without_echo() reads from stdin but uses /dev/tty when it\ndisables echo. This is unfortunate as there no guarantee that stdin is\nthe same device as /dev/tty. The perl version of \"add -p\" uses stdin\nwhen it sets the terminal mode, this commit does the same for the\nbuiltin version. There is still a difference between the perl and\nbuiltin versions though - the perl version will ignore any errors when\nsetting the terminal mode[1] and will still read single bytes when\nstdin is not a terminal. The builtin version displays a warning if\nsetting the terminal mode fails and switches to reading a line at a\ntime.\n\n[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 18 ++++++++++++------\n compat/terminal.h | 2 ++\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex da2f788137..bfbde3c1af 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n+static void close_term_fd(void)\n+{\n+\tif (term_fd)\n+\t\tclose(term_fd);\n+\tterm_fd = -1;\n+}\n+\n void restore_term(void)\n {\n \tif (term_fd < 0)\n \t\treturn;\n \n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n-\tclose(term_fd);\n-\tterm_fd = -1;\n+\tclose_term_fd();\n \tsigchain_pop_common();\n }\n \n int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n-\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n+\t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n \tif (term_fd < 0)\n \t\treturn -1;\n \tif (tcgetattr(term_fd, &old_term) < 0)\n@@ -66,8 +73,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n \n \tsigchain_pop_common();\n error:\n-\tclose(term_fd);\n-\tterm_fd = -1;\n+\tclose_term_fd();\n \treturn -1;\n }\n \n@@ -362,7 +368,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical(0) < 0) {\n+\tif (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex aeb24c9478..79ed00cf61 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -4,6 +4,8 @@\n enum save_term_flags {\n \t/* Save input and output settings */\n \tSAVE_TERM_DUPLEX = 1 << 0,\n+\t/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */\n+\tSAVE_TERM_STDIN = 1 << 1,\n };\n \n /*\n-- \n2.35.1\n\n"},{"id":"451385","messageId":"20220315105723.19398-2-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220315105723.19398-1-phillip.wood123@gmail.com","subject":"[PATCH v3 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-15T10:57:20Z","receivedAt":"2022-03-15T10:58:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe next commit will add another flag in addition to the existing\nfull_duplex so change the function signature to take a flags\nargument. Also alter the functions that call save_term() so that they\ncan pass flags down to it.\n\nThe choice to use an enum for tho bitwise flags is because gdb will\ndisplay the symbolic names of all the flags that are set rather than\nthe integer value.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 41 +++++++++++++++++++++--------------------\n compat/terminal.h | 7 ++++++-\n 2 files changed, 27 insertions(+), 21 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 3620184e79..da2f788137 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -34,7 +34,7 @@ void restore_term(void)\n \tsigchain_pop_common();\n }\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n@@ -47,11 +47,11 @@ int save_term(int full_duplex)\n \treturn 0;\n }\n \n-static int disable_bits(tcflag_t bits)\n+static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\tgoto error;\n \n \tt = old_term;\n@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)\n \treturn -1;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(enum save_term_flags flags)\n {\n-\treturn disable_bits(ECHO);\n+\treturn disable_bits(flags, ECHO);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(enum save_term_flags flags)\n {\n-\treturn disable_bits(ICANON | ECHO);\n+\treturn disable_bits(flags, ICANON | ECHO);\n }\n \n #elif defined(GIT_WINDOWS_NATIVE)\n@@ -126,15 +126,15 @@ void restore_term(void)\n \thconin = hconout = INVALID_HANDLE_VALUE;\n }\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n \t FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn -1;\n \n-\tif (full_duplex) {\n+\tif (flags & SAVE_TERM_DUPLEX) {\n \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n@@ -154,7 +154,7 @@ int save_term(int full_duplex)\n \treturn -1;\n }\n \n-static int disable_bits(DWORD bits)\n+static int disable_bits(enum save_term_flags flags, DWORD bits)\n {\n \tif (use_stty) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\treturn -1;\n \n \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)\n \treturn 0;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(enum save_term_flags flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT);\n+\treturn disable_bits(flags, ENABLE_ECHO_INPUT);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(enum save_term_flags flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n+\treturn disable_bits(flags,\n+\t\t\t ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n }\n \n /*\n@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n \t\treturn NULL;\n \t}\n \n-\tif (!echo && disable_echo()) {\n+\tif (!echo && disable_echo(0)) {\n \t\tfclose(input_fh);\n \t\tfclose(output_fh);\n \t\treturn NULL;\n@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical() < 0) {\n+\tif (warning_displayed || enable_non_canonical(0) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\n@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)\n \n #else\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n-\t/* full_duplex == 1, but no support available */\n-\treturn -full_duplex;\n+\t/* no duplex support available */\n+\treturn -!!(flags & SAVE_TERM_DUPLEX);\n }\n \n void restore_term(void)\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex 0fb9fa147c..aeb24c9478 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,14 +1,19 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+enum save_term_flags {\n+\t/* Save input and output settings */\n+\tSAVE_TERM_DUPLEX = 1 << 0,\n+};\n+\n /*\n * Save the terminal attributes so they can be restored later by a\n * call to restore_term(). Note that every successful call to\n * save_term() must be matched by a call to restore_term() even if the\n * attributes have not been changed. Returns 0 on success, -1 on\n * failure.\n */\n-int save_term(int full_duplex);\n+int save_term(enum save_term_flags flags);\n /* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n-- \n2.35.1\n\n"},{"id":"451386","messageId":"20220315105723.19398-1-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH v3 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-15T10:57:19Z","receivedAt":"2022-03-15T10:58:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThanks to Ævar, Carlo and Dscho for their comments on V2.\nChanges since V2:\n * Patch 1\n - fix save_term() prototype (thanks to Carlo)\n * Patch 2\n - avoid closing tty_fd if it is stdin\n * Patch 3\n - removed redundant #include (thanks to Ævar)\n - handle select() timeout correctly\n * Patch 4\n - reset job signals if settcattr() fails\n\nThese patches are based on a merge of pw/single-key-interactive and\nmaster. There are a couple of simple textual conflicts with\n'cb/save-term-across-editor-invocation' and unfortunately some\nsemantic conflicts as well. A diff on top of the textual resolution\nwhich handles the semantic conflicts is given after the range-diff at\nthe end of this message.\n\nV2 Cover Letter:\n\nThanks to Ramsay and Ævar for their comments on V1.\nChanges since V1:\n * Patch 1\n - use an enum for save_term()'s flags (suggested by Ævar)\n - fixed argument order in the windows code (thanks to Ramsay)\n * Patch 2\n - fixed a typo in a comment (thanks to Ramsay)\n * Patch 4\n - stopped duplicating the strings returned by gettext() (suggested by\n Ævar)\n - reworked error message handling in the signal handler to add an\n \"error: \" prefix (suggested by Ævar)\n - tweaked the background resume error message\n\nV1 Cover Letter:\n\nFix the remaining issues that I'm aware of when using the built in\n\"add -p\" with interactive.singlekey that are stopping it from being\nmerged to master. The first three patches make sure that we call\ntcsetattr() and the same file descriptor that we use for read() and\nwork around poll() being broken when reading from terminals on\nmacos. The final patch is more of an improvement rather than a bug fix\n(the same issue already exists in the perl version) and could proceed\nseparately.\n\nUnfortunately these patches conflict with\n'cb/save-term-across-editor-invocation' as well as the textual\nconflicts there is a semantic conflict as the argument to save_term()\nis changed so the code in editor.c will need updating.\n\nThese patches are based on 'pw/single-key-interactive'\n\nPhillip Wood (4):\n terminal: use flags for save_term()\n terminal: don't assume stdin is /dev/tty\n terminal: work around macos poll() bug\n terminal: restore settings on SIGTSTP\n\n compat/terminal.c | 226 +++++++++++++++++++++++++++++++++++++++-------\n compat/terminal.h | 9 +-\n 2 files changed, 202 insertions(+), 33 deletions(-)\n\nRange-diff against v2:\n1: fac5a0f5b7 ! 1: 2c4c5424d7 terminal: use flags for save_term()\n @@ compat/terminal.h\n * failure.\n */\n -int save_term(int full_duplex);\n -+int save_term(unsigned flags);\n ++int save_term(enum save_term_flags flags);\n /* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n2: bf29460ec6 ! 2: cf9275c40c terminal: don't assume stdin is /dev/tty\n @@ Commit message\n Signed-off-by: Phillip Wood \n \n ## compat/terminal.c ##\n -@@ compat/terminal.c: void restore_term(void)\n +@@ compat/terminal.c: static void restore_term_on_signal(int sig)\n + static int term_fd = -1;\n + static struct termios old_term;\n + \n ++static void close_term_fd(void)\n ++{\n ++\tif (term_fd)\n ++\t\tclose(term_fd);\n ++\tterm_fd = -1;\n ++}\n ++\n + void restore_term(void)\n + {\n + \tif (term_fd < 0)\n + \t\treturn;\n + \n + \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n +-\tclose(term_fd);\n +-\tterm_fd = -1;\n ++\tclose_term_fd();\n + \tsigchain_pop_common();\n + }\n + \n int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n @@ compat/terminal.c: void restore_term(void)\n \tif (term_fd < 0)\n \t\treturn -1;\n \tif (tcgetattr(term_fd, &old_term) < 0)\n +@@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n + \n + \tsigchain_pop_common();\n + error:\n +-\tclose(term_fd);\n +-\tterm_fd = -1;\n ++\tclose_term_fd();\n + \treturn -1;\n + }\n + \n @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n3: 1815606e82 ! 3: 4828d63ce5 terminal: work around macos poll() bug\n @@ compat/terminal.c: static int enable_non_canonical(enum save_term_flags flags)\n + * On macos it is not possible to use poll() with a terminal so use select\n + * instead.\n + */\n -+#include \n +static int getchar_with_timeout(int timeout)\n +{\n +\tstruct timeval tv, *tvp = NULL;\n @@ compat/terminal.c: static int enable_non_canonical(enum save_term_flags flags)\n +\tFD_ZERO(&readfds);\n +\tFD_SET(0, &readfds);\n +\tres = select(1, &readfds, NULL, NULL, tvp);\n -+\tif (res < 0)\n ++\tif (res <= 0)\n +\t\treturn EOF;\n +\n +\treturn getchar();\n @@ compat/terminal.c: int read_key_without_echo(struct strbuf *buf)\n -\t\t\tch = getchar();\n +\t\t\tch = getchar_with_timeout(500);\n \t\t\tif (ch == EOF)\n - \t\t\t\treturn 0;\n + \t\t\t\tbreak;\n \t\t\tstrbuf_addch(buf, ch);\n4: dd0e1fabb1 ! 4: 3185bc5223 terminal: restore settings on SIGTSTP\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +\terrno = saved_errno;\n +}\n +\n - void restore_term(void)\n - {\n - \tif (term_fd < 0)\n -@@ compat/terminal.c: void restore_term(void)\n - \tclose(term_fd);\n - \tterm_fd = -1;\n - \tsigchain_pop_common();\n ++static void reset_job_signals(void)\n ++{\n +\tif (restore_error_msg) {\n +\t\tsignal(SIGTTIN, SIG_DFL);\n +\t\tsignal(SIGTTOU, SIG_DFL);\n +\t\tsignal(SIGTSTP, SIG_DFL);\n +\t\trestore_error_msg = NULL;\n +\t\tbackground_resume_msg = NULL;\n +\t}\n ++}\n ++\n + static void close_term_fd(void)\n + {\n + \tif (term_fd)\n +@@ compat/terminal.c: void restore_term(void)\n + \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n + \tclose_term_fd();\n + \tsigchain_pop_common();\n ++\treset_job_signals();\n }\n \n int save_term(enum save_term_flags flags)\n @@ compat/terminal.c: int save_term(enum save_term_flags flags)\n \n \treturn 0;\n }\n +@@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n + \t\treturn 0;\n + \n + \tsigchain_pop_common();\n ++\treset_job_signals();\n + error:\n + \tclose_term_fd();\n + \treturn -1;\n @@ compat/terminal.c: static int getchar_with_timeout(int timeout)\n \tfd_set readfds;\n \tint res;\n @@ compat/terminal.c: static int getchar_with_timeout(int timeout)\n \tFD_ZERO(&readfds);\n \tFD_SET(0, &readfds);\n \tres = select(1, &readfds, NULL, NULL, tvp);\n --\tif (res < 0)\n --\t\treturn EOF;\n +-\tif (res <= 0)\n ++\tif (!res)\n + \t\treturn EOF;\n -\n +\tif (res < 0) {\n +\t\tif (errno == EINTR)\n\nSemantic conflict resolution:\n---- >8 ----\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex bffb1340ab..9a64b9a431 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -150,9 +150,8 @@ int save_term(enum save_term_flags flags)\n : open(\"/dev/tty\", O_RDWR);\n if (term_fd < 0)\n return -1;\n- if (full_duplex && !is_controlling_terminal(term_fd)) {\n- close(term_fd);\n- term_fd = -1;\n+ if (flags & SAVE_TERM_DUPLEX && !is_controlling_terminal(term_fd)) {\n+ close_term_fd();\n return -1;\n }\n if (tcgetattr(term_fd, &old_term) < 0)\ndiff --git a/editor.c b/editor.c\nindex 6c5c95e6a2..192d6ea75d 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -55,7 +55,7 @@ static int prepare_term(const char *editor)\n\n git_config_get_bool(\"editor.stty\", &need_saverestore);\n if (need_saverestore)\n- return save_term(1);\n+ return save_term(SAVE_TERM_DUPLEX);\n return 0;\n }\n\n-- \n2.35.1\n\n"},{"id":"451387","messageId":"20220315105723.19398-5-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220315105723.19398-1-phillip.wood123@gmail.com","subject":"[PATCH v3 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-15T10:57:23Z","receivedAt":"2022-03-15T10:58:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf the user suspends git while it is waiting for a keypress reset the\nterminal before stopping and restore the settings when git resumes. If\nthe user tries to resume in the background print an error\nmessage (taking care to use async safe functions) before stopping\nagain. Ideally we would reprint the prompt for the user when git\nresumes but this patch just restarts the read().\n\nThe signal handler is established with sigaction() rather than using\nsigchain_push() as this allows us to control the signal mask when the\nhandler is invoked and ensure SA_RESTART is used to restart the\nread() when resuming.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 131 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 128 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 89f326adc1..45a9084bb3 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -1,4 +1,4 @@\n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"compat/terminal.h\"\n #include \"sigchain.h\"\n #include \"strbuf.h\"\n@@ -23,6 +23,101 @@ static void restore_term_on_signal(int sig)\n static int term_fd = -1;\n static struct termios old_term;\n \n+static const char *background_resume_msg;\n+static const char *restore_error_msg;\n+static volatile sig_atomic_t ttou_received;\n+\n+static void write_err(const char *msg)\n+{\n+\twrite_in_full(2, \"error: \", strlen(\"error: \"));\n+\twrite_in_full(2, msg, strlen(msg));\n+\twrite_in_full(2, \"\\n\", 1);\n+}\n+\n+static void print_background_resume_msg(int signo)\n+{\n+\tint saved_errno = errno;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\n+\tttou_received = 1;\n+\twrite_err(background_resume_msg);\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\terrno = saved_errno;\n+}\n+\n+static void restore_terminal_on_suspend(int signo)\n+{\n+\tint saved_errno = errno;\n+\tint res;\n+\tstruct termios t;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\tint can_restore = 1;\n+\n+\tif (tcgetattr(term_fd, &t) < 0)\n+\t\tcan_restore = 0;\n+\n+\tif (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)\n+\t\twrite_err(restore_error_msg);\n+\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\tif (!can_restore) {\n+\t\twrite_err(restore_error_msg);\n+\t\tgoto out;\n+\t}\n+\t/*\n+\t * If we resume in the background then we receive SIGTTOU when calling\n+\t * tcsetattr() below. Set up a handler to print an error message in that\n+\t * case.\n+\t */\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, SIGTTOU);\n+\tsa.sa_mask = old_sa.sa_mask;\n+\tsa.sa_handler = print_background_resume_msg;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGTTOU, &sa, &old_sa);\n+ again:\n+\tttou_received = 0;\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\tres = tcsetattr(term_fd, TCSAFLUSH, &t);\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tif (ttou_received)\n+\t\tgoto again;\n+\telse if (res < 0)\n+\t\twrite_err(restore_error_msg);\n+\tsigaction(SIGTTOU, &old_sa, NULL);\n+ out:\n+\terrno = saved_errno;\n+}\n+\n+static void reset_job_signals(void)\n+{\n+\tif (restore_error_msg) {\n+\t\tsignal(SIGTTIN, SIG_DFL);\n+\t\tsignal(SIGTTOU, SIG_DFL);\n+\t\tsignal(SIGTSTP, SIG_DFL);\n+\t\trestore_error_msg = NULL;\n+\t\tbackground_resume_msg = NULL;\n+\t}\n+}\n+\n static void close_term_fd(void)\n {\n \tif (term_fd)\n@@ -38,10 +133,13 @@ void restore_term(void)\n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n \tclose_term_fd();\n \tsigchain_pop_common();\n+\treset_job_signals();\n }\n \n int save_term(enum save_term_flags flags)\n {\n+\tstruct sigaction sa;\n+\n \tif (term_fd < 0)\n \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n@@ -50,6 +148,26 @@ int save_term(enum save_term_flags flags)\n \tif (tcgetattr(term_fd, &old_term) < 0)\n \t\treturn -1;\n \tsigchain_push_common(restore_term_on_signal);\n+\t/*\n+\t * If job control is disabled then the shell will have set the\n+\t * disposition of SIGTSTP to SIG_IGN.\n+\t */\n+\tsigaction(SIGTSTP, NULL, &sa);\n+\tif (sa.sa_handler == SIG_IGN)\n+\t\treturn 0;\n+\n+\t/* avoid calling gettext() from signal handler */\n+\tbackground_resume_msg = _(\"cannot resume in the background, please use 'fg' to resume\");\n+\trestore_error_msg = _(\"cannot restore terminal settings\");\n+\tsa.sa_handler = restore_terminal_on_suspend;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigemptyset(&sa.sa_mask);\n+\tsigaddset(&sa.sa_mask, SIGTSTP);\n+\tsigaddset(&sa.sa_mask, SIGTTIN);\n+\tsigaddset(&sa.sa_mask, SIGTTOU);\n+\tsigaction(SIGTSTP, &sa, NULL);\n+\tsigaction(SIGTTIN, &sa, NULL);\n+\tsigaction(SIGTTOU, &sa, NULL);\n \n \treturn 0;\n }\n@@ -72,6 +190,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n \t\treturn 0;\n \n \tsigchain_pop_common();\n+\treset_job_signals();\n error:\n \tclose_term_fd();\n \treturn -1;\n@@ -97,6 +216,7 @@ static int getchar_with_timeout(int timeout)\n \tfd_set readfds;\n \tint res;\n \n+ again:\n \tif (timeout >= 0) {\n \t\ttv.tv_sec = timeout / 1000;\n \t\ttv.tv_usec = (timeout % 1000) * 1000;\n@@ -106,9 +226,14 @@ static int getchar_with_timeout(int timeout)\n \tFD_ZERO(&readfds);\n \tFD_SET(0, &readfds);\n \tres = select(1, &readfds, NULL, NULL, tvp);\n-\tif (res <= 0)\n+\tif (!res)\n \t\treturn EOF;\n-\n+\tif (res < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tgoto again;\n+\t\telse\n+\t\t\treturn EOF;\n+\t}\n \treturn getchar();\n }\n \n-- \n2.35.1\n\n"},{"id":"451388","messageId":"20220315105723.19398-4-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220315105723.19398-1-phillip.wood123@gmail.com","subject":"[PATCH v3 3/4] terminal: work around macos poll() bug","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-15T10:57:22Z","receivedAt":"2022-03-15T10:58:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nOn macos the builtin \"add -p\" does not handle keys that generate\nescape sequences because poll() does not work with terminals\nthere. Switch to using select() on non-windows platforms to work\naround this.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 42 ++++++++++++++++++++++++++++++++++++------\n 1 file changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex bfbde3c1af..89f326adc1 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -87,6 +87,31 @@ static int enable_non_canonical(enum save_term_flags flags)\n \treturn disable_bits(flags, ICANON | ECHO);\n }\n \n+/*\n+ * On macos it is not possible to use poll() with a terminal so use select\n+ * instead.\n+ */\n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct timeval tv, *tvp = NULL;\n+\tfd_set readfds;\n+\tint res;\n+\n+\tif (timeout >= 0) {\n+\t\ttv.tv_sec = timeout / 1000;\n+\t\ttv.tv_usec = (timeout % 1000) * 1000;\n+\t\ttvp = &tv;\n+\t}\n+\n+\tFD_ZERO(&readfds);\n+\tFD_SET(0, &readfds);\n+\tres = select(1, &readfds, NULL, NULL, tvp);\n+\tif (res <= 0)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #elif defined(GIT_WINDOWS_NATIVE)\n \n #define INPUT_PATH \"CONIN$\"\n@@ -252,6 +277,16 @@ static int mingw_getchar(void)\n }\n #define getchar mingw_getchar\n \n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n+\n+\tif (poll(&pfd, 1, timeout) < 1)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #endif\n \n #ifndef FORCE_TEXT\n@@ -402,12 +437,7 @@ int read_key_without_echo(struct strbuf *buf)\n \t\t * half a second when we know that the sequence is complete.\n \t\t */\n \t\twhile (!is_known_escape_sequence(buf->buf)) {\n-\t\t\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n-\n-\t\t\tif (poll(&pfd, 1, 500) < 1)\n-\t\t\t\tbreak;\n-\n-\t\t\tch = getchar();\n+\t\t\tch = getchar_with_timeout(500);\n \t\t\tif (ch == EOF)\n \t\t\t\tbreak;\n \t\t\tstrbuf_addch(buf, ch);\n-- \n2.35.1\n\n"},{"id":"451405","messageId":"xmqqzglrdsd8.fsf@gitster.g","threadId":"57509","inReplyTo":"20220315105723.19398-3-phillip.wood123@gmail.com","subject":"Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-15T17:42:59Z","receivedAt":"2022-03-15T17:43:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index da2f788137..bfbde3c1af 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)\n> static int term_fd = -1;\n\nThe variable can be -1 to signal \"no valid file descriptor\".\n\n> static struct termios old_term;\n> \n> +static void close_term_fd(void)\n> +{\n> +\tif (term_fd)\n> +\t\tclose(term_fd);\n> +\tterm_fd = -1;\n> +}\n> +\n\nAnd we use that negative value after closing it.\n\nThe check before closing it is wrong. It should be\n\n\tif (0 <= term_fd)\n\ninstead. Or we could mimick the beginning of restore_term(), i.e.\n\n\tif (term_fd < 0)\n\t\treturn;\n\tclose(term_fd);\n\tterm_fd = -1;\n\n> void restore_term(void)\n> {\n> \tif (term_fd < 0)\n> \t\treturn;\n> \n> \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n> -\tclose(term_fd);\n> -\tterm_fd = -1;\n> +\tclose_term_fd();\n\nBecause we come this far only when term_fd is valid, this change is\na no-op. If we are adding more calls to close_term_fd(), it may be\na good abstraction.\n\n> \tsigchain_pop_common();\n> }\n> \n> int save_term(enum save_term_flags flags)\n> {\n> \tif (term_fd < 0)\n> -\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n> +\t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n> +\t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n\nWe can avoid overly long line by wrapping differently:\n\n\t\tterm_fd = ((flags & SAVE_TERM_STDIN)\n\t\t\t ? 0\n\t\t\t : open(\"/dev/tty\", O_RDWR));\n\nWe can turn our head sideways to see the parse tree this way.\n\n> @@ -66,8 +73,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n> \n> \tsigchain_pop_common();\n> error:\n> -\tclose(term_fd);\n> -\tterm_fd = -1;\n> +\tclose_term_fd();\n\nOK.\n\n> @@ -362,7 +368,7 @@ int read_key_without_echo(struct strbuf *buf)\n> \tstatic int warning_displayed;\n> \tint ch;\n> \n> -\tif (warning_displayed || enable_non_canonical(0) < 0) {\n> +\tif (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {\n> \t\tif (!warning_displayed) {\n> \t\t\twarning(\"reading single keystrokes not supported on \"\n> \t\t\t\t\"this platform; reading line instead\");\n\nThe validity of this change is harder to see with only what we have\nin the patch, but the control flow is\n\n enable_non_canonical(bits)\n -> disable_bits(bits, ...)\n -> save_term(bits)\n\nAnd we are passing SAVE_TERM_STDIN to say \"reuse fd #0 and save the\nterminal settings of it, instead of opening /dev/tty anew\".\n\nWhat happens if FD#0 is *not* connected to a tty, by the way?\ntcgetattr() in save_term() would fail, without clearing term_fd\n(i.e. term_fd is still 0 when save_term() returns a failure), and\ngoes to the error code path, where close_term_fd() is called.\n\nBecause we have the \"if (term_fd)\" bug in save_term(), this bug is\nhidden, but I think save_term() upon seeing a failure from tcgetattr()\nshould clear term_fd to limit the damage, especially when it is trying\nto futz with caller supplied FD#0, not the tty it opened itself?\n\n> diff --git a/compat/terminal.h b/compat/terminal.h\n> index aeb24c9478..79ed00cf61 100644\n> --- a/compat/terminal.h\n> +++ b/compat/terminal.h\n> @@ -4,6 +4,8 @@\n> enum save_term_flags {\n> \t/* Save input and output settings */\n> \tSAVE_TERM_DUPLEX = 1 << 0,\n> +\t/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */\n> +\tSAVE_TERM_STDIN = 1 << 1,\n> };\n> \n> /*\n"},{"id":"451406","messageId":"xmqqr173drza.fsf@gitster.g","threadId":"57509","inReplyTo":"20220315105723.19398-5-phillip.wood123@gmail.com","subject":"Re: [PATCH v3 4/4] terminal: restore settings on SIGTSTP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-15T17:51:21Z","receivedAt":"2022-03-15T17:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> @@ -23,6 +23,101 @@ static void restore_term_on_signal(int sig)\n> static int term_fd = -1;\n> static struct termios old_term;\n> \n> +static const char *background_resume_msg;\n> +static const char *restore_error_msg;\n> +static volatile sig_atomic_t ttou_received;\n\nIt is a good idea to have a comment here to say why we had to\nreinvent a subset of error(), instead of forcing curious readers to\n\"git blame\" the log message for this commit (I am assuming that\n\"this is called from a signal handler and uses only functions that\nare safe in that context\" is the reason).\n\n> +static void write_err(const char *msg)\n> +{\n> +\twrite_in_full(2, \"error: \", strlen(\"error: \"));\n> +\twrite_in_full(2, msg, strlen(msg));\n> +\twrite_in_full(2, \"\\n\", 1);\n> +}\n> +\n> +static void print_background_resume_msg(int signo)\n> +{\n> +\tint saved_errno = errno;\n> +\tsigset_t mask;\n> +\tstruct sigaction old_sa;\n> +\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n> +\n> +\tttou_received = 1;\n> +\twrite_err(background_resume_msg);\n> +\tsigaction(signo, &sa, &old_sa);\n> +\traise(signo);\n> +\tsigemptyset(&mask);\n> +\tsigaddset(&mask, signo);\n> +\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n> +\t/* Stopped here */\n> +\tsigprocmask(SIG_BLOCK, &mask, NULL);\n> +\tsigaction(signo, &old_sa, NULL);\n> +\terrno = saved_errno;\n> +}\n> ...\n> +\t/* avoid calling gettext() from signal handler */\n> +\tbackground_resume_msg = _(\"cannot resume in the background, please use 'fg' to resume\");\n> +\trestore_error_msg = _(\"cannot restore terminal settings\");\n\nNice to see such an attention to detail here.\n\nThanks.\n"},{"id":"451408","messageId":"02fd01d83896$c0261240$407236c0$@nexbridge.com","threadId":"57509","inReplyTo":"xmqqzglrdsd8.fsf@gitster.g","subject":"RE: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-03-15T18:01:46Z","receivedAt":"2022-03-15T18:02:05Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On March 15, 2022 1:43 PM, Junio C Hamano wrote:\n>Phillip Wood writes:\n>\n>> diff --git a/compat/terminal.c b/compat/terminal.c index\n>> da2f788137..bfbde3c1af 100644\n>> --- a/compat/terminal.c\n>> +++ b/compat/terminal.c\n>> @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)\n>> static int term_fd = -1;\n>\n>The variable can be -1 to signal \"no valid file descriptor\".\n>\n>> static struct termios old_term;\n>>\n>> +static void close_term_fd(void)\n>> +{\n>> +\tif (term_fd)\n>> +\t\tclose(term_fd);\n>> +\tterm_fd = -1;\n>> +}\n>> +\n>\n>And we use that negative value after closing it.\n>\n>The check before closing it is wrong. It should be\n>\n>\tif (0 <= term_fd)\n\nShould this expression succeed if term_fd == stdin? I might be missing the point here.\n--Randall\n\n"},{"id":"451414","messageId":"xmqqilsfdok0.fsf@gitster.g","threadId":"57509","inReplyTo":"02fd01d83896$c0261240$407236c0$@nexbridge.com","subject":"Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-15T19:05:19Z","receivedAt":"2022-03-15T19:06:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n>>The check before closing it is wrong. It should be\n>>\n>>\tif (0 <= term_fd)\n>\n> Should this expression succeed if term_fd == stdin? I might be missing the point here.\n\nWe could use \"if (0 < term_fd)\" to make this guard both about\navoiding to call close() on an uninitialized FD and also about\navoiding to close standard input. I'd prefer to see them handled\nseparately as these live at different conceptual levels\n(i.e. closing -1 is a no-no no matter what, closing 0 is bad if it\nis what we did not open but what the caller supplied us via the\nSAVE_TERM_STDIN bit, but it may be warranted if it was what we\nobtained from an earlier call to open(\"/dev/tty\") we did ourselves).\n\n\n\n"},{"id":"451421","messageId":"031801d838a4$4d3c7770$e7b56650$@nexbridge.com","threadId":"57509","inReplyTo":"xmqqilsfdok0.fsf@gitster.g","subject":"RE: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-03-15T19:38:46Z","receivedAt":"2022-03-15T19:38:57Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On March 15, 2022 3:05 PM, Junio C Hamano wrote:\n>To: rsbecker@nexbridge.com\n>Cc: 'Phillip Wood' ; 'Git Mailing List'\n>; 'Phillip Wood' ; 'Ævar\n>Arnfjörð Bjarmason' ; 'Carlo Arenas' ;\n>'Johannes Schindelin' ; 'Ramsay Jones'\n>\n>Subject: Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty\n>\n> writes:\n>\n>>>The check before closing it is wrong. It should be\n>>>\n>>>\tif (0 <= term_fd)\n>>\n>> Should this expression succeed if term_fd == stdin? I might be missing the point\n>here.\n>\n>We could use \"if (0 < term_fd)\" to make this guard both about avoiding to call\n>close() on an uninitialized FD and also about avoiding to close standard input. I'd\n>prefer to see them handled separately as these live at different conceptual levels\n>(i.e. closing -1 is a no-no no matter what, closing 0 is bad if it is what we did not\n>open but what the caller supplied us via the SAVE_TERM_STDIN bit, but it may be\n>warranted if it was what we obtained from an earlier call to open(\"/dev/tty\") we\n>did ourselves).\n\nThanks. This is one of those situations where explaining magic numbers is important. I appreciate the insight.\n\nRegards,\nRandall\n\n"},{"id":"451497","messageId":"20220316185405.29551-1-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220304131126.8293-1-phillip.wood123@gmail.com","subject":"[PATCH v4 0/4] builtin add -p: hopefully final readkey fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-16T18:54:01Z","receivedAt":"2022-03-16T18:54:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThank to Junio for his comments on V3.\nChanges since V3:\n * Patch 2\n - only close tty_fd if it is open and SAVE_TERM_STDIN was not\n requested\n - close tty_fd in save_term() if there is an error.\n - folded long line\n * Patch 4\n - add a comment explaining the purpose of write_err()\n\nThere's an updated conflict resolution after the range-diff at the end\nof this message.\n\nV3 Cover Letter:\n\nThanks to Ævar, Carlo and Dscho for their comments on V2.\nChanges since V2:\n * Patch 1\n - fix save_term() prototype (thanks to Carlo)\n * Patch 2\n - avoid closing tty_fd if it is stdin\n * Patch 3\n - removed redundant #include (thanks to Ævar)\n - handle select() timeout correctly\n * Patch 4\n - reset job signals if settcattr() fails\n\nThese patches are based on a merge of pw/single-key-interactive and\nmaster. There are a couple of simple textual conflicts with\n'cb/save-term-across-editor-invocation' and unfortunately some\nsemantic conflicts as well. A diff on top of the textual resolution\nwhich handles the semantic conflicts is given after the range-diff at\nthe end of this message.\n\nV2 Cover Letter:\n\nThanks to Ramsay and Ævar for their comments on V1.\nChanges since V1:\n * Patch 1\n - use an enum for save_term()'s flags (suggested by Ævar)\n - fixed argument order in the windows code (thanks to Ramsay)\n * Patch 2\n - fixed a typo in a comment (thanks to Ramsay)\n * Patch 4\n - stopped duplicating the strings returned by gettext() (suggested by\n Ævar)\n - reworked error message handling in the signal handler to add an\n \"error: \" prefix (suggested by Ævar)\n - tweaked the background resume error message\n\nV1 Cover Letter:\n\nFix the remaining issues that I'm aware of when using the built in\n\"add -p\" with interactive.singlekey that are stopping it from being\nmerged to master. The first three patches make sure that we call\ntcsetattr() and the same file descriptor that we use for read() and\nwork around poll() being broken when reading from terminals on\nmacos. The final patch is more of an improvement rather than a bug fix\n(the same issue already exists in the perl version) and could proceed\nseparately.\n\nUnfortunately these patches conflict with\n'cb/save-term-across-editor-invocation' as well as the textual\nconflicts there is a semantic conflict as the argument to save_term()\nis changed so the code in editor.c will need updating.\n\nThese patches are based on 'pw/single-key-interactive'\n\n\nPhillip Wood (4):\n terminal: use flags for save_term()\n terminal: don't assume stdin is /dev/tty\n terminal: work around macos poll() bug\n terminal: restore settings on SIGTSTP\n\n compat/terminal.c | 238 +++++++++++++++++++++++++++++++++++++++-------\n compat/terminal.h | 9 +-\n 2 files changed, 211 insertions(+), 36 deletions(-)\n\nRange-diff against v3:\n-: ---------- > 1: 2c4c5424d7 terminal: use flags for save_term()\n1: cf9275c40c ! 2: 405a69852f terminal: don't assume stdin is /dev/tty\n @@ Commit message\n \n ## compat/terminal.c ##\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n + #define INPUT_PATH \"/dev/tty\"\n + #define OUTPUT_PATH \"/dev/tty\"\n + \n ++static volatile sig_atomic_t term_fd_needs_closing;\n static int term_fd = -1;\n static struct termios old_term;\n \n +static void close_term_fd(void)\n +{\n -+\tif (term_fd)\n ++\tif (term_fd_needs_closing)\n +\t\tclose(term_fd);\n ++\tterm_fd_needs_closing = 0;\n +\tterm_fd = -1;\n +}\n +\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n {\n \tif (term_fd < 0)\n -\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n -+\t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n -+\t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n ++\t\tterm_fd = ((flags & SAVE_TERM_STDIN)\n ++\t\t\t ? 0\n ++\t\t\t : open(\"/dev/tty\", O_RDWR));\n \tif (term_fd < 0)\n \t\treturn -1;\n - \tif (tcgetattr(term_fd, &old_term) < 0)\n +-\tif (tcgetattr(term_fd, &old_term) < 0)\n ++\tterm_fd_needs_closing = !(flags & SAVE_TERM_STDIN);\n ++\tif (tcgetattr(term_fd, &old_term) < 0) {\n ++\t\tclose_term_fd();\n + \t\treturn -1;\n ++\t}\n + \tsigchain_push_common(restore_term_on_signal);\n + \n + \treturn 0;\n @@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n + \tstruct termios t;\n + \n + \tif (save_term(flags) < 0)\n +-\t\tgoto error;\n ++\t\treturn -1;\n + \n + \tt = old_term;\n + \n +@@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n + \t\treturn 0;\n \n \tsigchain_pop_common();\n - error:\n +-error:\n -\tclose(term_fd);\n -\tterm_fd = -1;\n +\tclose_term_fd();\n2: 4828d63ce5 = 3: 70bc6e9c06 terminal: work around macos poll() bug\n3: 3185bc5223 ! 4: f38d162f75 terminal: restore settings on SIGTSTP\n @@ compat/terminal.c\n #include \"compat/terminal.h\"\n #include \"sigchain.h\"\n #include \"strbuf.h\"\n -@@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +@@ compat/terminal.c: static volatile sig_atomic_t term_fd_needs_closing;\n static int term_fd = -1;\n static struct termios old_term;\n \n +static const char *background_resume_msg;\n +static const char *restore_error_msg;\n +static volatile sig_atomic_t ttou_received;\n +\n ++/* async safe error function for use by signal handlers. */\n +static void write_err(const char *msg)\n +{\n +\twrite_in_full(2, \"error: \", strlen(\"error: \"));\n @@ compat/terminal.c: static void restore_term_on_signal(int sig)\n +\n static void close_term_fd(void)\n {\n - \tif (term_fd)\n + \tif (term_fd_needs_closing)\n @@ compat/terminal.c: void restore_term(void)\n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n \tclose_term_fd();\n @@ compat/terminal.c: void restore_term(void)\n +\tstruct sigaction sa;\n +\n \tif (term_fd < 0)\n - \t\tterm_fd = (flags & SAVE_TERM_STDIN) ? 0\n - \t\t\t\t\t\t : open(\"/dev/tty\", O_RDWR);\n + \t\tterm_fd = ((flags & SAVE_TERM_STDIN)\n + \t\t\t ? 0\n @@ compat/terminal.c: int save_term(enum save_term_flags flags)\n - \tif (tcgetattr(term_fd, &old_term) < 0)\n \t\treturn -1;\n + \t}\n \tsigchain_push_common(restore_term_on_signal);\n +\t/*\n +\t * If job control is disabled then the shell will have set the\n @@ compat/terminal.c: static int disable_bits(enum save_term_flags flags, tcflag_t\n \n \tsigchain_pop_common();\n +\treset_job_signals();\n - error:\n \tclose_term_fd();\n \treturn -1;\n + }\n @@ compat/terminal.c: static int getchar_with_timeout(int timeout)\n \tfd_set readfds;\n \tint res;\n\nConflict Resolution:\ndiff --cc compat/terminal.c\nindex 7db330c52d,8743df6c9a..1bfa92584a\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@@ -134,47 -29,31 +134,56 @@@ void restore_term(void\n \t\treturn;\n\n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n -\n -\tclose(term_fd);\n -\tterm_fd = -1;\n +\tclose_term_fd();\n \tsigchain_pop_common();\n +\treset_job_signals();\n }\n\n+ static int is_controlling_terminal(int fd)\n+ {\n+ \treturn (getpgid(0) == tcgetpgrp(fd));\n+ }\n+\n -int save_term(int full_duplex)\n +int save_term(enum save_term_flags flags)\n {\n +\tstruct sigaction sa;\n +\n \tif (term_fd < 0)\n -\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n +\t\tterm_fd = ((flags & SAVE_TERM_STDIN)\n +\t\t\t ? 0\n +\t\t\t : open(\"/dev/tty\", O_RDWR));\n \tif (term_fd < 0)\n \t\treturn -1;\n -\tif (full_duplex && !is_controlling_terminal(term_fd)) {\n -\t\tclose(term_fd);\n -\t\tterm_fd = -1;\n +\tterm_fd_needs_closing = !(flags & SAVE_TERM_STDIN);\n++\tif ((flags & SAVE_TERM_DUPLEX) && !is_controlling_terminal(term_fd)) {\n++\t\tclose_term_fd();\n+ \t\treturn -1;\n+ \t}\n -\tif (tcgetattr(term_fd, &old_term) < 0)\n +\tif (tcgetattr(term_fd, &old_term) < 0) {\n +\t\tclose_term_fd();\n \t\treturn -1;\n +\t}\n \tsigchain_push_common(restore_term_on_signal);\n +\t/*\n +\t * If job control is disabled then the shell will have set the\n +\t * disposition of SIGTSTP to SIG_IGN.\n +\t */\n +\tsigaction(SIGTSTP, NULL, &sa);\n +\tif (sa.sa_handler == SIG_IGN)\n +\t\treturn 0;\n +\n +\t/* avoid calling gettext() from signal handler */\n +\tbackground_resume_msg = _(\"cannot resume in the background, please use 'fg' to resume\");\n +\trestore_error_msg = _(\"cannot restore terminal settings\");\n +\tsa.sa_handler = restore_terminal_on_suspend;\n +\tsa.sa_flags = SA_RESTART;\n +\tsigemptyset(&sa.sa_mask);\n +\tsigaddset(&sa.sa_mask, SIGTSTP);\n +\tsigaddset(&sa.sa_mask, SIGTTIN);\n +\tsigaddset(&sa.sa_mask, SIGTTOU);\n +\tsigaction(SIGTSTP, &sa, NULL);\n +\tsigaction(SIGTTIN, &sa, NULL);\n +\tsigaction(SIGTTOU, &sa, NULL);\n\n \treturn 0;\n }\ndiff --cc editor.c\nindex 8b9648281d,6c5c95e6a2..192d6ea75d\n--- a/editor.c\n+++ b/editor.c\n@@@ -48,6 -49,16 +49,16 @@@ const char *git_sequence_editor(void\n \treturn editor;\n }\n\n+ static int prepare_term(const char *editor)\n+ {\n+ \tint need_saverestore = !strcmp(editor, DEFAULT_EDITOR);\n+\n+ \tgit_config_get_bool(\"editor.stty\", &need_saverestore);\n+ \tif (need_saverestore)\n -\t\treturn save_term(1);\n++\t\treturn save_term(SAVE_TERM_DUPLEX);\n+ \treturn 0;\n+ }\n+\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t struct strbuf *buffer, const char *const *env)\n {\n-- \n2.35.1\n\n"},{"id":"451498","messageId":"20220316185405.29551-2-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220316185405.29551-1-phillip.wood123@gmail.com","subject":"[PATCH v4 1/4] terminal: use flags for save_term()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-16T18:54:02Z","receivedAt":"2022-03-16T18:54:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe next commit will add another flag in addition to the existing\nfull_duplex so change the function signature to take a flags\nargument. Also alter the functions that call save_term() so that they\ncan pass flags down to it.\n\nThe choice to use an enum for tho bitwise flags is because gdb will\ndisplay the symbolic names of all the flags that are set rather than\nthe integer value.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 41 +++++++++++++++++++++--------------------\n compat/terminal.h | 7 ++++++-\n 2 files changed, 27 insertions(+), 21 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 3620184e79..da2f788137 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -34,7 +34,7 @@ void restore_term(void)\n \tsigchain_pop_common();\n }\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n@@ -47,11 +47,11 @@ int save_term(int full_duplex)\n \treturn 0;\n }\n \n-static int disable_bits(tcflag_t bits)\n+static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n {\n \tstruct termios t;\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\tgoto error;\n \n \tt = old_term;\n@@ -71,14 +71,14 @@ static int disable_bits(tcflag_t bits)\n \treturn -1;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(enum save_term_flags flags)\n {\n-\treturn disable_bits(ECHO);\n+\treturn disable_bits(flags, ECHO);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(enum save_term_flags flags)\n {\n-\treturn disable_bits(ICANON | ECHO);\n+\treturn disable_bits(flags, ICANON | ECHO);\n }\n \n #elif defined(GIT_WINDOWS_NATIVE)\n@@ -126,15 +126,15 @@ void restore_term(void)\n \thconin = hconout = INVALID_HANDLE_VALUE;\n }\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n \thconin = CreateFileA(\"CONIN$\", GENERIC_READ | GENERIC_WRITE,\n \t FILE_SHARE_READ, NULL, OPEN_EXISTING,\n \t FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn -1;\n \n-\tif (full_duplex) {\n+\tif (flags & SAVE_TERM_DUPLEX) {\n \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n@@ -154,7 +154,7 @@ int save_term(int full_duplex)\n \treturn -1;\n }\n \n-static int disable_bits(DWORD bits)\n+static int disable_bits(enum save_term_flags flags, DWORD bits)\n {\n \tif (use_stty) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -191,7 +191,7 @@ static int disable_bits(DWORD bits)\n \t\tuse_stty = 0;\n \t}\n \n-\tif (save_term(0) < 0)\n+\tif (save_term(flags) < 0)\n \t\treturn -1;\n \n \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n@@ -204,14 +204,15 @@ static int disable_bits(DWORD bits)\n \treturn 0;\n }\n \n-static int disable_echo(void)\n+static int disable_echo(enum save_term_flags flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT);\n+\treturn disable_bits(flags, ENABLE_ECHO_INPUT);\n }\n \n-static int enable_non_canonical(void)\n+static int enable_non_canonical(enum save_term_flags flags)\n {\n-\treturn disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n+\treturn disable_bits(flags,\n+\t\t\t ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT);\n }\n \n /*\n@@ -267,7 +268,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n \t\treturn NULL;\n \t}\n \n-\tif (!echo && disable_echo()) {\n+\tif (!echo && disable_echo(0)) {\n \t\tfclose(input_fh);\n \t\tfclose(output_fh);\n \t\treturn NULL;\n@@ -361,7 +362,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical() < 0) {\n+\tif (warning_displayed || enable_non_canonical(0) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\n@@ -413,10 +414,10 @@ int read_key_without_echo(struct strbuf *buf)\n \n #else\n \n-int save_term(int full_duplex)\n+int save_term(enum save_term_flags flags)\n {\n-\t/* full_duplex == 1, but no support available */\n-\treturn -full_duplex;\n+\t/* no duplex support available */\n+\treturn -!!(flags & SAVE_TERM_DUPLEX);\n }\n \n void restore_term(void)\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex 0fb9fa147c..aeb24c9478 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,14 +1,19 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+enum save_term_flags {\n+\t/* Save input and output settings */\n+\tSAVE_TERM_DUPLEX = 1 << 0,\n+};\n+\n /*\n * Save the terminal attributes so they can be restored later by a\n * call to restore_term(). Note that every successful call to\n * save_term() must be matched by a call to restore_term() even if the\n * attributes have not been changed. Returns 0 on success, -1 on\n * failure.\n */\n-int save_term(int full_duplex);\n+int save_term(enum save_term_flags flags);\n /* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n-- \n2.35.1\n\n"},{"id":"451499","messageId":"20220316185405.29551-3-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220316185405.29551-1-phillip.wood123@gmail.com","subject":"[PATCH v4 2/4] terminal: don't assume stdin is /dev/tty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-16T18:54:03Z","receivedAt":"2022-03-16T18:54:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nread_key_without_echo() reads from stdin but uses /dev/tty when it\ndisables echo. This is unfortunate as there no guarantee that stdin is\nthe same device as /dev/tty. The perl version of \"add -p\" uses stdin\nwhen it sets the terminal mode, this commit does the same for the\nbuiltin version. There is still a difference between the perl and\nbuiltin versions though - the perl version will ignore any errors when\nsetting the terminal mode[1] and will still read single bytes when\nstdin is not a terminal. The builtin version displays a warning if\nsetting the terminal mode fails and switches to reading a line at a\ntime.\n\n[1] https://github.com/jonathanstowe/TermReadKey/blob/b061c913bbf7ff9bad9b4eea6caae189eacd6063/ReadKey.xs#L1090\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 29 ++++++++++++++++++++---------\n compat/terminal.h | 2 ++\n 2 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex da2f788137..4893294eb6 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -20,28 +20,41 @@ static void restore_term_on_signal(int sig)\n #define INPUT_PATH \"/dev/tty\"\n #define OUTPUT_PATH \"/dev/tty\"\n \n+static volatile sig_atomic_t term_fd_needs_closing;\n static int term_fd = -1;\n static struct termios old_term;\n \n+static void close_term_fd(void)\n+{\n+\tif (term_fd_needs_closing)\n+\t\tclose(term_fd);\n+\tterm_fd_needs_closing = 0;\n+\tterm_fd = -1;\n+}\n+\n void restore_term(void)\n {\n \tif (term_fd < 0)\n \t\treturn;\n \n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n-\tclose(term_fd);\n-\tterm_fd = -1;\n+\tclose_term_fd();\n \tsigchain_pop_common();\n }\n \n int save_term(enum save_term_flags flags)\n {\n \tif (term_fd < 0)\n-\t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\t\tterm_fd = ((flags & SAVE_TERM_STDIN)\n+\t\t\t ? 0\n+\t\t\t : open(\"/dev/tty\", O_RDWR));\n \tif (term_fd < 0)\n \t\treturn -1;\n-\tif (tcgetattr(term_fd, &old_term) < 0)\n+\tterm_fd_needs_closing = !(flags & SAVE_TERM_STDIN);\n+\tif (tcgetattr(term_fd, &old_term) < 0) {\n+\t\tclose_term_fd();\n \t\treturn -1;\n+\t}\n \tsigchain_push_common(restore_term_on_signal);\n \n \treturn 0;\n@@ -52,7 +65,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n \tstruct termios t;\n \n \tif (save_term(flags) < 0)\n-\t\tgoto error;\n+\t\treturn -1;\n \n \tt = old_term;\n \n@@ -65,9 +78,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n \t\treturn 0;\n \n \tsigchain_pop_common();\n-error:\n-\tclose(term_fd);\n-\tterm_fd = -1;\n+\tclose_term_fd();\n \treturn -1;\n }\n \n@@ -362,7 +373,7 @@ int read_key_without_echo(struct strbuf *buf)\n \tstatic int warning_displayed;\n \tint ch;\n \n-\tif (warning_displayed || enable_non_canonical(0) < 0) {\n+\tif (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) {\n \t\tif (!warning_displayed) {\n \t\t\twarning(\"reading single keystrokes not supported on \"\n \t\t\t\t\"this platform; reading line instead\");\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex aeb24c9478..79ed00cf61 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -4,6 +4,8 @@\n enum save_term_flags {\n \t/* Save input and output settings */\n \tSAVE_TERM_DUPLEX = 1 << 0,\n+\t/* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */\n+\tSAVE_TERM_STDIN = 1 << 1,\n };\n \n /*\n-- \n2.35.1\n\n"},{"id":"451500","messageId":"20220316185405.29551-4-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220316185405.29551-1-phillip.wood123@gmail.com","subject":"[PATCH v4 3/4] terminal: work around macos poll() bug","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-16T18:54:04Z","receivedAt":"2022-03-16T18:54:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nOn macos the builtin \"add -p\" does not handle keys that generate\nescape sequences because poll() does not work with terminals\nthere. Switch to using select() on non-windows platforms to work\naround this.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 42 ++++++++++++++++++++++++++++++++++++------\n 1 file changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 4893294eb6..2ae8a6f13e 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -92,6 +92,31 @@ static int enable_non_canonical(enum save_term_flags flags)\n \treturn disable_bits(flags, ICANON | ECHO);\n }\n \n+/*\n+ * On macos it is not possible to use poll() with a terminal so use select\n+ * instead.\n+ */\n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct timeval tv, *tvp = NULL;\n+\tfd_set readfds;\n+\tint res;\n+\n+\tif (timeout >= 0) {\n+\t\ttv.tv_sec = timeout / 1000;\n+\t\ttv.tv_usec = (timeout % 1000) * 1000;\n+\t\ttvp = &tv;\n+\t}\n+\n+\tFD_ZERO(&readfds);\n+\tFD_SET(0, &readfds);\n+\tres = select(1, &readfds, NULL, NULL, tvp);\n+\tif (res <= 0)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #elif defined(GIT_WINDOWS_NATIVE)\n \n #define INPUT_PATH \"CONIN$\"\n@@ -257,6 +282,16 @@ static int mingw_getchar(void)\n }\n #define getchar mingw_getchar\n \n+static int getchar_with_timeout(int timeout)\n+{\n+\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n+\n+\tif (poll(&pfd, 1, timeout) < 1)\n+\t\treturn EOF;\n+\n+\treturn getchar();\n+}\n+\n #endif\n \n #ifndef FORCE_TEXT\n@@ -407,12 +442,7 @@ int read_key_without_echo(struct strbuf *buf)\n \t\t * half a second when we know that the sequence is complete.\n \t\t */\n \t\twhile (!is_known_escape_sequence(buf->buf)) {\n-\t\t\tstruct pollfd pfd = { .fd = 0, .events = POLLIN };\n-\n-\t\t\tif (poll(&pfd, 1, 500) < 1)\n-\t\t\t\tbreak;\n-\n-\t\t\tch = getchar();\n+\t\t\tch = getchar_with_timeout(500);\n \t\t\tif (ch == EOF)\n \t\t\t\tbreak;\n \t\t\tstrbuf_addch(buf, ch);\n-- \n2.35.1\n\n"},{"id":"451501","messageId":"20220316185405.29551-5-phillip.wood123@gmail.com","threadId":"57509","inReplyTo":"20220316185405.29551-1-phillip.wood123@gmail.com","subject":"[PATCH v4 4/4] terminal: restore settings on SIGTSTP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-16T18:54:05Z","receivedAt":"2022-03-16T18:54:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf the user suspends git while it is waiting for a keypress reset the\nterminal before stopping and restore the settings when git resumes. If\nthe user tries to resume in the background print an error\nmessage (taking care to use async safe functions) before stopping\nagain. Ideally we would reprint the prompt for the user when git\nresumes but this patch just restarts the read().\n\nThe signal handler is established with sigaction() rather than using\nsigchain_push() as this allows us to control the signal mask when the\nhandler is invoked and ensure SA_RESTART is used to restart the\nread() when resuming.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 132 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 129 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 2ae8a6f13e..7db330c52d 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -1,4 +1,4 @@\n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"compat/terminal.h\"\n #include \"sigchain.h\"\n #include \"strbuf.h\"\n@@ -24,6 +24,102 @@ static volatile sig_atomic_t term_fd_needs_closing;\n static int term_fd = -1;\n static struct termios old_term;\n \n+static const char *background_resume_msg;\n+static const char *restore_error_msg;\n+static volatile sig_atomic_t ttou_received;\n+\n+/* async safe error function for use by signal handlers. */\n+static void write_err(const char *msg)\n+{\n+\twrite_in_full(2, \"error: \", strlen(\"error: \"));\n+\twrite_in_full(2, msg, strlen(msg));\n+\twrite_in_full(2, \"\\n\", 1);\n+}\n+\n+static void print_background_resume_msg(int signo)\n+{\n+\tint saved_errno = errno;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\n+\tttou_received = 1;\n+\twrite_err(background_resume_msg);\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\terrno = saved_errno;\n+}\n+\n+static void restore_terminal_on_suspend(int signo)\n+{\n+\tint saved_errno = errno;\n+\tint res;\n+\tstruct termios t;\n+\tsigset_t mask;\n+\tstruct sigaction old_sa;\n+\tstruct sigaction sa = { .sa_handler = SIG_DFL };\n+\tint can_restore = 1;\n+\n+\tif (tcgetattr(term_fd, &t) < 0)\n+\t\tcan_restore = 0;\n+\n+\tif (tcsetattr(term_fd, TCSAFLUSH, &old_term) < 0)\n+\t\twrite_err(restore_error_msg);\n+\n+\tsigaction(signo, &sa, &old_sa);\n+\traise(signo);\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, signo);\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\t/* Stopped here */\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tsigaction(signo, &old_sa, NULL);\n+\tif (!can_restore) {\n+\t\twrite_err(restore_error_msg);\n+\t\tgoto out;\n+\t}\n+\t/*\n+\t * If we resume in the background then we receive SIGTTOU when calling\n+\t * tcsetattr() below. Set up a handler to print an error message in that\n+\t * case.\n+\t */\n+\tsigemptyset(&mask);\n+\tsigaddset(&mask, SIGTTOU);\n+\tsa.sa_mask = old_sa.sa_mask;\n+\tsa.sa_handler = print_background_resume_msg;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGTTOU, &sa, &old_sa);\n+ again:\n+\tttou_received = 0;\n+\tsigprocmask(SIG_UNBLOCK, &mask, NULL);\n+\tres = tcsetattr(term_fd, TCSAFLUSH, &t);\n+\tsigprocmask(SIG_BLOCK, &mask, NULL);\n+\tif (ttou_received)\n+\t\tgoto again;\n+\telse if (res < 0)\n+\t\twrite_err(restore_error_msg);\n+\tsigaction(SIGTTOU, &old_sa, NULL);\n+ out:\n+\terrno = saved_errno;\n+}\n+\n+static void reset_job_signals(void)\n+{\n+\tif (restore_error_msg) {\n+\t\tsignal(SIGTTIN, SIG_DFL);\n+\t\tsignal(SIGTTOU, SIG_DFL);\n+\t\tsignal(SIGTSTP, SIG_DFL);\n+\t\trestore_error_msg = NULL;\n+\t\tbackground_resume_msg = NULL;\n+\t}\n+}\n+\n static void close_term_fd(void)\n {\n \tif (term_fd_needs_closing)\n@@ -40,10 +136,13 @@ void restore_term(void)\n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n \tclose_term_fd();\n \tsigchain_pop_common();\n+\treset_job_signals();\n }\n \n int save_term(enum save_term_flags flags)\n {\n+\tstruct sigaction sa;\n+\n \tif (term_fd < 0)\n \t\tterm_fd = ((flags & SAVE_TERM_STDIN)\n \t\t\t ? 0\n@@ -56,6 +155,26 @@ int save_term(enum save_term_flags flags)\n \t\treturn -1;\n \t}\n \tsigchain_push_common(restore_term_on_signal);\n+\t/*\n+\t * If job control is disabled then the shell will have set the\n+\t * disposition of SIGTSTP to SIG_IGN.\n+\t */\n+\tsigaction(SIGTSTP, NULL, &sa);\n+\tif (sa.sa_handler == SIG_IGN)\n+\t\treturn 0;\n+\n+\t/* avoid calling gettext() from signal handler */\n+\tbackground_resume_msg = _(\"cannot resume in the background, please use 'fg' to resume\");\n+\trestore_error_msg = _(\"cannot restore terminal settings\");\n+\tsa.sa_handler = restore_terminal_on_suspend;\n+\tsa.sa_flags = SA_RESTART;\n+\tsigemptyset(&sa.sa_mask);\n+\tsigaddset(&sa.sa_mask, SIGTSTP);\n+\tsigaddset(&sa.sa_mask, SIGTTIN);\n+\tsigaddset(&sa.sa_mask, SIGTTOU);\n+\tsigaction(SIGTSTP, &sa, NULL);\n+\tsigaction(SIGTTIN, &sa, NULL);\n+\tsigaction(SIGTTOU, &sa, NULL);\n \n \treturn 0;\n }\n@@ -78,6 +197,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits)\n \t\treturn 0;\n \n \tsigchain_pop_common();\n+\treset_job_signals();\n \tclose_term_fd();\n \treturn -1;\n }\n@@ -102,6 +222,7 @@ static int getchar_with_timeout(int timeout)\n \tfd_set readfds;\n \tint res;\n \n+ again:\n \tif (timeout >= 0) {\n \t\ttv.tv_sec = timeout / 1000;\n \t\ttv.tv_usec = (timeout % 1000) * 1000;\n@@ -111,9 +232,14 @@ static int getchar_with_timeout(int timeout)\n \tFD_ZERO(&readfds);\n \tFD_SET(0, &readfds);\n \tres = select(1, &readfds, NULL, NULL, tvp);\n-\tif (res <= 0)\n+\tif (!res)\n \t\treturn EOF;\n-\n+\tif (res < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tgoto again;\n+\t\telse\n+\t\t\treturn EOF;\n+\t}\n \treturn getchar();\n }\n \n-- \n2.35.1\n\n"}]}