* [PATCH] clean: explicitly `fflush` stdout @ 2020-04-08 19:33 Johannes Schindelin via GitGitGadget 2020-04-08 20:14 ` Jeff King 2020-04-10 11:27 ` [PATCH v2 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 0 siblings, 2 replies; 17+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-04-08 19:33 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, 마누엘 From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de> For performance reasons `stdout` is buffered by default. That leads to problems if after printing to `stdout` a read on `stdin` is performed. For that reason interactive commands like `git clean -i` do not function properly anymore if the `stdout` is not flushed by `fflush(stdout)` before trying to read from `stdin`. So let's precede all reads on `stdin` in `git clean -i` by flushing `stdout`. Signed-off-by: 마누엘 <nalla@hamal.uberspace.de> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Explicitly fflush stdout in git clean This is yet another patch that was funneled through a Git for Windows PR. It has served us well for almost five years now, and it is beyond time that it find its final home in core Git. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-755%2Fdscho%2Ffflush-in-git-clean-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-755/dscho/fflush-in-git-clean-v1 Pull-Request: https://github.com/git/git/pull/755 builtin/clean.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/clean.c b/builtin/clean.c index 5abf087e7c4..2bd06d13395 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -580,6 +580,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff) clean_get_color(CLEAN_COLOR_RESET)); } + fflush(stdout); if (strbuf_getline_lf(&choice, stdin) != EOF) { strbuf_trim(&choice); } else { @@ -662,6 +663,7 @@ static int filter_by_patterns_cmd(void) clean_print_color(CLEAN_COLOR_PROMPT); printf(_("Input ignore patterns>> ")); clean_print_color(CLEAN_COLOR_RESET); + fflush(stdout); if (strbuf_getline_lf(&confirm, stdin) != EOF) strbuf_trim(&confirm); else @@ -760,6 +762,7 @@ static int ask_each_cmd(void) qname = quote_path_relative(item->string, NULL, &buf); /* TRANSLATORS: Make sure to keep [y/N] as is */ printf(_("Remove %s [y/N]? "), qname); + fflush(stdout); if (strbuf_getline_lf(&confirm, stdin) != EOF) { strbuf_trim(&confirm); } else { base-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775 -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] clean: explicitly `fflush` stdout 2020-04-08 19:33 [PATCH] clean: explicitly `fflush` stdout Johannes Schindelin via GitGitGadget @ 2020-04-08 20:14 ` Jeff King 2020-04-08 21:51 ` Junio C Hamano 2020-04-10 14:03 ` Johannes Schindelin 2020-04-10 11:27 ` [PATCH v2 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 1 sibling, 2 replies; 17+ messages in thread From: Jeff King @ 2020-04-08 20:14 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Johannes Schindelin, 마누엘 On Wed, Apr 08, 2020 at 07:33:00PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de> > > For performance reasons `stdout` is buffered by default. That leads to > problems if after printing to `stdout` a read on `stdin` is performed. This first paragraph left me scratching my head. It's only a problem for interactive uses, right? Would: This can lead to problems for interactive commands which write a prompt to `stdout` and then read user input on `stdin`. If the prompt is left in the buffer, the user will not realize the program is waiting for their input. make sense? > For that reason interactive commands like `git clean -i` do not function > properly anymore if the `stdout` is not flushed by `fflush(stdout)` before > trying to read from `stdin`. I'm not sure I understand this "anymore". Did the buffering change at some point, introducing a regression? > So let's precede all reads on `stdin` in `git clean -i` by flushing > `stdout`. This (and the patch) make sense to me. It might be worth factoring out a "read input from user" function and putting the flush there, but with only three affected call sites, I'm OK with it either way. > This is yet another patch that was funneled through a Git for Windows > PR. It has served us well for almost five years now, and it is beyond > time that it find its final home in core Git. Agreed. I guess it didn't affect people on other platforms much because stdout to a terminal is generally line-buffered on POSIX systems. But it's better not to rely on that, plus it could create confusion if somebody tried to manipulate the interactive operation through a pipe (e.g., driving it from a GUI or something). -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] clean: explicitly `fflush` stdout 2020-04-08 20:14 ` Jeff King @ 2020-04-08 21:51 ` Junio C Hamano 2020-04-08 22:38 ` Jeff King 2020-04-10 14:03 ` Johannes Schindelin 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2020-04-08 21:51 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, 마누엘 Jeff King <peff@peff.net> writes: > This (and the patch) make sense to me. It might be worth factoring out a > "read input from user" function and putting the flush there, but with > only three affected call sites, I'm OK with it either way. > >> This is yet another patch that was funneled through a Git for Windows >> PR. It has served us well for almost five years now, and it is beyond >> time that it find its final home in core Git. > > Agreed. I guess it didn't affect people on other platforms much because > stdout to a terminal is generally line-buffered on POSIX systems. But > it's better not to rely on that, plus it could create confusion if > somebody tried to manipulate the interactive operation through a pipe > (e.g., driving it from a GUI or something). Hmph, I thought it was more common to do prompts etc. on the standard error stream, which tends to make the buffering of the output less of a problem, but apparently these prompts are given to the standard output. I am also OK to sprinkle fflush(stdout) but in the longer term, it would probably be a good idea to introduce a helper to "prompt then grab input" or "read user input" (if the former, we'd be able to bring consistency into "which between the standard output or the standard error does a prompt come?", and if the latter we'd do fflush(NULL) before reading), especially if there are many git subcommands that go interactive. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] clean: explicitly `fflush` stdout 2020-04-08 21:51 ` Junio C Hamano @ 2020-04-08 22:38 ` Jeff King 0 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2020-04-08 22:38 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, 마누엘 On Wed, Apr 08, 2020 at 02:51:11PM -0700, Junio C Hamano wrote: > > Agreed. I guess it didn't affect people on other platforms much because > > stdout to a terminal is generally line-buffered on POSIX systems. But > > it's better not to rely on that, plus it could create confusion if > > somebody tried to manipulate the interactive operation through a pipe > > (e.g., driving it from a GUI or something). > > Hmph, I thought it was more common to do prompts etc. on the > standard error stream, which tends to make the buffering of the > output less of a problem, but apparently these prompts are given to > the standard output. The prompts in git-add--interactive.perl go to stdout, as do the ones for "am --interactive". I think that's reasonable, if you think of it as the main output of the program. At one point the "am" ones actually went to /dev/tty, but IMHO that's a mistake. In 99% of cases it behaves the same as looking at stdin/stdout (because they're connected to the terminal anyway), and in the other 1% it's just as likely to be the wrong thing as the right. Plus it makes testing much harder. > I am also OK to sprinkle fflush(stdout) but in > the longer term, it would probably be a good idea to introduce a > helper to "prompt then grab input" or "read user input" (if the > former, we'd be able to bring consistency into "which between the > standard output or the standard error does a prompt come?", and if > the latter we'd do fflush(NULL) before reading), especially if there > are many git subcommands that go interactive. Agreed. There is git_prompt(), but that does the tty thing, which I think we'd want to avoid in most cases. It's used by the credential code, which really does want to use a terminal to do things like turn off character echoing. Plus it's not the main function of the program, but rather a side effect (so we have to avoid disrupting the main stdin/stdout). -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] clean: explicitly `fflush` stdout 2020-04-08 20:14 ` Jeff King 2020-04-08 21:51 ` Junio C Hamano @ 2020-04-10 14:03 ` Johannes Schindelin 1 sibling, 0 replies; 17+ messages in thread From: Johannes Schindelin @ 2020-04-10 14:03 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, 마누엘 Hi Peff, On Wed, 8 Apr 2020, Jeff King wrote: > On Wed, Apr 08, 2020 at 07:33:00PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de> > > > > For performance reasons `stdout` is buffered by default. That leads to > > problems if after printing to `stdout` a read on `stdin` is performed. > > This first paragraph left me scratching my head. It's only a problem for > interactive uses, right? Would: > > This can lead to problems for interactive commands which write a > prompt to `stdout` and then read user input on `stdin`. If the prompt > is left in the buffer, the user will not realize the program is > waiting for their input. > > make sense? Thank you, yes, that makes sense. Based on another suggestion of yours, I did refactor the code a bit more and already sent out the result as v2. Thank you, Dscho > > > For that reason interactive commands like `git clean -i` do not function > > properly anymore if the `stdout` is not flushed by `fflush(stdout)` before > > trying to read from `stdin`. > > I'm not sure I understand this "anymore". Did the buffering change at > some point, introducing a regression? > > > So let's precede all reads on `stdin` in `git clean -i` by flushing > > `stdout`. > > This (and the patch) make sense to me. It might be worth factoring out a > "read input from user" function and putting the flush there, but with > only three affected call sites, I'm OK with it either way. > > > This is yet another patch that was funneled through a Git for Windows > > PR. It has served us well for almost five years now, and it is beyond > > time that it find its final home in core Git. > > Agreed. I guess it didn't affect people on other platforms much because > stdout to a terminal is generally line-buffered on POSIX systems. But > it's better not to rely on that, plus it could create confusion if > somebody tried to manipulate the interactive operation through a pipe > (e.g., driving it from a GUI or something). > > -Peff > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/2] Explicitly fflush stdout in git clean 2020-04-08 19:33 [PATCH] clean: explicitly `fflush` stdout Johannes Schindelin via GitGitGadget 2020-04-08 20:14 ` Jeff King @ 2020-04-10 11:27 ` Johannes Schindelin via GitGitGadget 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget ` (2 more replies) 1 sibling, 3 replies; 17+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-04-10 11:27 UTC (permalink / raw) To: git; +Cc: Jeff King, Johannes Schindelin This is yet another patch that was funneled through a Git for Windows PR, read: it has been battle-tested. Of course, the current iteration of this patch series has not been battle-tested, so please do review carefully, so that we can prevent bugs from slipping in during the review process. Changes since v1: * Added a preparatory patch that refactors all interactive input reading via strbuf_getline(..., stdin). * Adjusted the patch and its commit message accordingly. Johannes Schindelin (1): Refactor code asking the user for input interactively 마누엘 (1): Explicitly `fflush` stdout before expecting interactive input add-interactive.c | 4 ++-- add-patch.c | 4 ++-- builtin/clean.c | 14 ++++---------- prompt.c | 12 ++++++++++++ prompt.h | 2 ++ shell.c | 4 ++-- 6 files changed, 24 insertions(+), 16 deletions(-) base-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-755%2Fdscho%2Ffflush-in-git-clean-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-755/dscho/fflush-in-git-clean-v2 Pull-Request: https://github.com/git/git/pull/755 Range-diff vs v1: 1: 21acd883b94 < -: ----------- clean: explicitly `fflush` stdout -: ----------- > 1: 9d2ee78a9e4 Refactor code asking the user for input interactively -: ----------- > 2: d3949e42004 Explicitly `fflush` stdout before expecting interactive input -- gitgitgadget ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] Refactor code asking the user for input interactively 2020-04-10 11:27 ` [PATCH v2 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget @ 2020-04-10 11:27 ` Johannes Schindelin via GitGitGadget 2020-04-10 12:27 ` Derrick Stolee ` (2 more replies) 2020-04-10 11:27 ` [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2 siblings, 3 replies; 17+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-04-10 11:27 UTC (permalink / raw) To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> There are quite a few code locations (e.g. `git clean --interactive`) where Git asks the user for an answer. In preparation for fixing a bug shared by all of them, and also to DRY up the code, let's refactor it. Please note that most of these callers trimmed white-space both at the beginning and at the end of the answer, instead of trimming only the end (as the caller in `add-patch.c` does). THerefore, technically speaking, we change behavior in this patch. At the same time, it can be argued that this is actually a bug fix. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- add-interactive.c | 4 ++-- add-patch.c | 4 ++-- builtin/clean.c | 14 ++++---------- prompt.c | 10 ++++++++++ prompt.h | 2 ++ shell.c | 4 ++-- 6 files changed, 22 insertions(+), 16 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 4a9bf85cac0..29cd2fe0201 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -9,6 +9,7 @@ #include "lockfile.h" #include "dir.h" #include "run-command.h" +#include "prompt.h" static void init_color(struct repository *r, struct add_i_state *s, const char *slot_name, char *dst, @@ -289,13 +290,12 @@ static ssize_t list_and_choose(struct add_i_state *s, fputs(singleton ? "> " : ">> ", stdout); fflush(stdout); - if (strbuf_getline(&input, stdin) == EOF) { + if (git_read_line_interactively(&input) == EOF) { putchar('\n'); if (immediate) res = LIST_AND_CHOOSE_QUIT; break; } - strbuf_trim(&input); if (!input.len) break; diff --git a/add-patch.c b/add-patch.c index d8dafa8168d..d8bfe379be4 100644 --- a/add-patch.c +++ b/add-patch.c @@ -7,6 +7,7 @@ #include "color.h" #include "diff.h" #include "compat/terminal.h" +#include "prompt.h" enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK, @@ -1158,9 +1159,8 @@ static int read_single_character(struct add_p_state *s) return res; } - if (strbuf_getline(&s->answer, stdin) == EOF) + if (git_read_line_interactively(&s->answer) == EOF) return EOF; - strbuf_trim_trailing_newline(&s->answer); return 0; } diff --git a/builtin/clean.c b/builtin/clean.c index 5abf087e7c4..c8c011d2ddf 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -18,6 +18,7 @@ #include "color.h" #include "pathspec.h" #include "help.h" +#include "prompt.h" static int force = -1; /* unset */ static int interactive; @@ -420,7 +421,6 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff) return found; } - /* * Parse user input, and return choice(s) for menu (menu_stuff). * @@ -580,9 +580,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff) clean_get_color(CLEAN_COLOR_RESET)); } - if (strbuf_getline_lf(&choice, stdin) != EOF) { - strbuf_trim(&choice); - } else { + if (git_read_line_interactively(&choice) == EOF) { eof = 1; break; } @@ -662,9 +660,7 @@ static int filter_by_patterns_cmd(void) clean_print_color(CLEAN_COLOR_PROMPT); printf(_("Input ignore patterns>> ")); clean_print_color(CLEAN_COLOR_RESET); - if (strbuf_getline_lf(&confirm, stdin) != EOF) - strbuf_trim(&confirm); - else + if (git_read_line_interactively(&confirm) == EOF) putchar('\n'); /* quit filter_by_pattern mode if press ENTER or Ctrl-D */ @@ -760,9 +756,7 @@ static int ask_each_cmd(void) qname = quote_path_relative(item->string, NULL, &buf); /* TRANSLATORS: Make sure to keep [y/N] as is */ printf(_("Remove %s [y/N]? "), qname); - if (strbuf_getline_lf(&confirm, stdin) != EOF) { - strbuf_trim(&confirm); - } else { + if (git_read_line_interactively(&confirm) == EOF) { putchar('\n'); eof = 1; } diff --git a/prompt.c b/prompt.c index 6d5885d0096..098dcfb7cf9 100644 --- a/prompt.c +++ b/prompt.c @@ -74,3 +74,13 @@ char *git_prompt(const char *prompt, int flags) } return r; } + +int git_read_line_interactively(struct strbuf *line) +{ + int ret = strbuf_getline_lf(line, stdin); + + if (ret != EOF) + strbuf_trim_trailing_newline(line); + + return ret; +} diff --git a/prompt.h b/prompt.h index e04cced030c..e294e93541c 100644 --- a/prompt.h +++ b/prompt.h @@ -6,4 +6,6 @@ char *git_prompt(const char *prompt, int flags); +int git_read_line_interactively(struct strbuf *line); + #endif /* PROMPT_H */ diff --git a/shell.c b/shell.c index 54cca7439de..cef7ffdc9e1 100644 --- a/shell.c +++ b/shell.c @@ -4,6 +4,7 @@ #include "strbuf.h" #include "run-command.h" #include "alias.h" +#include "prompt.h" #define COMMAND_DIR "git-shell-commands" #define HELP_COMMAND COMMAND_DIR "/help" @@ -76,12 +77,11 @@ static void run_shell(void) int count; fprintf(stderr, "git> "); - if (strbuf_getline_lf(&line, stdin) == EOF) { + if (git_read_line_interactively(&line) == EOF) { fprintf(stderr, "\n"); strbuf_release(&line); break; } - strbuf_trim(&line); rawargs = strbuf_detach(&line, NULL); split_args = xstrdup(rawargs); count = split_cmdline(split_args, &argv); -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] Refactor code asking the user for input interactively 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget @ 2020-04-10 12:27 ` Derrick Stolee 2020-04-10 14:01 ` Johannes Schindelin 2020-04-10 15:07 ` Jeff King 2020-04-10 17:44 ` Junio C Hamano 2 siblings, 1 reply; 17+ messages in thread From: Derrick Stolee @ 2020-04-10 12:27 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, git; +Cc: Jeff King, Johannes Schindelin On 4/10/2020 7:27 AM, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > There are quite a few code locations (e.g. `git clean --interactive`) > where Git asks the user for an answer. In preparation for fixing a bug > shared by all of them, and also to DRY up the code, let's refactor it. > > Please note that most of these callers trimmed white-space both at the > beginning and at the end of the answer, instead of trimming only the > end (as the caller in `add-patch.c` does). add-patch also only trims the newline! This is still a good change. > THerefore, technically speaking, we change behavior in this patch. At Strange capitalization here. > the same time, it can be argued that this is actually a bug fix. > > @@ -1158,9 +1159,8 @@ static int read_single_character(struct add_p_state *s) > return res; > } > > - if (strbuf_getline(&s->answer, stdin) == EOF) > + if (git_read_line_interactively(&s->answer) == EOF) > return EOF; > - strbuf_trim_trailing_newline(&s->answer); (Pointing out the trailing newline trim here.) > + > +int git_read_line_interactively(struct strbuf *line) > +{ > + int ret = strbuf_getline_lf(line, stdin); > + > + if (ret != EOF) > + strbuf_trim_trailing_newline(line); > + > + return ret; > +} This looks good. Do we need a trailing newline or something? The way the diff ends abruptly after the "}" line made me think so. Thanks, -Stolee ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] Refactor code asking the user for input interactively 2020-04-10 12:27 ` Derrick Stolee @ 2020-04-10 14:01 ` Johannes Schindelin 0 siblings, 0 replies; 17+ messages in thread From: Johannes Schindelin @ 2020-04-10 14:01 UTC (permalink / raw) To: Derrick Stolee; +Cc: Johannes Schindelin via GitGitGadget, git, Jeff King Hi Dr Stolee, On Fri, 10 Apr 2020, Derrick Stolee wrote: > On 4/10/2020 7:27 AM, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > There are quite a few code locations (e.g. `git clean --interactive`) > > where Git asks the user for an answer. In preparation for fixing a bug > > shared by all of them, and also to DRY up the code, let's refactor it. > > > > Please note that most of these callers trimmed white-space both at the > > beginning and at the end of the answer, instead of trimming only the > > end (as the caller in `add-patch.c` does). > > add-patch also only trims the newline! This is still a good change. > > > THerefore, technically speaking, we change behavior in this patch. At > > Strange capitalization here. I fixed it and force-pushed, waiting a little with submitting it in case there are more things reviewers point out. > > the same time, it can be argued that this is actually a bug fix. > > > > > @@ -1158,9 +1159,8 @@ static int read_single_character(struct add_p_state *s) > > return res; > > } > > > > - if (strbuf_getline(&s->answer, stdin) == EOF) > > + if (git_read_line_interactively(&s->answer) == EOF) > > return EOF; > > - strbuf_trim_trailing_newline(&s->answer); > > (Pointing out the trailing newline trim here.) > > > + > > +int git_read_line_interactively(struct strbuf *line) > > +{ > > + int ret = strbuf_getline_lf(line, stdin); > > + > > + if (ret != EOF) > > + strbuf_trim_trailing_newline(line); > > + > > + return ret; > > +} > > This looks good. Do we need a trailing newline or something? > The way the diff ends abruptly after the "}" line made me > think so. No, this file ends in that line, therefore it ends abruptly ;-) If I add another newline, `git diff` shows a red `+` at the end, i.e. we consider it a white-space problem. Thank you for your review! Dscho > > Thanks, > -Stolee > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] Refactor code asking the user for input interactively 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget 2020-04-10 12:27 ` Derrick Stolee @ 2020-04-10 15:07 ` Jeff King 2020-04-10 17:44 ` Junio C Hamano 2 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2020-04-10 15:07 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin On Fri, Apr 10, 2020 at 11:27:50AM +0000, Johannes Schindelin via GitGitGadget wrote: > diff --git a/prompt.h b/prompt.h > index e04cced030c..e294e93541c 100644 > --- a/prompt.h > +++ b/prompt.h > @@ -6,4 +6,6 @@ > > char *git_prompt(const char *prompt, int flags); > > +int git_read_line_interactively(struct strbuf *line); > + It might be worth adding some comments discussing why one would use git_prompt() versus git_read_line_interactively(). Other than that, both patches look good to me. Thanks for calling out the changed trimming behavior preemptively. I agree it should not be a big deal either way. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] Refactor code asking the user for input interactively 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget 2020-04-10 12:27 ` Derrick Stolee 2020-04-10 15:07 ` Jeff King @ 2020-04-10 17:44 ` Junio C Hamano 2 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2020-04-10 17:44 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Jeff King, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > - if (strbuf_getline(&input, stdin) == EOF) { > + if (git_read_line_interactively(&input) == EOF) { It's not like we are mimicking or giving a thin wrapper to improve an existing read_line_interactively() from a third-party, so I do not see much point in giving "git_" prefix here. On the other hand, "strbuf_" prefix may not hurt (but the type of its first parameter is sufficient so it is not exactly required). > printf(_("Remove %s [y/N]? "), qname); > - if (strbuf_getline_lf(&confirm, stdin) != EOF) { > - strbuf_trim(&confirm); > - } else { > + if (git_read_line_interactively(&confirm) == EOF) { > putchar('\n'); > eof = 1; A fat-finger that gave an answer " yes <RET>" used to be still taken as a yes but now it is interpreted as "no", because the new helper trims a lot less. In general, the existing code should be already choosing the safer default, so such a change in behaviour brought in by this change, even if they were not intentional, should probably be safe. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input 2020-04-10 11:27 ` [PATCH v2 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget @ 2020-04-10 11:27 ` 마누엘 via GitGitGadget 2020-04-10 12:28 ` Derrick Stolee 2020-04-10 18:16 ` [PATCH v3 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2 siblings, 1 reply; 17+ messages in thread From: 마누엘 via GitGitGadget @ 2020-04-10 11:27 UTC (permalink / raw) To: git; +Cc: Jeff King, Johannes Schindelin, 마누엘 From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de> At least one interactive command writes a prompt to `stdout` and then reads user input on `stdin`: `git clean --interactive`. If the prompt is left in the buffer, the user will not realize the program is waiting for their input. So let's just flush `stdout` before reading the user's input. Signed-off-by: 마누엘 <nalla@hamal.uberspace.de> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- prompt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/prompt.c b/prompt.c index 098dcfb7cf9..5ded21a017f 100644 --- a/prompt.c +++ b/prompt.c @@ -77,8 +77,10 @@ char *git_prompt(const char *prompt, int flags) int git_read_line_interactively(struct strbuf *line) { - int ret = strbuf_getline_lf(line, stdin); + int ret; + fflush(stdout); + ret = strbuf_getline_lf(line, stdin); if (ret != EOF) strbuf_trim_trailing_newline(line); -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input 2020-04-10 11:27 ` [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget @ 2020-04-10 12:28 ` Derrick Stolee 0 siblings, 0 replies; 17+ messages in thread From: Derrick Stolee @ 2020-04-10 12:28 UTC (permalink / raw) To: 마누엘 via GitGitGadget, git Cc: Jeff King, Johannes Schindelin, 마누엘 On 4/10/2020 7:27 AM, 마누엘 via GitGitGadget wrote: > From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de> > > At least one interactive command writes a prompt to `stdout` and then > reads user input on `stdin`: `git clean --interactive`. If the prompt is > left in the buffer, the user will not realize the program is waiting for > their input. > > So let's just flush `stdout` before reading the user's input. > > Signed-off-by: 마누엘 <nalla@hamal.uberspace.de> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > prompt.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/prompt.c b/prompt.c > index 098dcfb7cf9..5ded21a017f 100644 > --- a/prompt.c > +++ b/prompt.c > @@ -77,8 +77,10 @@ char *git_prompt(const char *prompt, int flags) > > int git_read_line_interactively(struct strbuf *line) > { > - int ret = strbuf_getline_lf(line, stdin); > + int ret; > > + fflush(stdout); > + ret = strbuf_getline_lf(line, stdin); > if (ret != EOF) > strbuf_trim_trailing_newline(line); Nice and "clean" ;) LGTM -Stolee ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 0/2] Explicitly fflush stdout in git clean 2020-04-10 11:27 ` [PATCH v2 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget 2020-04-10 11:27 ` [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget @ 2020-04-10 18:16 ` Johannes Schindelin via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget 2 siblings, 2 replies; 17+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-04-10 18:16 UTC (permalink / raw) To: git; +Cc: Jeff King, Derrick Stolee, Johannes Schindelin This is yet another patch that was funneled through a Git for Windows PR, read: it has been battle-tested. Of course, the current iteration of this patch series has not been battle-tested, so please do review carefully, so that we can prevent bugs from slipping in during the review process. Changes since v2: * Fixed the capitalization of "THerefore" in the commit message. Changes since v1: * Added a preparatory patch that refactors all interactive input reading via strbuf_getline(..., stdin). * Adjusted the patch and its commit message accordingly. Johannes Schindelin (1): Refactor code asking the user for input interactively 마누엘 (1): Explicitly `fflush` stdout before expecting interactive input add-interactive.c | 4 ++-- add-patch.c | 4 ++-- builtin/clean.c | 14 ++++---------- prompt.c | 12 ++++++++++++ prompt.h | 2 ++ shell.c | 4 ++-- 6 files changed, 24 insertions(+), 16 deletions(-) base-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-755%2Fdscho%2Ffflush-in-git-clean-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-755/dscho/fflush-in-git-clean-v3 Pull-Request: https://github.com/git/git/pull/755 Range-diff vs v2: 1: 9d2ee78a9e4 ! 1: 072f95090ae Refactor code asking the user for input interactively @@ Commit message beginning and at the end of the answer, instead of trimming only the end (as the caller in `add-patch.c` does). - THerefore, technically speaking, we change behavior in this patch. At + Therefore, technically speaking, we change behavior in this patch. At the same time, it can be argued that this is actually a bug fix. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 2: d3949e42004 = 2: 063d2aaa401 Explicitly `fflush` stdout before expecting interactive input -- gitgitgadget ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] Refactor code asking the user for input interactively 2020-04-10 18:16 ` [PATCH v3 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget @ 2020-04-10 18:16 ` Johannes Schindelin via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget 1 sibling, 0 replies; 17+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-04-10 18:16 UTC (permalink / raw) To: git; +Cc: Jeff King, Derrick Stolee, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> There are quite a few code locations (e.g. `git clean --interactive`) where Git asks the user for an answer. In preparation for fixing a bug shared by all of them, and also to DRY up the code, let's refactor it. Please note that most of these callers trimmed white-space both at the beginning and at the end of the answer, instead of trimming only the end (as the caller in `add-patch.c` does). Therefore, technically speaking, we change behavior in this patch. At the same time, it can be argued that this is actually a bug fix. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- add-interactive.c | 4 ++-- add-patch.c | 4 ++-- builtin/clean.c | 14 ++++---------- prompt.c | 10 ++++++++++ prompt.h | 2 ++ shell.c | 4 ++-- 6 files changed, 22 insertions(+), 16 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 4a9bf85cac0..29cd2fe0201 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -9,6 +9,7 @@ #include "lockfile.h" #include "dir.h" #include "run-command.h" +#include "prompt.h" static void init_color(struct repository *r, struct add_i_state *s, const char *slot_name, char *dst, @@ -289,13 +290,12 @@ static ssize_t list_and_choose(struct add_i_state *s, fputs(singleton ? "> " : ">> ", stdout); fflush(stdout); - if (strbuf_getline(&input, stdin) == EOF) { + if (git_read_line_interactively(&input) == EOF) { putchar('\n'); if (immediate) res = LIST_AND_CHOOSE_QUIT; break; } - strbuf_trim(&input); if (!input.len) break; diff --git a/add-patch.c b/add-patch.c index d8dafa8168d..d8bfe379be4 100644 --- a/add-patch.c +++ b/add-patch.c @@ -7,6 +7,7 @@ #include "color.h" #include "diff.h" #include "compat/terminal.h" +#include "prompt.h" enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK, @@ -1158,9 +1159,8 @@ static int read_single_character(struct add_p_state *s) return res; } - if (strbuf_getline(&s->answer, stdin) == EOF) + if (git_read_line_interactively(&s->answer) == EOF) return EOF; - strbuf_trim_trailing_newline(&s->answer); return 0; } diff --git a/builtin/clean.c b/builtin/clean.c index 5abf087e7c4..c8c011d2ddf 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -18,6 +18,7 @@ #include "color.h" #include "pathspec.h" #include "help.h" +#include "prompt.h" static int force = -1; /* unset */ static int interactive; @@ -420,7 +421,6 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff) return found; } - /* * Parse user input, and return choice(s) for menu (menu_stuff). * @@ -580,9 +580,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff) clean_get_color(CLEAN_COLOR_RESET)); } - if (strbuf_getline_lf(&choice, stdin) != EOF) { - strbuf_trim(&choice); - } else { + if (git_read_line_interactively(&choice) == EOF) { eof = 1; break; } @@ -662,9 +660,7 @@ static int filter_by_patterns_cmd(void) clean_print_color(CLEAN_COLOR_PROMPT); printf(_("Input ignore patterns>> ")); clean_print_color(CLEAN_COLOR_RESET); - if (strbuf_getline_lf(&confirm, stdin) != EOF) - strbuf_trim(&confirm); - else + if (git_read_line_interactively(&confirm) == EOF) putchar('\n'); /* quit filter_by_pattern mode if press ENTER or Ctrl-D */ @@ -760,9 +756,7 @@ static int ask_each_cmd(void) qname = quote_path_relative(item->string, NULL, &buf); /* TRANSLATORS: Make sure to keep [y/N] as is */ printf(_("Remove %s [y/N]? "), qname); - if (strbuf_getline_lf(&confirm, stdin) != EOF) { - strbuf_trim(&confirm); - } else { + if (git_read_line_interactively(&confirm) == EOF) { putchar('\n'); eof = 1; } diff --git a/prompt.c b/prompt.c index 6d5885d0096..098dcfb7cf9 100644 --- a/prompt.c +++ b/prompt.c @@ -74,3 +74,13 @@ char *git_prompt(const char *prompt, int flags) } return r; } + +int git_read_line_interactively(struct strbuf *line) +{ + int ret = strbuf_getline_lf(line, stdin); + + if (ret != EOF) + strbuf_trim_trailing_newline(line); + + return ret; +} diff --git a/prompt.h b/prompt.h index e04cced030c..e294e93541c 100644 --- a/prompt.h +++ b/prompt.h @@ -6,4 +6,6 @@ char *git_prompt(const char *prompt, int flags); +int git_read_line_interactively(struct strbuf *line); + #endif /* PROMPT_H */ diff --git a/shell.c b/shell.c index 54cca7439de..cef7ffdc9e1 100644 --- a/shell.c +++ b/shell.c @@ -4,6 +4,7 @@ #include "strbuf.h" #include "run-command.h" #include "alias.h" +#include "prompt.h" #define COMMAND_DIR "git-shell-commands" #define HELP_COMMAND COMMAND_DIR "/help" @@ -76,12 +77,11 @@ static void run_shell(void) int count; fprintf(stderr, "git> "); - if (strbuf_getline_lf(&line, stdin) == EOF) { + if (git_read_line_interactively(&line) == EOF) { fprintf(stderr, "\n"); strbuf_release(&line); break; } - strbuf_trim(&line); rawargs = strbuf_detach(&line, NULL); split_args = xstrdup(rawargs); count = split_cmdline(split_args, &argv); -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 2/2] Explicitly `fflush` stdout before expecting interactive input 2020-04-10 18:16 ` [PATCH v3 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget @ 2020-04-10 18:16 ` 마누엘 via GitGitGadget 1 sibling, 0 replies; 17+ messages in thread From: 마누엘 via GitGitGadget @ 2020-04-10 18:16 UTC (permalink / raw) To: git Cc: Jeff King, Derrick Stolee, Johannes Schindelin, 마누엘 From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de> At least one interactive command writes a prompt to `stdout` and then reads user input on `stdin`: `git clean --interactive`. If the prompt is left in the buffer, the user will not realize the program is waiting for their input. So let's just flush `stdout` before reading the user's input. Signed-off-by: 마누엘 <nalla@hamal.uberspace.de> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- prompt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/prompt.c b/prompt.c index 098dcfb7cf9..5ded21a017f 100644 --- a/prompt.c +++ b/prompt.c @@ -77,8 +77,10 @@ char *git_prompt(const char *prompt, int flags) int git_read_line_interactively(struct strbuf *line) { - int ret = strbuf_getline_lf(line, stdin); + int ret; + fflush(stdout); + ret = strbuf_getline_lf(line, stdin); if (ret != EOF) strbuf_trim_trailing_newline(line); -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] Explicitly fflush stdout in git clean @ 2020-04-10 12:46 Lia Rosas 0 siblings, 0 replies; 17+ messages in thread From: Lia Rosas @ 2020-04-10 12:46 UTC (permalink / raw) To: gitgitgadget; +Cc: git, johannes.schindelin, peff Sent from my iPhone ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-04-10 18:17 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-08 19:33 [PATCH] clean: explicitly `fflush` stdout Johannes Schindelin via GitGitGadget 2020-04-08 20:14 ` Jeff King 2020-04-08 21:51 ` Junio C Hamano 2020-04-08 22:38 ` Jeff King 2020-04-10 14:03 ` Johannes Schindelin 2020-04-10 11:27 ` [PATCH v2 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2020-04-10 11:27 ` [PATCH v2 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget 2020-04-10 12:27 ` Derrick Stolee 2020-04-10 14:01 ` Johannes Schindelin 2020-04-10 15:07 ` Jeff King 2020-04-10 17:44 ` Junio C Hamano 2020-04-10 11:27 ` [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget 2020-04-10 12:28 ` Derrick Stolee 2020-04-10 18:16 ` [PATCH v3 0/2] Explicitly fflush stdout in git clean Johannes Schindelin via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 1/2] Refactor code asking the user for input interactively Johannes Schindelin via GitGitGadget 2020-04-10 18:16 ` [PATCH v3 2/2] Explicitly `fflush` stdout before expecting interactive input 마누엘 via GitGitGadget 2020-04-10 12:46 [PATCH v2 0/2] Explicitly fflush stdout in git clean Lia Rosas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).