Skip to contentSkip to conversation

clean: explicitly `fflush` stdout

PATCH Superseded Johannes Schindelin via GitGitGadget 5 participants 16 messages Apr 8, 2020 – Apr 10, 2020

lore
Share & tools
lore

Johannes Schindelin via GitGitGadget

author
#0 lore raw

PATCH 1/1 clean: explicitly `fflush` stdout

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

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 마누엘 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
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(+)
builtin/clean.c +3 -0
index 5abf087e7c4..2bd06d13395 100644
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 4 lines · Johannes Schindelin via GitGitGadget

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

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?

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 3 lines

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

#2 lore raw
Quoted 8 lines · Jeff King

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

#3 lore raw
Quoted 10 lines · Junio C Hamano
Quoted 5 lines

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.

Quoted 7 lines

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

#4 lore raw
In reply to Jeff King

Hi Peff,

Quoted 10 lines · Jeff King
Quoted 4 lines · Johannes Schindelin via GitGitGadget

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

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:

Quoted 6 lines

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

Quoted 27 lines
Quoted 4 lines

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?

Quoted 2 lines

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.

Quoted 3 lines

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

author
#5 lore raw

v2 cover Explicitly fflush stdout in git clean

In reply to Johannes Schindelin via GitGitGadget

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
Signature · 1 line
gitgitgadget
author
#6 lore raw

PATCH v2 1/2 Refactor code asking the user for input interactively

From: Johannes Schindelin <[email protected]>

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 <[email protected]>
---
 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(-)
add-interactive.c +2 -2
index 4a9bf85cac0..29cd2fe0201 100644
@@ -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;
add-patch.c +2 -2
index d8dafa8168d..d8bfe379be4 100644
@@ -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;
 }
builtin/clean.c +4 -10
index 5abf087e7c4..c8c011d2ddf 100644
@@ -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;
 			}
prompt.c +10 -0
index 6d5885d0096..098dcfb7cf9 100644
@@ -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;
+}
prompt.h +2 -0
index e04cced030c..e294e93541c 100644
@@ -6,4 +6,6 @@
 
 char *git_prompt(const char *prompt, int flags);
 
+int git_read_line_interactively(struct strbuf *line);
+
 #endif /* PROMPT_H */
shell.c +2 -2
index 54cca7439de..cef7ffdc9e1 100644
@@ -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);
Signature · 1 line
gitgitgadget
#7 lore raw

Re: [PATCH v2 1/2] Refactor code asking the user for input interactively

Quoted 9 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Quoted 1 line

THerefore, technically speaking, we change behavior in this patch. At

Strange capitalization here.

add-patch.c static int read_single_character(struct add_p_state *s) · line 1163

Quoted 11 lines

the same time, it can be argued that this is actually a bug fix.

add-patch.c +1 -2
@@ -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.)

prompt.c char *git_prompt(const char *prompt, int flags) · line 86

Quoted 10 lines
prompt.c +10 -0
+
+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

#8 lore raw

Re: [PATCH v2 1/2] Refactor code asking the user for input interactively

Hi Dr Stolee,

Quoted 11 lines · Derrick Stolee
Quoted 9 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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).

Quoted 5 lines

add-patch also only trims the newline! This is still a good change.

Quoted 1 line

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.

Quoted 14 lines

add-patch.c static int read_single_character(struct add_p_state *s) · line 1163

Quoted 11 lines

the same time, it can be argued that this is actually a bug fix.

add-patch.c +1 -2
@@ -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.)

Quoted 14 lines

prompt.c char *git_prompt(const char *prompt, int flags) · line 86

Quoted 10 lines
prompt.c +10 -0
+
+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

Quoted 5 lines

Thanks,
-Stolee

#9 lore raw

Re: [PATCH v2 1/2] Refactor code asking the user for input interactively

In reply to Johannes Schindelin via GitGitGadget

prompt.h line 10

Quoted 10 lines · Johannes Schindelin via GitGitGadget
prompt.h +2 -0
index e04cced030c..e294e93541c 100644
@@ -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

#10 lore raw

Re: [PATCH v2 1/2] Refactor code asking the user for input interactively

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

add-interactive.c static ssize_t list_and_choose(struct add_i_state *s, · line 293

Quoted 2 lines · writes:
add-interactive.c +1 -1
-		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).

builtin/clean.c static int ask_each_cmd(void) · line 761

Quoted 7 lines
builtin/clean.c +1 -3
 			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.

author
#11 lore raw

PATCH v2 2/2 Explicitly `fflush` stdout before expecting interactive input

In reply to Johannes Schindelin via GitGitGadget

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

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 마누엘 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
 prompt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
prompt.c +3 -1
index 098dcfb7cf9..5ded21a017f 100644
@@ -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);
Signature · 1 line
gitgitgadget
#12 lore raw

Re: [PATCH v2 2/2] Explicitly `fflush` stdout before expecting interactive input

Quoted 20 lines · 마누엘 via GitGitGadget

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

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 마누엘 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
 prompt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
prompt.c +0 -0
index 098dcfb7cf9..5ded21a017f 100644

prompt.c char *git_prompt(const char *prompt, int flags) · line 85

Quoted 10 lines
prompt.c +3 -1
@@ -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

author
#13 lore raw

v3 cover Explicitly fflush stdout in git clean

In reply to Johannes Schindelin via GitGitGadget

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 <[email protected]>

2: d3949e42004 = 2: 063d2aaa401 Explicitly fflush stdout before expecting interactive input

Signature · 1 line
gitgitgadget
author
#14 lore raw

PATCH v3 1/2 Refactor code asking the user for input interactively

From: Johannes Schindelin <[email protected]>

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 <[email protected]>
---
 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(-)
add-interactive.c +2 -2
index 4a9bf85cac0..29cd2fe0201 100644
@@ -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;
add-patch.c +2 -2
index d8dafa8168d..d8bfe379be4 100644
@@ -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;
 }
builtin/clean.c +4 -10
index 5abf087e7c4..c8c011d2ddf 100644
@@ -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;
 			}
prompt.c +10 -0
index 6d5885d0096..098dcfb7cf9 100644
@@ -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;
+}
prompt.h +2 -0
index e04cced030c..e294e93541c 100644
@@ -6,4 +6,6 @@
 
 char *git_prompt(const char *prompt, int flags);
 
+int git_read_line_interactively(struct strbuf *line);
+
 #endif /* PROMPT_H */
shell.c +2 -2
index 54cca7439de..cef7ffdc9e1 100644
@@ -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);
Signature · 1 line
gitgitgadget
author
#15 lore raw

PATCH v3 2/2 Explicitly `fflush` stdout before expecting interactive input

In reply to Johannes Schindelin via GitGitGadget

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

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 마누엘 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
 prompt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
prompt.c +3 -1
index 098dcfb7cf9..5ded21a017f 100644
@@ -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);
Signature · 1 line
gitgitgadget