| Message ID | 20210506165102.123739-2-firminmartin24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch: introduce --confirm-overwrite | expand |
Firmin Martin <firminmartin24@gmail.com> writes: > Currently, git_prompt ignores input coming from anywhere other than > terminal (pipe, redirection etc.) meaning that standard prompt > auto-answering methods would have no effect: > > echo 'Y' | git ... > yes 'Y' | git ... > git ... <input.txt > > It also prevents git subcommands using git_prompt to be tested using > such methods. For testing, wouldn't lib-terminal.sh be usable for your purpose? If not, what is the reason why it is insufficient? Can we fix that instead? Allowing prompter to read from pipe has a big downside in the production code: you cannot pipe data into our command, and let it ask interactive questions from the end user by opening /dev/tty. > This patch fixes this issue by considering standard input when !isatty(0). > It also rearranges the control flow to close input and output file handlers. So this "fix" is probably very unwelcome, especially if done unconditionally. Thanks.
On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote: > Firmin Martin <firminmartin24@gmail.com> writes: > > > Currently, git_prompt ignores input coming from anywhere other than > > terminal (pipe, redirection etc.) meaning that standard prompt > > auto-answering methods would have no effect: > > > > echo 'Y' | git ... > > yes 'Y' | git ... > > git ... <input.txt > > > > It also prevents git subcommands using git_prompt to be tested using > > such methods. > > For testing, wouldn't lib-terminal.sh be usable for your purpose? > If not, what is the reason why it is insufficient? Can we fix that > instead? That doesn't work, because it insists on reading from /dev/tty and not the pty that lib-terminal will set up as stdin. But... > Allowing prompter to read from pipe has a big downside in the > production code: you cannot pipe data into our command, and let it > ask interactive questions from the end user by opening /dev/tty. Right. The main purpose of the function was to let git-remote-https, whose stdin is connected to git-fetch, get a password from the user. Reading from stdin would break things badly there[1]. Looking at the second patch, the motivation here seems to be to use git_prompt() for another run-of-the-mill prompt. But the right answer is: don't do that. In fact, we recently-ish removed a similar case in 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was likewise causing problems with the test suite. I think we might consider renaming git_prompt(), or adding an explanatory comment above it. -Peff [1] Sadly I don't think our test suite could notice the breakage introduced by this function. It uses the askpass feature to avoid triggering this code at all, because of course we can not reliably read from /dev/tty in the script. But with just this patch applied, and no credential helpers defined, trying "git ls-remote https://github.com/you/some-private-repo" shows the problem: you get prompted, but it never reads your input.
Jeff King <peff@peff.net> writes: >> For testing, wouldn't lib-terminal.sh be usable for your purpose? >> If not, what is the reason why it is insufficient? Can we fix that >> instead? > > That doesn't work, because it insists on reading from /dev/tty and not > the pty that lib-terminal will set up as stdin. But... I somehow thought that lib-terminal was a bit more complete in that it would make the pty we allocate to the controlling terminal for the process that runs the test program. Sigh.
Jeff King <peff@peff.net> writes: Hi Peff, > On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote: > >> Firmin Martin <firminmartin24@gmail.com> writes: >> >> > Currently, git_prompt ignores input coming from anywhere other than >> > terminal (pipe, redirection etc.) meaning that standard prompt >> > auto-answering methods would have no effect: >> > >> > echo 'Y' | git ... >> > yes 'Y' | git ... >> > git ... <input.txt >> > >> > It also prevents git subcommands using git_prompt to be tested using >> > such methods. >> >> For testing, wouldn't lib-terminal.sh be usable for your purpose? >> If not, what is the reason why it is insufficient? Can we fix that >> instead? > > That doesn't work, because it insists on reading from /dev/tty and not > the pty that lib-terminal will set up as stdin. But... > >> Allowing prompter to read from pipe has a big downside in the >> production code: you cannot pipe data into our command, and let it >> ask interactive questions from the end user by opening /dev/tty. > > Right. The main purpose of the function was to let git-remote-https, > whose stdin is connected to git-fetch, get a password from the user. > Reading from stdin would break things badly there[1]. > > Looking at the second patch, the motivation here seems to be to use > git_prompt() for another run-of-the-mill prompt. But the right answer > is: don't do that. In fact, we recently-ish removed a similar case in > 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was > likewise causing problems with the test suite. I actually inspired myself from the two occurrences of git_prompt in builtin/bisect--helper.c introduced in 09535f056b (bisect--helper: reimplement `bisect_autostart` shell function in C, 2020-09-24). Not sure if they should also be converted to a simple fgets. I will do that in the v2 of this series if the prompt is still proven useful. > > I think we might consider renaming git_prompt(), or adding an > explanatory comment above it. I would be happy to do that. A comment along the line of 97387c8bdd (am: read interactive input from stdin, 2019-05-20) and a "CAUTION: don't use it for regular prompt" would suffice ? > > -Peff > > [1] Sadly I don't think our test suite could notice the breakage > introduced by this function. It uses the askpass feature to avoid > triggering this code at all, because of course we can not reliably > read from /dev/tty in the script. But with just this patch applied, > and no credential helpers defined, trying "git ls-remote > https://github.com/you/some-private-repo" shows the problem: you get > prompted, but it never reads your input. Many thanks for your comments, Firmin
On Mon, May 10, 2021 at 06:18:36AM +0200, Firmin Martin wrote: > > Looking at the second patch, the motivation here seems to be to use > > git_prompt() for another run-of-the-mill prompt. But the right answer > > is: don't do that. In fact, we recently-ish removed a similar case in > > 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was > > likewise causing problems with the test suite. > > I actually inspired myself from the two occurrences of git_prompt in > builtin/bisect--helper.c introduced in 09535f056b (bisect--helper: > reimplement `bisect_autostart` shell function in C, 2020-09-24). > Not sure if they should also be converted to a simple fgets. Yes, I think they should be switched. It looks like since my earlier patches to "am" we have grown a git_read_line_interactively() helper. See: https://lore.kernel.org/git/pull.755.git.git.1586374380709.gitgitgadget@gmail.com/ They should probably use that (and likewise, it would make sense for git-am to switch to it). > > I think we might consider renaming git_prompt(), or adding an > > explanatory comment above it. > > I would be happy to do that. A comment along the line of 97387c8bdd (am: > read interactive input from stdin, 2019-05-20) and a "CAUTION: don't use > it for regular prompt" would suffice ? Yeah. You might want to point people at git_read_line_interactively() in the same header file, too. -Peff
Jeff King <peff@peff.net> writes: > On Mon, May 10, 2021 at 06:18:36AM +0200, Firmin Martin wrote: > >> > Looking at the second patch, the motivation here seems to be to use >> > git_prompt() for another run-of-the-mill prompt. But the right answer >> > is: don't do that. In fact, we recently-ish removed a similar case in >> > 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was >> > likewise causing problems with the test suite. >> >> I actually inspired myself from the two occurrences of git_prompt in >> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper: >> reimplement `bisect_autostart` shell function in C, 2020-09-24). >> Not sure if they should also be converted to a simple fgets. > > Yes, I think they should be switched. OK, that is because in the context of a "bisect" session, we won't be feeding any real data from its standard input, unlike "git am" that may well be eating a patch stream from its standard input stream. If so, makes sense.
On Tue, May 11, 2021 at 12:38:10PM +0900, Junio C Hamano wrote: > >> I actually inspired myself from the two occurrences of git_prompt in > >> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper: > >> reimplement `bisect_autostart` shell function in C, 2020-09-24). > >> Not sure if they should also be converted to a simple fgets. > > > > Yes, I think they should be switched. > > OK, that is because in the context of a "bisect" session, we won't > be feeding any real data from its standard input, unlike "git am" > that may well be eating a patch stream from its standard input > stream. If so, makes sense. Yes, though even in "git am", we forbid using interactive mode with patches on stdin (and did so even when we were reading from the tty; presumably the rule dates back to when it was a shell script and was using stdin). -Peff
Jeff King <peff@peff.net> writes: > On Tue, May 11, 2021 at 12:38:10PM +0900, Junio C Hamano wrote: > >> >> I actually inspired myself from the two occurrences of git_prompt in >> >> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper: >> >> reimplement `bisect_autostart` shell function in C, 2020-09-24). >> >> Not sure if they should also be converted to a simple fgets. >> > >> > Yes, I think they should be switched. >> >> OK, that is because in the context of a "bisect" session, we won't >> be feeding any real data from its standard input, unlike "git am" >> that may well be eating a patch stream from its standard input >> stream. If so, makes sense. > > Yes, though even in "git am", we forbid using interactive mode with > patches on stdin (and did so even when we were reading from the tty; > presumably the rule dates back to when it was a shell script and was > using stdin). As long as the "prompt and accept an single-line answer from the end user" is restricted to "git am -i", I'll be perfectly OK with that. I just do not want my regular "type '|' in my MUA to pipe the current article to a command, and give 'git am -s' as the command" workflow to get broken in the future when somebody blindly follows a carelessly written direction to use a helper that reads from the standard input for confirmation. The condition under which use of that helper is appropriate needs to be clearly spelled out. Thanks.
On Tue, May 11, 2021 at 03:17:13PM +0900, Junio C Hamano wrote: > >> OK, that is because in the context of a "bisect" session, we won't > >> be feeding any real data from its standard input, unlike "git am" > >> that may well be eating a patch stream from its standard input > >> stream. If so, makes sense. > > > > Yes, though even in "git am", we forbid using interactive mode with > > patches on stdin (and did so even when we were reading from the tty; > > presumably the rule dates back to when it was a shell script and was > > using stdin). > > As long as the "prompt and accept an single-line answer from the end > user" is restricted to "git am -i", I'll be perfectly OK with that. > I just do not want my regular "type '|' in my MUA to pipe the > current article to a command, and give 'git am -s' as the command" > workflow to get broken in the future when somebody blindly follows a > carelessly written direction to use a helper that reads from the > standard input for confirmation. The condition under which use of > that helper is appropriate needs to be clearly spelled out. Yeah, I don't think anybody is proposing to change the behavior of "git am" here (we might swap out the current fgets(stdin) for a helper which does the equivalent). But I agree that any comment recommending one versus the other should probably remind people to think about how stdin is otherwise used in the program, and whether that will cause any conflicts. -Peff
diff --git a/compat/terminal.c b/compat/terminal.c index 43b73ddc75..c12e0b9ab9 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -202,41 +202,50 @@ static int mingw_getchar(void) char *git_terminal_prompt(const char *prompt, int echo) { static struct strbuf buf = STRBUF_INIT; - int r; - FILE *input_fh, *output_fh; + int r, input_not_from_tty = !isatty(STDIN_FILENO); + FILE *input_fh = NULL, *output_fh = NULL; + char* ret = NULL; + + if (input_not_from_tty) + input_fh = stdin; + else + input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT); - input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT); if (!input_fh) - return NULL; + goto done; output_fh = fopen(OUTPUT_PATH, "w" FORCE_TEXT); - if (!output_fh) { - fclose(input_fh); - return NULL; - } - if (!echo && disable_echo()) { - fclose(input_fh); - fclose(output_fh); - return NULL; - } + if (!output_fh) + goto done; + + if (!echo && disable_echo()) + goto done; fputs(prompt, output_fh); fflush(output_fh); r = strbuf_getline_lf(&buf, input_fh); - if (!echo) { + + if (input_not_from_tty) + fputs(buf.buf, output_fh); + + if (!echo || input_not_from_tty) { putc('\n', output_fh); fflush(output_fh); } restore_term(); - fclose(input_fh); - fclose(output_fh); - if (r == EOF) - return NULL; - return buf.buf; + if (r != EOF) + ret = buf.buf; +done: + if (input_fh && input_fh != stdin) + fclose(input_fh); + if (output_fh) + fclose(output_fh); + + return ret; } /*
Currently, git_prompt ignores input coming from anywhere other than terminal (pipe, redirection etc.) meaning that standard prompt auto-answering methods would have no effect: echo 'Y' | git ... yes 'Y' | git ... git ... <input.txt It also prevents git subcommands using git_prompt to be tested using such methods. This patch fixes this issue by considering standard input when !isatty(0). It also rearranges the control flow to close input and output file handlers. Signed-off-by: Firmin Martin <firminmartin24@gmail.com> --- compat/terminal.c | 47 ++++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 19 deletions(-)