clean: explicitly `fflush` stdout

Commit Message

John Passaro via GitGitGadget April 8, 2020, 7:33 p.m. UTC

From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <[e-mail]>

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: 마누엘 <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
    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(+)


base-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775

Comments

Jeff King April 8, 2020, 8:14 p.m. UTC | #1

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?= <[e-mail]>
> 
> 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

Junio C Hamano April 8, 2020, 9:51 p.m. UTC | #2

Jeff King <[e-mail]> 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.

Jeff King April 8, 2020, 10:38 p.m. UTC | #3

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

Johannes Schindelin April 10, 2020, 2:03 p.m. UTC | #4

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?= <[e-mail]>
> >
> > 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
>

Patch

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 {