Firmin Martin [off-list ref] 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 [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...
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 [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
On Mon, May 10, 2021 at 06:18:36AM +0200, Firmin Martin wrote:
quoted
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).
quoted
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