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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Firmin Martin <firminmartin24@gmail.com>,
	git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmail.com>,
	Erik Faye-Lund <kusmabite@gmail.com>,
	Denton Liu <liu.denton@gmail.com>
Subject: Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe
Date: Fri, 7 May 2021 00:54:18 -0400	[thread overview]
Message-ID: <YJTH+sTP/O5Nxtp9@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqr1ijwi0i.fsf@gitster.g>

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.