{"thread":{"id":"57421","subject":"[PATCH 0/3] a couple of read_key_without_echo() fixes","startedAt":"2022-02-16T10:54:39Z","lastAt":"2022-03-28T10:51:37Z","messageCount":18,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Phillip Wood","Carlo Arenas","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"448578","messageId":"pull.1146.git.1645008873.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":null,"subject":"[PATCH 0/3] a couple of read_key_without_echo() fixes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:54:29Z","receivedAt":"2022-02-16T10:54:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Fix a couple of bugs I noticed when using the builtin \"add -p\" with\ninteractive.singlekey=true. The first patch is a general fix for the\nterminal save/restore functionality which forgot to call sigchain_pop() when\nit restored the terminal. The last two fix reading single keys in\nnon-canonical mode by making sure we wait for an initial key press and only\nread one byte at a time from the underlying file descriptor.\n\nNote that these are untested on windows beyond our CI compiling them\n\nPhillip Wood (3):\n terminal: pop signal handler when terminal is restored\n terminal: set VMIN and VTIME in non-canonical mode\n add -p: disable stdin buffering when interactive.singlekey is set\n\n add-interactive.c | 2 ++\n compat/terminal.c | 27 ++++++++++++++++++++++-----\n compat/terminal.h | 8 ++++++++\n 3 files changed, 32 insertions(+), 5 deletions(-)\n\n\nbase-commit: b80121027d1247a0754b3cc46897fee75c050b44\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1146\n-- \ngitgitgadget\n"},{"id":"448579","messageId":"9a3c2cea0f904130e944842c3f1d2f31a6b7e95b.1645008873.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.git.1645008873.gitgitgadget@gmail.com","subject":"[PATCH 1/3] terminal: pop signal handler when terminal is restored","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:54:30Z","receivedAt":"2022-02-16T10:54:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen disable_bits() changes the terminal attributes it uses\nsigchain_push_common() to restore the terminal if a signal is received\nbefore restore_term() is called. However there is no corresponding\ncall to sigchain_pop_common() when the settings are restored so the\nsignal handler is left on the sigchain stack. This leaves the stack\nunbalanced so code such as\n\nsigchain_push_common(my_handler);\n...\nread_key_without_echo(...);\n...\nsigchain_pop_common();\n\npops the handler pushed by disable_bits() rather than the one it\nintended to. Additionally \"git add -p\" changes the terminal settings\nevery time it reads a key press so the stack can grow significantly.\n\nIn order to fix this save_term() now sets up the signal handler so\nrestore_term() can unconditionally call sigchain_pop_common(). There\nare no callers of save_term() outside of terminal.c as the only\nexternal caller was removed by e3f7e01b50 (\"Revert \"editor: save and\nreset terminal after calling EDITOR\"\", 2021-11-22). Any future callers\nof save_term() should benefit from having the signal handler set up\nfor them. For example if we reinstate the code to protect against an\neditor failing to restore the terminal attributes we would want that\ncode to restore the terminal attributes on SIGINT. (As an aside\nrun_command() installs a signal handler that waits for the child\nbefore re-raising the signal so we would be guaranteed to restore the\nsettings after the child has exited.)\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 17 +++++++++++++----\n compat/terminal.h | 8 ++++++++\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 5b903e7c7e3..20803badd03 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -11,7 +11,7 @@\n static void restore_term_on_signal(int sig)\n {\n \trestore_term();\n-\tsigchain_pop(sig);\n+\t/* restore_term calls sigchain_pop_common */\n \traise(sig);\n }\n \n@@ -31,14 +31,20 @@ void restore_term(void)\n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n \tclose(term_fd);\n \tterm_fd = -1;\n+\tsigchain_pop_common();\n }\n \n int save_term(int full_duplex)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\tif (term_fd < 0)\n+\t\treturn -1;\n+\tif (tcgetattr(term_fd, &old_term) < 0)\n+\t\treturn -1;\n+\tsigchain_push_common(restore_term_on_signal);\n \n-\treturn (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);\n+\treturn 0;\n }\n \n static int disable_bits(tcflag_t bits)\n@@ -49,12 +55,12 @@ static int disable_bits(tcflag_t bits)\n \t\tgoto error;\n \n \tt = old_term;\n-\tsigchain_push_common(restore_term_on_signal);\n \n \tt.c_lflag &= ~bits;\n \tif (!tcsetattr(term_fd, TCSAFLUSH, &t))\n \t\treturn 0;\n \n+\tsigchain_pop_common();\n error:\n \tclose(term_fd);\n \tterm_fd = -1;\n@@ -100,6 +106,8 @@ void restore_term(void)\n \t\treturn;\n \t}\n \n+\tsigchain_pop_common();\n+\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn;\n \n@@ -134,6 +142,7 @@ int save_term(int full_duplex)\n \n \tGetConsoleMode(hconin, &cmode_in);\n \tuse_stty = 0;\n+\tsigchain_push_common(restore_term_on_signal);\n \treturn 0;\n error:\n \tCloseHandle(hconin);\n@@ -177,10 +186,10 @@ static int disable_bits(DWORD bits)\n \tif (save_term(0) < 0)\n \t\treturn -1;\n \n-\tsigchain_push_common(restore_term_on_signal);\n \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n \t\tCloseHandle(hconin);\n \t\thconin = INVALID_HANDLE_VALUE;\n+\t\tsigchain_pop_common();\n \t\treturn -1;\n \t}\n \ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex e1770c575b2..0fb9fa147c4 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,7 +1,15 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\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+/* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n char *git_terminal_prompt(const char *prompt, int echo);\n-- \ngitgitgadget\n\n"},{"id":"448580","messageId":"02009172e081ab5c4de8e2476c1142f97b41698e.1645008873.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.git.1645008873.gitgitgadget@gmail.com","subject":"[PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:54:31Z","receivedAt":"2022-02-16T10:54:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf VMIN and VTIME are both set to zero then the terminal performs\nnon-blocking reads which means that read_key_without_echo() returns\nEOF if there is no key press pending. This results in the user being\nunable to select anything when running \"git add -p\". Fix this by\nexplicitly setting VMIN and VTIME when enabling non-canonical mode.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 20803badd03..d882dfa06e3 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits)\n \tt = old_term;\n \n \tt.c_lflag &= ~bits;\n+\tif (bits & ICANON) {\n+\t\tt.c_cc[VMIN] = 1;\n+\t\tt.c_cc[VTIME] = 0;\n+\t}\n \tif (!tcsetattr(term_fd, TCSAFLUSH, &t))\n \t\treturn 0;\n \n@@ -159,7 +163,11 @@ static int disable_bits(DWORD bits)\n \n \t\tif (bits & ENABLE_LINE_INPUT) {\n \t\t\tstring_list_append(&stty_restore, \"icanon\");\n-\t\t\tstrvec_push(&cp.args, \"-icanon\");\n+\t\t\t/*\n+\t\t\t * POSIX allows VMIN and VTIME to overlap with VEOF and\n+\t\t\t * VEOL - let's hope that is not the case on windows.\n+\t\t\t */\n+\t\t\tstrvec_pushl(&cp.args, \"-icanon\", \"min\", \"1\", \"time\", \"0\", NULL);\n \t\t}\n \n \t\tif (bits & ENABLE_ECHO_INPUT) {\n-- \ngitgitgadget\n\n"},{"id":"448581","messageId":"6d8423b6e1e969ffed02b148546b88d118affd09.1645008873.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.git.1645008873.gitgitgadget@gmail.com","subject":"[PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:54:32Z","receivedAt":"2022-02-16T10:54:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe builtin \"add -p\" reads the key \"F2\" as three separate keys \"^[\",\n\"O\" and \"Q\". The \"Q\" causes it to quit which is probably not what the\nuser was expecting. This is because it uses poll() to check for\npending input when reading escape sequences but reads the input with\ngetchar() which is buffered by default and so hoovers up all the\npending input leading poll() think there isn't anything pending. Fix\nthis by calling setbuf() to disable input buffering if\ninteractive.singlekey is set.\n\nLooking at the comment above mingw_getchar() in terminal.c I wonder if\nthat function is papering over this bug and could be removed.\nUnfortunately I don't have access to windows to test that.\n\nSigned-off-by: Phillip Wood \n---\n add-interactive.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6498ae196f1..ad78774ca26 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \t\t\t &s->interactive_diff_algorithm);\n \n \tgit_config_get_bool(\"interactive.singlekey\", &s->use_single_key);\n+\tif (s->use_single_key)\n+\t\tsetbuf(stdin, NULL);\n }\n \n void clear_add_i_state(struct add_i_state *s)\n-- \ngitgitgadget\n"},{"id":"448639","messageId":"xmqqpmnmcwwu.fsf@gitster.g","threadId":"57421","inReplyTo":"02009172e081ab5c4de8e2476c1142f97b41698e.1645008873.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T21:40:49Z","receivedAt":"2022-02-16T21:40:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> If VMIN and VTIME are both set to zero then the terminal performs\n> non-blocking reads which means that read_key_without_echo() returns\n> EOF if there is no key press pending. This results in the user being\n> unable to select anything when running \"git add -p\". Fix this by\n> explicitly setting VMIN and VTIME when enabling non-canonical mode.\n\nMakes sense.\n\n>\n> Signed-off-by: Phillip Wood \n> ---\n> compat/terminal.c | 10 +++++++++-\n> 1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 20803badd03..d882dfa06e3 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits)\n> \tt = old_term;\n> \n> \tt.c_lflag &= ~bits;\n> +\tif (bits & ICANON) {\n> +\t\tt.c_cc[VMIN] = 1;\n> +\t\tt.c_cc[VTIME] = 0;\n> +\t}\n\nQuite sensible. I wonder if we can simplify the \"are we looking at\nan ESC that is the first byte of a multi-byte control sequence?\"\nlogic in the caller by using inter-character delay, but it is\nprobably better to go one step at a time like this patch does.\n\n> \tif (!tcsetattr(term_fd, TCSAFLUSH, &t))\n> \t\treturn 0;\n> \n> @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits)\n> \n> \t\tif (bits & ENABLE_LINE_INPUT) {\n> \t\t\tstring_list_append(&stty_restore, \"icanon\");\n> -\t\t\tstrvec_push(&cp.args, \"-icanon\");\n> +\t\t\t/*\n> +\t\t\t * POSIX allows VMIN and VTIME to overlap with VEOF and\n> +\t\t\t * VEOL - let's hope that is not the case on windows.\n> +\t\t\t */\n> +\t\t\tstrvec_pushl(&cp.args, \"-icanon\", \"min\", \"1\", \"time\", \"0\", NULL);\n\nInteresting. So each call to read_key_without_echo() ends up being\na run_command(\"stty -icanon min 1 time 0\") followed by a read\nfollowed by another \"stty\".\n\nAt least while in -icanon mode, VEOF and VEOL do not take effect,\nand the potential overlap would not matter. It really depends on\nwhat happens upon restore.\n\nDo we have similar \"let's hope\" on the tcsetattr() side, too?\n\n> \t\t}\n> \n> \t\tif (bits & ENABLE_ECHO_INPUT) {\n\nThanks.\n"},{"id":"448640","messageId":"xmqqleyacws4.fsf@gitster.g","threadId":"57421","inReplyTo":"6d8423b6e1e969ffed02b148546b88d118affd09.1645008873.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] add -p: disable stdin buffering when interactive.singlekey is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T21:43:39Z","receivedAt":"2022-02-16T21:43:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 6498ae196f1..ad78774ca26 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n> \t\t\t &s->interactive_diff_algorithm);\n> \n> \tgit_config_get_bool(\"interactive.singlekey\", &s->use_single_key);\n> +\tif (s->use_single_key)\n> +\t\tsetbuf(stdin, NULL);\n\nOK. Will queue. Thanks.\n\n> }\n> \n> void clear_add_i_state(struct add_i_state *s)\n"},{"id":"448679","messageId":"f6c31ac8-b675-99c8-1577-07c26619e277@gmail.com","threadId":"57421","inReplyTo":"xmqqpmnmcwwu.fsf@gitster.g","subject":"Re: [PATCH 2/3] terminal: set VMIN and VTIME in non-canonical mode","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-17T10:59:12Z","receivedAt":"2022-02-17T11:01:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/02/2022 21:40, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> From: Phillip Wood \n>>\n>> If VMIN and VTIME are both set to zero then the terminal performs\n>> non-blocking reads which means that read_key_without_echo() returns\n>> EOF if there is no key press pending. This results in the user being\n>> unable to select anything when running \"git add -p\". Fix this by\n>> explicitly setting VMIN and VTIME when enabling non-canonical mode.\n> \n> Makes sense.\n> \n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>> compat/terminal.c | 10 +++++++++-\n>> 1 file changed, 9 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/compat/terminal.c b/compat/terminal.c\n>> index 20803badd03..d882dfa06e3 100644\n>> --- a/compat/terminal.c\n>> +++ b/compat/terminal.c\n>> @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits)\n>> \tt = old_term;\n>> \n>> \tt.c_lflag &= ~bits;\n>> +\tif (bits & ICANON) {\n>> +\t\tt.c_cc[VMIN] = 1;\n>> +\t\tt.c_cc[VTIME] = 0;\n>> +\t}\n> \n> Quite sensible. I wonder if we can simplify the \"are we looking at\n> an ESC that is the first byte of a multi-byte control sequence?\"\n> logic in the caller by using inter-character delay, but it is\n> probably better to go one step at a time like this patch does.\n\nI wondered about that too, but we'd need to keep the poll() for windows \nI think (at least in the case where it is using the windows console \nrather that mintty) so I'm not sure that it ends up any simpler than \nspecial casing poll on macos.\n\n>> \tif (!tcsetattr(term_fd, TCSAFLUSH, &t))\n>> \t\treturn 0;\n>> \n>> @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits)\n>> \n>> \t\tif (bits & ENABLE_LINE_INPUT) {\n>> \t\t\tstring_list_append(&stty_restore, \"icanon\");\n>> -\t\t\tstrvec_push(&cp.args, \"-icanon\");\n>> +\t\t\t/*\n>> +\t\t\t * POSIX allows VMIN and VTIME to overlap with VEOF and\n>> +\t\t\t * VEOL - let's hope that is not the case on windows.\n>> +\t\t\t */\n>> +\t\t\tstrvec_pushl(&cp.args, \"-icanon\", \"min\", \"1\", \"time\", \"0\", NULL);\n> \n> Interesting. So each call to read_key_without_echo() ends up being\n> a run_command(\"stty -icanon min 1 time 0\") followed by a read\n> followed by another \"stty\".\n> \n> At least while in -icanon mode, VEOF and VEOL do not take effect,\n> and the potential overlap would not matter. It really depends on\n> what happens upon restore.\n\nIndeed, we could run another stty process so we can remember the \noriginal VEOF and VEOL but that's more complexity and forking.\n\n> Do we have similar \"let's hope\" on the tcsetattr() side, too?\n\nWe're OK there because we modify a copy of the original attributes when \nchanging the settings and use the unmodified originals when restoring them.\n\nBest Wishes\n\nPhillip\n\n>> \t\t}\n>> \n>> \t\tif (bits & ENABLE_ECHO_INPUT) {\n> \n> Thanks.\n\n"},{"id":"449181","messageId":"pull.1146.v2.git.1645556015.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.git.1645008873.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-22T18:53:31Z","receivedAt":"2022-02-22T18:53:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"I have added a new patch to the beginning of the series that fixes a case\nwhere we did not call restore_term() when leaving read_key_without_echo(). I\nhave also reworded the commit message to patch 2 as SIGINT is actually\nignored while the editor is running (we should probably change that code to\nuse wait_after_clean instead).\n\nCover letter for V1:\n\nFix a couple of bugs I noticed when using the builtin \"add -p\" with\ninteractive.singlekey=true. The first patch is a general fix for the\nterminal save/restore functionality which forgot to call sigchain_pop() when\nit restored the terminal. The last two fix reading single keys in\nnon-canonical mode by making sure we wait for an initial key press and only\nread one byte at a time from the underlying file descriptor.\n\nNote that these are untested on windows beyond our CI compiling them\n\nPhillip Wood (4):\n terminal: always reset terminal when reading without echo\n terminal: pop signal handler when terminal is restored\n terminal: set VMIN and VTIME in non-canonical mode\n add -p: disable stdin buffering when interactive.singlekey is set\n\n add-interactive.c | 2 ++\n compat/terminal.c | 29 +++++++++++++++++++++++------\n compat/terminal.h | 8 ++++++++\n 3 files changed, 33 insertions(+), 6 deletions(-)\n\n\nbase-commit: b80121027d1247a0754b3cc46897fee75c050b44\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1146\n\nRange-diff vs v1:\n\n -: ----------- > 1: 45609d61afc terminal: always reset terminal when reading without echo\n 1: 9a3c2cea0f9 ! 2: 0020953f1cf terminal: pop signal handler when terminal is restored\n @@ Commit message\n external caller was removed by e3f7e01b50 (\"Revert \"editor: save and\n reset terminal after calling EDITOR\"\", 2021-11-22). Any future callers\n of save_term() should benefit from having the signal handler set up\n - for them. For example if we reinstate the code to protect against an\n - editor failing to restore the terminal attributes we would want that\n - code to restore the terminal attributes on SIGINT. (As an aside\n - run_command() installs a signal handler that waits for the child\n - before re-raising the signal so we would be guaranteed to restore the\n - settings after the child has exited.)\n + for them.\n \n Signed-off-by: Phillip Wood \n \n 2: 02009172e08 = 3: 7ae9b236554 terminal: set VMIN and VTIME in non-canonical mode\n 3: 6d8423b6e1e = 4: 39b061a471b add -p: disable stdin buffering when interactive.singlekey is set\n\n-- \ngitgitgadget\n"},{"id":"449182","messageId":"45609d61afc647de3afafc4203d904676837ffe3.1645556015.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.v2.git.1645556015.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] terminal: always reset terminal when reading without echo","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-22T18:53:32Z","receivedAt":"2022-02-22T18:53:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nBreak out of the loop to ensure restore_term() is called before\nreturning.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 5b903e7c7e3..fb8c70a6251 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -385,7 +385,7 @@ int read_key_without_echo(struct strbuf *buf)\n \n \t\t\tch = getchar();\n \t\t\tif (ch == EOF)\n-\t\t\t\treturn 0;\n+\t\t\t\tbreak;\n \t\t\tstrbuf_addch(buf, ch);\n \t\t}\n \t}\n-- \ngitgitgadget\n\n"},{"id":"449183","messageId":"0020953f1cf901e66883853392915ba8ce07ca61.1645556015.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.v2.git.1645556015.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] terminal: pop signal handler when terminal is restored","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-22T18:53:33Z","receivedAt":"2022-02-22T18:53:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen disable_bits() changes the terminal attributes it uses\nsigchain_push_common() to restore the terminal if a signal is received\nbefore restore_term() is called. However there is no corresponding\ncall to sigchain_pop_common() when the settings are restored so the\nsignal handler is left on the sigchain stack. This leaves the stack\nunbalanced so code such as\n\nsigchain_push_common(my_handler);\n...\nread_key_without_echo(...);\n...\nsigchain_pop_common();\n\npops the handler pushed by disable_bits() rather than the one it\nintended to. Additionally \"git add -p\" changes the terminal settings\nevery time it reads a key press so the stack can grow significantly.\n\nIn order to fix this save_term() now sets up the signal handler so\nrestore_term() can unconditionally call sigchain_pop_common(). There\nare no callers of save_term() outside of terminal.c as the only\nexternal caller was removed by e3f7e01b50 (\"Revert \"editor: save and\nreset terminal after calling EDITOR\"\", 2021-11-22). Any future callers\nof save_term() should benefit from having the signal handler set up\nfor them.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 17 +++++++++++++----\n compat/terminal.h | 8 ++++++++\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex fb8c70a6251..11288cfe5c9 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -11,7 +11,7 @@\n static void restore_term_on_signal(int sig)\n {\n \trestore_term();\n-\tsigchain_pop(sig);\n+\t/* restore_term calls sigchain_pop_common */\n \traise(sig);\n }\n \n@@ -31,14 +31,20 @@ void restore_term(void)\n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n \tclose(term_fd);\n \tterm_fd = -1;\n+\tsigchain_pop_common();\n }\n \n int save_term(int full_duplex)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n+\tif (term_fd < 0)\n+\t\treturn -1;\n+\tif (tcgetattr(term_fd, &old_term) < 0)\n+\t\treturn -1;\n+\tsigchain_push_common(restore_term_on_signal);\n \n-\treturn (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);\n+\treturn 0;\n }\n \n static int disable_bits(tcflag_t bits)\n@@ -49,12 +55,12 @@ static int disable_bits(tcflag_t bits)\n \t\tgoto error;\n \n \tt = old_term;\n-\tsigchain_push_common(restore_term_on_signal);\n \n \tt.c_lflag &= ~bits;\n \tif (!tcsetattr(term_fd, TCSAFLUSH, &t))\n \t\treturn 0;\n \n+\tsigchain_pop_common();\n error:\n \tclose(term_fd);\n \tterm_fd = -1;\n@@ -100,6 +106,8 @@ void restore_term(void)\n \t\treturn;\n \t}\n \n+\tsigchain_pop_common();\n+\n \tif (hconin == INVALID_HANDLE_VALUE)\n \t\treturn;\n \n@@ -134,6 +142,7 @@ int save_term(int full_duplex)\n \n \tGetConsoleMode(hconin, &cmode_in);\n \tuse_stty = 0;\n+\tsigchain_push_common(restore_term_on_signal);\n \treturn 0;\n error:\n \tCloseHandle(hconin);\n@@ -177,10 +186,10 @@ static int disable_bits(DWORD bits)\n \tif (save_term(0) < 0)\n \t\treturn -1;\n \n-\tsigchain_push_common(restore_term_on_signal);\n \tif (!SetConsoleMode(hconin, cmode_in & ~bits)) {\n \t\tCloseHandle(hconin);\n \t\thconin = INVALID_HANDLE_VALUE;\n+\t\tsigchain_pop_common();\n \t\treturn -1;\n \t}\n \ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex e1770c575b2..0fb9fa147c4 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,7 +1,15 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\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+/* Restore the terminal attributes that were saved with save_term() */\n void restore_term(void);\n \n char *git_terminal_prompt(const char *prompt, int echo);\n-- \ngitgitgadget\n\n"},{"id":"449184","messageId":"7ae9b2365542b68d7d75f39b126d2acec3e223ff.1645556015.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.v2.git.1645556015.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] terminal: set VMIN and VTIME in non-canonical mode","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-22T18:53:34Z","receivedAt":"2022-02-22T18:53:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf VMIN and VTIME are both set to zero then the terminal performs\nnon-blocking reads which means that read_key_without_echo() returns\nEOF if there is no key press pending. This results in the user being\nunable to select anything when running \"git add -p\". Fix this by\nexplicitly setting VMIN and VTIME when enabling non-canonical mode.\n\nSigned-off-by: Phillip Wood \n---\n compat/terminal.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 11288cfe5c9..3620184e790 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits)\n \tt = old_term;\n \n \tt.c_lflag &= ~bits;\n+\tif (bits & ICANON) {\n+\t\tt.c_cc[VMIN] = 1;\n+\t\tt.c_cc[VTIME] = 0;\n+\t}\n \tif (!tcsetattr(term_fd, TCSAFLUSH, &t))\n \t\treturn 0;\n \n@@ -159,7 +163,11 @@ static int disable_bits(DWORD bits)\n \n \t\tif (bits & ENABLE_LINE_INPUT) {\n \t\t\tstring_list_append(&stty_restore, \"icanon\");\n-\t\t\tstrvec_push(&cp.args, \"-icanon\");\n+\t\t\t/*\n+\t\t\t * POSIX allows VMIN and VTIME to overlap with VEOF and\n+\t\t\t * VEOL - let's hope that is not the case on windows.\n+\t\t\t */\n+\t\t\tstrvec_pushl(&cp.args, \"-icanon\", \"min\", \"1\", \"time\", \"0\", NULL);\n \t\t}\n \n \t\tif (bits & ENABLE_ECHO_INPUT) {\n-- \ngitgitgadget\n\n"},{"id":"449185","messageId":"39b061a471bb6e5306f94b3a6d7e19be6ed6a92f.1645556015.git.gitgitgadget@gmail.com","threadId":"57421","inReplyTo":"pull.1146.v2.git.1645556015.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] add -p: disable stdin buffering when interactive.singlekey is set","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-22T18:53:35Z","receivedAt":"2022-02-22T18:53:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe builtin \"add -p\" reads the key \"F2\" as three separate keys \"^[\",\n\"O\" and \"Q\". The \"Q\" causes it to quit which is probably not what the\nuser was expecting. This is because it uses poll() to check for\npending input when reading escape sequences but reads the input with\ngetchar() which is buffered by default and so hoovers up all the\npending input leading poll() think there isn't anything pending. Fix\nthis by calling setbuf() to disable input buffering if\ninteractive.singlekey is set.\n\nLooking at the comment above mingw_getchar() in terminal.c I wonder if\nthat function is papering over this bug and could be removed.\nUnfortunately I don't have access to windows to test that.\n\nSigned-off-by: Phillip Wood \n---\n add-interactive.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6498ae196f1..ad78774ca26 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)\n \t\t\t &s->interactive_diff_algorithm);\n \n \tgit_config_get_bool(\"interactive.singlekey\", &s->use_single_key);\n+\tif (s->use_single_key)\n+\t\tsetbuf(stdin, NULL);\n }\n \n void clear_add_i_state(struct add_i_state *s)\n-- \ngitgitgadget\n"},{"id":"449346","messageId":"xmqqv8x5s1w3.fsf@gitster.g","threadId":"57421","inReplyTo":"pull.1146.v2.git.1645556015.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T21:34:36Z","receivedAt":"2022-02-23T21:34:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> I have added a new patch to the beginning of the series that fixes a case\n> where we did not call restore_term() when leaving read_key_without_echo(). I\n> have also reworded the commit message to patch 2 as SIGINT is actually\n> ignored while the editor is running (we should probably change that code to\n> use wait_after_clean instead).\n\nAll patches looked good. Thanks.\n\nLet's mark it for 'next' and below soonish.\n\n>\n> Cover letter for V1:\n>\n> Fix a couple of bugs I noticed when using the builtin \"add -p\" with\n> interactive.singlekey=true. The first patch is a general fix for the\n> terminal save/restore functionality which forgot to call sigchain_pop() when\n> it restored the terminal. The last two fix reading single keys in\n> non-canonical mode by making sure we wait for an initial key press and only\n> read one byte at a time from the underlying file descriptor.\n>\n> Note that these are untested on windows beyond our CI compiling them\n>\n> Phillip Wood (4):\n> terminal: always reset terminal when reading without echo\n> terminal: pop signal handler when terminal is restored\n> terminal: set VMIN and VTIME in non-canonical mode\n> add -p: disable stdin buffering when interactive.singlekey is set\n>\n> add-interactive.c | 2 ++\n> compat/terminal.c | 29 +++++++++++++++++++++++------\n> compat/terminal.h | 8 ++++++++\n> 3 files changed, 33 insertions(+), 6 deletions(-)\n>\n>\n> base-commit: b80121027d1247a0754b3cc46897fee75c050b44\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1146\n>\n> Range-diff vs v1:\n>\n> -: ----------- > 1: 45609d61afc terminal: always reset terminal when reading without echo\n> 1: 9a3c2cea0f9 ! 2: 0020953f1cf terminal: pop signal handler when terminal is restored\n> @@ Commit message\n> external caller was removed by e3f7e01b50 (\"Revert \"editor: save and\n> reset terminal after calling EDITOR\"\", 2021-11-22). Any future callers\n> of save_term() should benefit from having the signal handler set up\n> - for them. For example if we reinstate the code to protect against an\n> - editor failing to restore the terminal attributes we would want that\n> - code to restore the terminal attributes on SIGINT. (As an aside\n> - run_command() installs a signal handler that waits for the child\n> - before re-raising the signal so we would be guaranteed to restore the\n> - settings after the child has exited.)\n> + for them.\n> \n> Signed-off-by: Phillip Wood \n> \n> 2: 02009172e08 = 3: 7ae9b236554 terminal: set VMIN and VTIME in non-canonical mode\n> 3: 6d8423b6e1e = 4: 39b061a471b add -p: disable stdin buffering when interactive.singlekey is set\n"},{"id":"449438","messageId":"8d44b509-ff19-0629-30f5-ae785c73c3aa@gmail.com","threadId":"57421","inReplyTo":"xmqqv8x5s1w3.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-24T14:30:08Z","receivedAt":"2022-02-24T14:30:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 23/02/2022 21:34, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> I have added a new patch to the beginning of the series that fixes a case\n>> where we did not call restore_term() when leaving read_key_without_echo(). I\n>> have also reworded the commit message to patch 2 as SIGINT is actually\n>> ignored while the editor is running (we should probably change that code to\n>> use wait_after_clean instead).\n> \n> All patches looked good. Thanks.\n> \n> Let's mark it for 'next' and below soonish.\n\nThat sounds good. I've got a couple more patches based on top of these \nwhich hopefully fix the remaining problems (notably the macos poll() \nbug). I'll polish and post them next week. Once those are in I hope \nwe'll be able to enable the builtin \"add -p\" by default.\n\nThanks\n\nPhillip\n\n>>\n>> Cover letter for V1:\n>>\n>> Fix a couple of bugs I noticed when using the builtin \"add -p\" with\n>> interactive.singlekey=true. The first patch is a general fix for the\n>> terminal save/restore functionality which forgot to call sigchain_pop() when\n>> it restored the terminal. The last two fix reading single keys in\n>> non-canonical mode by making sure we wait for an initial key press and only\n>> read one byte at a time from the underlying file descriptor.\n>>\n>> Note that these are untested on windows beyond our CI compiling them\n>>\n>> Phillip Wood (4):\n>> terminal: always reset terminal when reading without echo\n>> terminal: pop signal handler when terminal is restored\n>> terminal: set VMIN and VTIME in non-canonical mode\n>> add -p: disable stdin buffering when interactive.singlekey is set\n>>\n>> add-interactive.c | 2 ++\n>> compat/terminal.c | 29 +++++++++++++++++++++++------\n>> compat/terminal.h | 8 ++++++++\n>> 3 files changed, 33 insertions(+), 6 deletions(-)\n>>\n>>\n>> base-commit: b80121027d1247a0754b3cc46897fee75c050b44\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1146%2Fphillipwood%2Fwip%2Fadd-p-vmin-v2-v2\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1146/phillipwood/wip/add-p-vmin-v2-v2\n>> Pull-Request: https://github.com/gitgitgadget/git/pull/1146\n>>\n>> Range-diff vs v1:\n>>\n>> -: ----------- > 1: 45609d61afc terminal: always reset terminal when reading without echo\n>> 1: 9a3c2cea0f9 ! 2: 0020953f1cf terminal: pop signal handler when terminal is restored\n>> @@ Commit message\n>> external caller was removed by e3f7e01b50 (\"Revert \"editor: save and\n>> reset terminal after calling EDITOR\"\", 2021-11-22). Any future callers\n>> of save_term() should benefit from having the signal handler set up\n>> - for them. For example if we reinstate the code to protect against an\n>> - editor failing to restore the terminal attributes we would want that\n>> - code to restore the terminal attributes on SIGINT. (As an aside\n>> - run_command() installs a signal handler that waits for the child\n>> - before re-raising the signal so we would be guaranteed to restore the\n>> - settings after the child has exited.)\n>> + for them.\n>> \n>> Signed-off-by: Phillip Wood \n>> \n>> 2: 02009172e08 = 3: 7ae9b236554 terminal: set VMIN and VTIME in non-canonical mode\n>> 3: 6d8423b6e1e = 4: 39b061a471b add -p: disable stdin buffering when interactive.singlekey is set\n"},{"id":"451926","messageId":"CAPUEspg1cJC_UwjJFx-jnzWsascY++S3UgM1UCLRcnK_Mv2wOg@mail.gmail.com","threadId":"57421","inReplyTo":"8d44b509-ff19-0629-30f5-ae785c73c3aa@gmail.com","subject":"Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-03-22T20:18:03Z","receivedAt":"2022-03-22T20:18:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood wrote:\n> That sounds good. I've got a couple more patches based on top of these\n> which hopefully fix the remaining problems (notably the macos poll()\n> bug). I'll polish and post them next week. Once those are in I hope\n> we'll be able to enable the builtin \"add -p\" by default.\n\nAs this topic just hit master noticed (I apologize for not doing it\nsooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed\nwith 1/4) and therefore enabling the builtin by default as proposed\noriginally by dscho could proceed without the additional series.\n\nCarlo\n"},{"id":"451953","messageId":"xmqqy211t512.fsf@gitster.g","threadId":"57421","inReplyTo":"CAPUEspg1cJC_UwjJFx-jnzWsascY++S3UgM1UCLRcnK_Mv2wOg@mail.gmail.com","subject":"Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T09:03:05Z","receivedAt":"2022-03-23T09:03:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood wrote:\n>> That sounds good. I've got a couple more patches based on top of these\n>> which hopefully fix the remaining problems (notably the macos poll()\n>> bug). I'll polish and post them next week. Once those are in I hope\n>> we'll be able to enable the builtin \"add -p\" by default.\n>\n> As this topic just hit master noticed (I apologize for not doing it\n> sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed\n> with 1/4) and therefore enabling the builtin by default as proposed\n> originally by dscho could proceed without the additional series.\n\nAs long as it is a belated success report, nobody would mind. A\nbelated failure report would be a cause of sorrow and grief, but\neven then, it is better to have it late than never.\n\nThanks for testing.\n\n"},{"id":"452113","messageId":"nycvar.QRO.7.76.6.2203241427550.388@tvgsbejvaqbjf.bet","threadId":"57421","inReplyTo":"xmqqy211t512.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-24T13:29:59Z","receivedAt":"2022-03-24T13:30:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 23 Mar 2022, Junio C Hamano wrote:\n\n> Carlo Arenas writes:\n>\n> > On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood wrote:\n> >> That sounds good. I've got a couple more patches based on top of these\n> >> which hopefully fix the remaining problems (notably the macos poll()\n> >> bug). I'll polish and post them next week. Once those are in I hope\n> >> we'll be able to enable the builtin \"add -p\" by default.\n> >\n> > As this topic just hit master noticed (I apologize for not doing it\n> > sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed\n> > with 1/4) and therefore enabling the builtin by default as proposed\n> > originally by dscho could proceed without the additional series.\n>\n> As long as it is a belated success report, nobody would mind. A\n> belated failure report would be a cause of sorrow and grief, but\n> even then, it is better to have it late than never.\n>\n> Thanks for testing.\n\nFWIW I tried to reproduce the reported issues, but since I do not have any\nhardware running macOS anymore, I only had the avenue to debug via `tmate`\non one of GitHub Actions' macOS agents, but the problem did not reproduce\nfor me via SSH.\n\nI am grateful for the great work put in by Carlo and Phillip to fix these\nissues.\n\nThanks,\nDscho\n"},{"id":"452460","messageId":"7ff6c154-4f39-bd66-db89-970e92526ba8@gmail.com","threadId":"57421","inReplyTo":"CAPUEspg1cJC_UwjJFx-jnzWsascY++S3UgM1UCLRcnK_Mv2wOg@mail.gmail.com","subject":"Re: [PATCH v2 0/4] a couple of read_key_without_echo() fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-28T10:51:28Z","receivedAt":"2022-03-28T10:51:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 22/03/2022 20:18, Carlo Arenas wrote:\n> On Thu, Feb 24, 2022 at 6:30 AM Phillip Wood wrote:\n>> That sounds good. I've got a couple more patches based on top of these\n>> which hopefully fix the remaining problems (notably the macos poll()\n>> bug). I'll polish and post them next week. Once those are in I hope\n>> we'll be able to enable the builtin \"add -p\" by default.\n> \n> As this topic just hit master noticed (I apologize for not doing it\n> sooner) the macOS problem (tested in 10.15.7) was gone (suspect fixed\n> with 1/4) and therefore enabling the builtin by default as proposed\n> originally by dscho could proceed without the additional series.\n\nThanks for testing this series\n\nBest Wishes\n\nPhillip\n"}]}