git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 2, 2021, 16:48 UTC
Message-ID
<patch-2.2-964e7f4531f-20211102T155046Z-avarab@gmail.com>
In-Reply-To
<cover-0.2-00000000000-20211102T155046Z-avarab@gmail.com>

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(-)
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

-- 
2.33.1.1570.g069344fdd45
Previous: Junio C HamanoNext: Jeff King
Message 14 of 30 in “Abort (core dumped)”
  1. Alejandro SanchezMay 20, 2019
  2. Jeff KingMay 20, 2019
  3. 0/4 fix BUG() with "git am -i --resolved"Jeff King, May 20, 2019
  4. 1/4 am: simplify prompt response handlingJeff King, May 20, 2019
  5. 2/4 am: read interactive input from stdinJeff King, May 20, 2019
  6. 3/4 am: drop tty requirement for --interactiveJeff King, May 20, 2019
  7. Jeff KingMay 20, 2019
  8. Johannes SchindelinMay 23, 2019
  9. Jeff KingMay 24, 2019
  10. 0/2 prompt.c: split up git_prompt(), read from /dev/tty, not STDINÆvar Arnfjörð Bjarmason, Nov 2, 2021
  11. 1/2 prompt.c: split up the password and non-password handlingÆvar Arnfjörð Bjarmason, Nov 2, 2021
  12. Jeff KingNov 3, 2021
  13. Junio C HamanoNov 3, 2021
  14. 2/2 prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=trueÆvar Arnfjörð Bjarmason, Nov 2, 2021
  15. Jeff KingNov 3, 2021
  16. Ævar Arnfjörð BjarmasonNov 3, 2021
  17. Jeff KingNov 4, 2021
  18. Junio C HamanoNov 3, 2021
  19. Johannes SchindelinNov 4, 2021
  20. Jeff KingNov 4, 2021
  21. Jeff KingNov 4, 2021
  22. 4/4 am: fix --interactive HEAD tree resolutionJeff King, May 20, 2019
  23. Johannes SchindelinMay 23, 2019
  24. Jeff KingMay 24, 2019
  25. 4/4 am: fix --interactive HEAD tree resolutionJeff King, May 24, 2019
  26. Johannes SchindelinMay 28, 2019
  27. Jeff KingMay 28, 2019
  28. Johannes SchindelinMay 29, 2019
  29. Alejandro SanchezSep 26, 2019
  30. Jeff KingSep 26, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.