| Message ID | 20220309110325.36917-1-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin add -p: hopefully final readkey fixes | expand |
Phillip Wood <phillip.wood123@gmail.com> writes: > V1 Cover Letter: > > Fix the remaining issues that I'm aware of when using the built in > "add -p" with interactive.singlekey that are stopping it from being > merged to master. The first three patches make sure that we call > tcsetattr() and the same file descriptor that we use for read() and > work around poll() being broken when reading from terminals on > macos. The final patch is more of an improvement rather than a bug fix > (the same issue already exists in the perl version) and could proceed > separately. > > Unfortunately these patches conflict with > 'cb/save-term-across-editor-invocation' as well as the textual > conflicts there is a semantic conflict as the argument to save_term() > is changed so the code in editor.c will need updating. > > These patches are based on 'pw/single-key-interactive' Is it still true, or does the base only apply to v1? $ git checkout --detach pw/single-key-interactive HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set $ git am -s ./+pw4-v2-add-p-single Applying: terminal: use flags for save_term() Applying: terminal: don't assume stdin is /dev/tty Applying: terminal: work around macos poll() bug error: patch failed: compat/terminal.c:397 error: compat/terminal.c: patch does not apply Patch failed at 0003 terminal: work around macos poll() bug hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". $ exit
Junio C Hamano <gitster@pobox.com> writes: >> These patches are based on 'pw/single-key-interactive' > > Is it still true, or does the base only apply to v1? > > $ git checkout --detach pw/single-key-interactive > HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set > $ git am -s ./+pw4-v2-add-p-single > Applying: terminal: use flags for save_term() > Applying: terminal: don't assume stdin is /dev/tty > Applying: terminal: work around macos poll() bug > error: patch failed: compat/terminal.c:397 > error: compat/terminal.c: patch does not apply > Patch failed at 0003 terminal: work around macos poll() bug > hint: Use 'git am --show-current-patch=diff' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > $ exit I think I figured it out. A merge of pw/single-key-interactive into a recent tip of 'master' wants the "return 0" in the preimage below to be "break" in compat/terminal.c > @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf) > * half a second when we know that the sequence is complete. > */ > while (!is_known_escape_sequence(buf->buf)) { > - struct pollfd pfd = { .fd = 0, .events = POLLIN }; > - > - if (poll(&pfd, 1, 500) < 1) > - break; > - > - ch = getchar(); > + ch = getchar_with_timeout(500); > if (ch == EOF) > return 0; > strbuf_addch(buf, ch); I'll wiggle it in to see how well it fares with other topics in flight. Thanks.
Hi Phillip, On Wed, 9 Mar 2022, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Thanks to Ramsay and Ævar for their comments on V1. > Changes since V1: > * Patch 1 > - use an enum for save_term()'s flags (suggested by Ævar) > - fixed argument order in the windows code (thanks to Ramsay) > * Patch 2 > - fixed a typo in a comment (thanks to Ramsay) > * Patch 4 > - stopped duplicating the strings returned by gettext() (suggested by > Ævar) > - reworked error message handling in the signal handler to add an > "error: " prefix (suggested by Ævar) > - tweaked the background resume error message While I did not ask for any of these changes, they look good to me. I had a look over the range-diff and found it reasonable. Onwards to a bright built-in `add -p` future! Thanks, Dscho
Hi Junio On 09/03/2022 23:37, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >>> These patches are based on 'pw/single-key-interactive' >> >> Is it still true, or does the base only apply to v1? The base is unchanged but does not seem to match pw/single-key-interactive. I'm not sure what happened there. They are based on 300db53b37 ("add -p: disable stdin buffering when interactive.singlekey is set", 2022-02-16) which is the second parent of e53fb7aa3f ("Merge branch 'pw/single-key-interactive' into seen", 2022-02-20) >> $ git checkout --detach pw/single-key-interactive >> HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set >> $ git am -s ./+pw4-v2-add-p-single >> Applying: terminal: use flags for save_term() >> Applying: terminal: don't assume stdin is /dev/tty >> Applying: terminal: work around macos poll() bug >> error: patch failed: compat/terminal.c:397 >> error: compat/terminal.c: patch does not apply >> Patch failed at 0003 terminal: work around macos poll() bug >> hint: Use 'git am --show-current-patch=diff' to see the failed patch >> When you have resolved this problem, run "git am --continue". >> If you prefer to skip this patch, run "git am --skip" instead. >> To restore the original branch and stop patching, run "git am --abort". >> $ exit > > I think I figured it out. A merge of pw/single-key-interactive into > a recent tip of 'master' wants the "return 0" in the preimage below > to be "break" in compat/terminal.c > > >> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf) >> * half a second when we know that the sequence is complete. >> */ >> while (!is_known_escape_sequence(buf->buf)) { >> - struct pollfd pfd = { .fd = 0, .events = POLLIN }; >> - >> - if (poll(&pfd, 1, 500) < 1) >> - break; >> - >> - ch = getchar(); >> + ch = getchar_with_timeout(500); >> if (ch == EOF) >> return 0; >> strbuf_addch(buf, ch); That looks good to me. However unfortunately there are some semantic conflicts as well. The patch below is based on 6b1f77214c ("Merge branch 'pw/add-p-single-key' into seen", 2022-03-09), hopefully Thunderbird wont mangle it. Whilst preparing the fixup I realized I need to reroll to fix a closing stdin in patch 2 and resetting the job signals on error in patch 4. What's the best base to use when rerolling? Best Wishes Phillip ---- >8 ---- From: Phillip Wood <phillip.wood@dunelm.org.uk> Date: Thu, 10 Mar 2022 11:05:26 +0000 Subject: [PATCH] fixup! Merge branch 'pw/add-p-single-key' into seen --- compat/terminal.c | 3 ++- editor.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/compat/terminal.c b/compat/terminal.c index 3172f4f408..d1ed5c07dc 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -141,7 +141,8 @@ int save_term(enum save_term_flags flags) if (term_fd < 0) return -1; if ((flags & SAVE_TERM_DUPLEX) && !is_controlling_terminal(term_fd)) { - close(term_fd); + if (term_fd) /* avoid closing stdin */ + close(term_fd); term_fd = -1; return -1; } diff --git a/editor.c b/editor.c index 6c5c95e6a2..192d6ea75d 100644 --- a/editor.c +++ b/editor.c @@ -55,7 +55,7 @@ static int prepare_term(const char *editor) git_config_get_bool("editor.stty", &need_saverestore); if (need_saverestore) - return save_term(1); + return save_term(SAVE_TERM_DUPLEX); return 0; }
Hi Dscho On 10/03/2022 13:25, Johannes Schindelin wrote: > Hi Phillip, > > On Wed, 9 Mar 2022, Phillip Wood wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Thanks to Ramsay and Ævar for their comments on V1. >> Changes since V1: >> * Patch 1 >> - use an enum for save_term()'s flags (suggested by Ævar) >> - fixed argument order in the windows code (thanks to Ramsay) >> * Patch 2 >> - fixed a typo in a comment (thanks to Ramsay) >> * Patch 4 >> - stopped duplicating the strings returned by gettext() (suggested by >> Ævar) >> - reworked error message handling in the signal handler to add an >> "error: " prefix (suggested by Ævar) >> - tweaked the background resume error message > > While I did not ask for any of these changes, they look good to me. I had > a look over the range-diff and found it reasonable. > > Onwards to a bright built-in `add -p` future! Hopefully! I spotted a couple more small issues earlier so there will be a v3 soonish Best Wishes Phillip
On 10/03/2022 13:28, Phillip Wood wrote: > Hi Junio > > On 09/03/2022 23:37, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >>> $ git checkout --detach pw/single-key-interactive >>> HEAD is now at ac618c418e add -p: disable stdin buffering when >>> interactive.singlekey is set >>> $ git am -s ./+pw4-v2-add-p-single >>> Applying: terminal: use flags for save_term() >>> Applying: terminal: don't assume stdin is /dev/tty >>> Applying: terminal: work around macos poll() bug >>> error: patch failed: compat/terminal.c:397 >>> error: compat/terminal.c: patch does not apply >>> Patch failed at 0003 terminal: work around macos poll() bug >>> hint: Use 'git am --show-current-patch=diff' to see the failed patch >>> When you have resolved this problem, run "git am --continue". >>> If you prefer to skip this patch, run "git am --skip" instead. >>> To restore the original branch and stop patching, run "git am --abort". >>> $ exit >> >> I think I figured it out. A merge of pw/single-key-interactive into >> a recent tip of 'master' wants the "return 0" in the preimage below >> to be "break" in compat/terminal.c >> >> >>> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf) >>> * half a second when we know that the sequence is complete. >>> */ >>> while (!is_known_escape_sequence(buf->buf)) { >>> - struct pollfd pfd = { .fd = 0, .events = POLLIN }; >>> - >>> - if (poll(&pfd, 1, 500) < 1) >>> - break; >>> - >>> - ch = getchar(); >>> + ch = getchar_with_timeout(500); >>> if (ch == EOF) >>> return 0; Looking more closely that should be "break" not "return 0". I think what has happened is that I accidentally based these on an old version of pw/single-key-interactive which did not contain 24d7ce383a ("terminal: always reset terminal when reading without echo", 2022-02-22) Sorry for the confusion Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Looking more closely that should be "break" not "return 0". I think > what has happened is that I accidentally based these on an old version > of pw/single-key-interactive which did not contain 24d7ce383a > ("terminal: always reset terminal when reading without echo", > 2022-02-22) I thought the base pw/single-key-interactive topic is solid enough and ready for 'next'? It probably is a good idea to rebase these follow-on patches on top of a merge of the base topic into 'master'. I'll keep v2 in 'seen' but mark the topic as "Expecting a reroll" in the draft of the next issue of the "What's cooking" report. Thanks.