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

Jeff King [off-list ref] writes:

Hi Peff,

On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote:

quoted

Firmin Martin [off-list ref] writes:

quoted

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

quoted

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