diff mbox series

[v1,1/8] compat/terminal: let prompt accept input from pipe

Message ID 20210506165102.123739-2-firminmartin24@gmail.com (mailing list archive)
State New
Headers show
Series format-patch: introduce --confirm-overwrite | expand

Commit Message

Firmin Martin May 6, 2021, 4:50 p.m. UTC
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(-)

Comments

Junio C Hamano May 6, 2021, 11:37 p.m. UTC | #1
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.
Jeff King May 7, 2021, 4:54 a.m. UTC | #2
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.
Junio C Hamano May 7, 2021, 5:25 a.m. UTC | #3
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.
Firmin Martin May 10, 2021, 4:18 a.m. UTC | #4
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
Jeff King May 10, 2021, 9:32 p.m. UTC | #5
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
Junio C Hamano May 11, 2021, 3:38 a.m. UTC | #6
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.
Jeff King May 11, 2021, 6:10 a.m. UTC | #7
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
Junio C Hamano May 11, 2021, 6:17 a.m. UTC | #8
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.
Jeff King May 11, 2021, 6:37 a.m. UTC | #9
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 mbox series

Patch

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;
 }
 
 /*