Jeff King <peff@xxxxxxxx> writes: Hi Peff, > On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote: > >> Firmin Martin <firminmartin24@xxxxxxxxx> 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
