| Message ID | patch-2.2-964e7f4531f-20211102T155046Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | prompt.c: split up git_prompt(), read from /dev/tty, not STDIN | expand |
On Tue, Nov 02, 2021 at 05:48:10PM +0100, Ævar Arnfjörð Bjarmason wrote: > In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we > we fixed a behavior change in the conversion of git-am from a > shellscript to a C program by changing it from using git_prompt() to > using fgets(..., stdin). This ensured that we could run: > > echo y | git am --interactive [...] > > But along with that in the subsequent 6e7baf246a2 (am: drop tty > requirement for --interactive, 2019-05-20) we had to remove support > for: > > git am --interactive </dev/null > > This change builds on the refactoring of git_prompt() into "normal > prompt" and "wants password" functions in the preceding commit, and > moves "git am --interactive" back to using the prompt function. Why do we want to do that? The only reason I mentioned that "/dev/null" thing in the earlier commit is that it's pointless. IMHO nothing should be using git_prompt() outside of the credential code. They should just be reading from stdin, which is much more flexible. If a caller knows that stdin is coming from elsewhere, they can redirect from /dev/tty. > It seems to me that using /dev/tty is desirable over using stdin, > these prompts are meant to be interactive, and our acceptance of stdin > was an artifact of how these commands were originally implemented in > shellscript. Basically, I think I just disagree with this paragraph entirely. Moving to stdin in the commits you referenced was done to help testing, but I also think it's just a more flexible direction overall. -Peff
On Wed, Nov 03 2021, Jeff King wrote: > On Tue, Nov 02, 2021 at 05:48:10PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we >> we fixed a behavior change in the conversion of git-am from a >> shellscript to a C program by changing it from using git_prompt() to >> using fgets(..., stdin). This ensured that we could run: >> >> echo y | git am --interactive [...] >> >> But along with that in the subsequent 6e7baf246a2 (am: drop tty >> requirement for --interactive, 2019-05-20) we had to remove support >> for: >> >> git am --interactive </dev/null >> >> This change builds on the refactoring of git_prompt() into "normal >> prompt" and "wants password" functions in the preceding commit, and >> moves "git am --interactive" back to using the prompt function. > > Why do we want to do that? The only reason I mentioned that "/dev/null" > thing in the earlier commit is that it's pointless. > > IMHO nothing should be using git_prompt() outside of the credential > code. They should just be reading from stdin, which is much more > flexible. If a caller knows that stdin is coming from elsewhere, they > can redirect from /dev/tty. > >> It seems to me that using /dev/tty is desirable over using stdin, >> these prompts are meant to be interactive, and our acceptance of stdin >> was an artifact of how these commands were originally implemented in >> shellscript. > > Basically, I think I just disagree with this paragraph entirely. Moving > to stdin in the commits you referenced was done to help testing, but I > also think it's just a more flexible direction overall. I'm fine with it either way, my reading of your 2019-ish commits was that you did that not to intentionally get that behavior, but to work around that test issue. So we do always want the "read from stdin" behavior, so I can get those bisect tests by just changing its behavior too, with no need for the test variable? If so I'm fine with that. I think it's a good thing in general to have a not-for-password git_prompt() API, because it makes it easy to make that use some readline-like API, i.e. one that would have tab completion, and handle the loop some (but not all) callers have around handling retries etc, we'd also be able to translate the "Y" "n" characters...
Jeff King <peff@peff.net> writes: > Basically, I think I just disagree with this paragraph entirely. Moving > to stdin in the commits you referenced was done to help testing, but I > also think it's just a more flexible direction overall. It is OK, and it is more convenient for writing test scripts, to take interactive input from the standard input stream, if the command does not use the standard input for other purposes. "git am -i <mbox" cannot take prompted input via the standard input, but "git am -i mbox" is an easy workaround, for example. Commands that are designed to be used in the downstream of a pipe (e.g. "git rev-list ... | git pack-objects") cannot easily use such a workaround, so they may still need to open and interact with /dev/tty if they want to do an interactive input, though [*]. [Footnote] * "pack-objects" is an excellent example of a command that takes its primary input from the standard input, but is a horrible example otherwise, because it probably would not make sense for it to take any prompted input.
Hi Junio & Peff, On Wed, 3 Nov 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Basically, I think I just disagree with this paragraph entirely. > > Moving to stdin in the commits you referenced was done to help > > testing, but I also think it's just a more flexible direction overall. > > It is OK, and it is more convenient for writing test scripts, to take > interactive input from the standard input stream, if the command does > not use the standard input for other purposes. I think I remember when we talked about this, it was in the context of `git add -p` becoming a built-in, and we all agreed that it is actually a very nice side effect that you can feed commands to `git add -p` in scripts via stdin, not only for testing. It might have been in the context of another command, but even then it is a fact that this is a very nice side effect. Ciao, Dscho
On Thu, Nov 04, 2021 at 09:48:35AM +0100, Johannes Schindelin wrote: > Hi Junio & Peff, > > On Wed, 3 Nov 2021, Junio C Hamano wrote: > > > Jeff King <peff@peff.net> writes: > > > > > Basically, I think I just disagree with this paragraph entirely. > > > Moving to stdin in the commits you referenced was done to help > > > testing, but I also think it's just a more flexible direction overall. > > > > It is OK, and it is more convenient for writing test scripts, to take > > interactive input from the standard input stream, if the command does > > not use the standard input for other purposes. > > I think I remember when we talked about this, it was in the context of > `git add -p` becoming a built-in, and we all agreed that it is actually a > very nice side effect that you can feed commands to `git add -p` in > scripts via stdin, not only for testing. > > It might have been in the context of another command, but even then it is > a fact that this is a very nice side effect. Yes, we definitely had that discussion about "add -p", and I agree it is nice. People are probably less likely to drive other tools like git-am and git-bisect in such a way, though, as their interactive modes just do a lot less. -Peff
On Wed, Nov 03, 2021 at 10:42:14AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Basically, I think I just disagree with this paragraph entirely. Moving > > to stdin in the commits you referenced was done to help testing, but I > > also think it's just a more flexible direction overall. > > It is OK, and it is more convenient for writing test scripts, to > take interactive input from the standard input stream, if the > command does not use the standard input for other purposes. > > "git am -i <mbox" cannot take prompted input via the standard input, > but "git am -i mbox" is an easy workaround, for example. > > Commands that are designed to be used in the downstream of a pipe > (e.g. "git rev-list ... | git pack-objects") cannot easily use such > a workaround, so they may still need to open and interact with > /dev/tty if they want to do an interactive input, though [*]. True. The most Unix-y thing there would be to provide an option for reading interactive input from an arbitrary descriptor. That gives the most flexibility, though it's probably a bit arcane for most folks to do: git foo | git bar --interactive-from=3 3</dev/tty We could directly allow: git foo | git bar --interactive-from=/dev/tty which is a bit less arcane. Or alternatively this could come from the environment, like: export GIT_INTERACTIVE_FROM=/dev/tty git foo | git bar --interactive Which is equivalent-ish to having a boolean env variable to say "read from the terminal", except that it retains some more of the flexibility (especially if we treat a numeric value as a descriptor). Of course yet another option is to teach commands like pack-objects that read input only from stdin to accept a command-line option to read that input from a file. Then stdin is free for interactive use. ;) But I would not do any of that until we had a command that was a good candidate. In the case of git-am and git-bisect, I think it's fine to assume that "-i" will use stdin. -Peff
On Wed, Nov 03, 2021 at 04:12:43PM +0100, Ævar Arnfjörð Bjarmason wrote: > I'm fine with it either way, my reading of your 2019-ish commits was > that you did that not to intentionally get that behavior, but to work > around that test issue. > > So we do always want the "read from stdin" behavior, so I can get those > bisect tests by just changing its behavior too, with no need for the > test variable? If so I'm fine with that. Yes. The thing that makes it OK to do from a backwards-compatibility standpoint is that even though the tools read from /dev/tty now, they also insist that stdin is a tty. So in practice it will always be the same thing (technically you _could_ redirect stdin from a different tty, but that's sufficiently bizarre that I think we can discount it). > I think it's a good thing in general to have a not-for-password > git_prompt() API, because it makes it easy to make that use some > readline-like API, i.e. one that would have tab completion, and handle > the loop some (but not all) callers have around handling retries etc, > we'd also be able to translate the "Y" "n" characters... Yeah, I'd be OK with that direction, but I think it is not really the same thing that the existing git_prompt() was designed for (where you most certainly don't want readline-ish things like history for your password). Perhaps a good first step is not so much splitting git_prompt() into two pieces, as simply renaming it in place. It really was designed for the credential request and not much else, but the name is a bit more generic. But I'd probably wait on that until we were looking at actually adding a functional readline-ish interface before even doing that. -Peff
diff --git a/builtin/am.c b/builtin/am.c index 8677ea2348a..1e90b9ea0cd 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1693,7 +1693,7 @@ static int do_interactive(struct am_state *state) assert(state->msg); for (;;) { - char reply[64]; + const char *reply; puts(_("Commit Body is:")); puts("--------------------------"); @@ -1705,9 +1705,7 @@ static int do_interactive(struct am_state *state) * in your translation. The program will only accept English * input at this point. */ - printf(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: ")); - if (!fgets(reply, sizeof(reply), stdin)) - die("unable to read from stdin; aborting"); + reply = git_prompt_echo(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: ")); if (*reply == 'y' || *reply == 'Y') { return 0; @@ -2437,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) strvec_push(&paths, mkpath("%s/%s", prefix, argv[i])); } - if (state.interactive && !paths.nr) + if (state.interactive && !paths.nr && isatty(0)) die(_("interactive mode requires patches on the command line")); am_setup(&state, patch_format, paths.v, keep_cr); diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 30533a70b53..dd73d76df3e 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -830,9 +830,6 @@ static int bisect_autostart(struct bisect_terms *terms) fprintf_ln(stderr, _("You need to start by \"git bisect " "start\"\n")); - if (!isatty(STDIN_FILENO)) - return -1; - /* * TRANSLATORS: Make sure to include [Y] and [n] in your * translation. The program will only accept English input diff --git a/prompt.c b/prompt.c index 458d6637506..273bc30bf0e 100644 --- a/prompt.c +++ b/prompt.c @@ -6,9 +6,15 @@ char *git_prompt(const char *prompt, unsigned int echo) { + const char *test_var = "GIT_TEST_TERMINAL_PROMPT"; char *r = NULL; - if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) { + if (git_env_bool(test_var, 0) && !isatty(0)) { + char reply[64]; + if (!fgets(reply, sizeof(reply), stdin)) + die("unable to read from stdin in '%s=true' mode", test_var); + return xstrdup(reply); + } else if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) { r = git_terminal_prompt(prompt, echo); if (!r) die_errno("could not read"); diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 1be85d064e7..2afb1b57b45 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -45,6 +45,47 @@ test_expect_success 'set up basic repo with 1 file (hello) and 4 commits' ' HASH4=$(git rev-parse --verify HEAD) ' +test_expect_success 'bisect "good" without a "start": no prompt' ' + cat >expect <<-\EOF && + You need to start by "git bisect start" + + fatal: unable to read from stdin in '\''GIT_TEST_TERMINAL_PROMPT=true'\'' mode + EOF + test_expect_code 128 git bisect good HEAD 2>actual && + test_cmp expect actual && + test_must_fail git bisect bad HEAD~ 2>actual && + test_cmp expect actual +' + +test_expect_success 'bisect "good" without a "start": have prompt' ' + cat >expect <<-\EOF && + You need to start by "git bisect start" + + EOF + echo n | test_expect_code 1 git bisect good HEAD 2>actual && + test_cmp expect actual && + echo n | test_must_fail git bisect bad HEAD~ 2>actual && + test_cmp expect actual +' + +test_expect_success 'bisect "good" without a "start": answer prompt' ' + cat >expect <<-\EOF && + You need to start by "git bisect start" + + EOF + echo Y | git bisect good HEAD 2>actual && + test_cmp expect actual && + + # We will only get this far with the "Y" prompt + cat >expect <<-\EOF && + Some good revs are not ancestors of the bad rev. + git bisect cannot work properly in this case. + Maybe you mistook good and bad revs? + EOF + test_must_fail git bisect bad HEAD~ 2>actual && + test_cmp expect actual +' + test_expect_success 'bisect starts with only one bad' ' git bisect reset && git bisect start && diff --git a/t/test-lib.sh b/t/test-lib.sh index 2679a7596a6..778a08ffe4b 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -476,6 +476,10 @@ export GIT_TEST_MERGE_ALGORITHM GIT_TRACE_BARE=1 export GIT_TRACE_BARE +# Have git_prompt_noecho() accept stdin +GIT_TEST_TERMINAL_PROMPT=true +export GIT_TEST_TERMINAL_PROMPT + # Use specific version of the index file format if test -n "${GIT_TEST_INDEX_VERSION:+isset}" then
In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we we fixed a behavior change in the conversion of git-am from a shellscript to a C program by changing it from using git_prompt() to using fgets(..., stdin). This ensured that we could run: echo y | git am --interactive [...] But along with that in the subsequent 6e7baf246a2 (am: drop tty requirement for --interactive, 2019-05-20) we had to remove support for: git am --interactive </dev/null This change builds on the refactoring of git_prompt() into "normal prompt" and "wants password" functions in the preceding commit, and moves "git am --interactive" back to using the prompt function. This allows us to have our cake and eat it too by adding a GIT_TERMINAL_PROMPT=true mode to test-lib.sh. Adjusting "git am --interactive" for use in our tests (see e.g. "t/t4257-am-interactive.sh") was what 97387c8bdd9 and 6e7baf246a2 were aiming for. Then more recently in 09535f056b0 (bisect--helper: reimplement `bisect_autostart` shell function in C, 2020-09-24) we've had the same sort of behavior change happen to "git bisect"'s interactive question mode, it now uses git_prompt()'s /dev/tty, not stdin. It seems to me that using /dev/tty is desirable over using stdin, these prompts are meant to be interactive, and our acceptance of stdin was an artifact of how these commands were originally implemented in shellscript. So let's move "git am --interactive" back to using "git_prompt()" (which is called "git_prompt_echo()" as of the preceding commit), and similarly remove the "!isatty(STDIN_FILENO)" test added in 09535f056b0, that control flow was converted as-is from the shellscript behavior. Let's also change a similar assertion added to "git am" in 6e7baf246a2. Now we'll die on: # no arguments provided git am --interactive But not: git am --interactive </dev/null Or: git am --interactive <mbox To do this we'll need to add a GIT_TEST_TERMINAL_PROMPT variable for use in test-lib.sh, by doing so this "echo input | git cmd ..." behavior of interactive commands is now isolated to our own test suite, instead of leaking out into the wild. Now that we've done that we can exhaustively test the prompt behavior of "git bisect", which wasn't previously possible. There is some discussion downthread of the series 97387c8bdd9 is in about whether we should always accept stdin input in these commands[1]. I think that's arguably a good idea, and perhaps we'll need to change the approach here. Using a git_prompt_echo() that we know never needs to handle passwords should provide us with an easy path towards deciding what to do in those cases, we'll be able to consistently pick one behavior or the other, instead of having the behavior of specific commands cater to test-only needs. The lack of _() on the new die() message is intentional. This message will only be emitted if there's a bug in our own test suite, so it's a waste of translator time to translate it. 1. https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/am.c | 8 +++----- builtin/bisect--helper.c | 3 --- prompt.c | 8 +++++++- t/t6030-bisect-porcelain.sh | 41 +++++++++++++++++++++++++++++++++++++ t/test-lib.sh | 4 ++++ 5 files changed, 55 insertions(+), 9 deletions(-)