Abort (core dumped)

Abort (core dumped)

[Alejandro Sanchez]

A core dump occurred while trying to interactively apply (3-way) a
series of patches from a mailbox.

git version 2.20.1

Steps to reproduce:

alex@polaris:~/slurm/source$ git am -3 -i
~/Downloads/6033-18.08-final-patchset-v2.patch
Commit Body is:
--------------------------
Use correct signed/unsiged types.

Change a few variables in archiving to use the correct signed or
unsigned type to avoid implicit casting.

Bug 6033.
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y
Applying: Use correct signed/unsiged types.
Commit Body is:
--------------------------
Remove unused static variable high_buffer_size.

It was set but never read.

Bug 6033.
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y
Applying: Remove unused static variable high_buffer_size.
Commit Body is:
--------------------------
Handle duplicate archive file names.

The time period of the archive file currently depends on submit or start
time and whether the purge period is in hours, days, or months.
Previously, if the archive file name already exists, we would overwrite
the old archive file with the assumption that these are duplicate
records being archived after an archive load. However, that could result
in lost records in a couple of ways:

 * If there were runaway jobs that were part of an old archive file's
 time period and are later fixed and then purged, the old file would
 be overwritten.
 * If jobs or steps are purged but there are still jobs or steps in
 that time period that are pending or running, the pending or running
 jobs and steps won't be purged. When they finish and are purged, the
 old file would be overwritten.

Instead of overwriting the old file, we append a number to the file name
to create a new file. This will also be important in an upcoming commit.

Bug 6033.
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y
Applying: Handle duplicate archive file names.
Using index info to reconstruct a base tree...
M    NEWS
Falling back to patching base and 3-way merge...
Auto-merging NEWS
CONFLICT (content): Merge conflict in NEWS
error: Failed to merge in the changes.
Patch failed at 0003 Handle duplicate archive file names.
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am -i --continue".
If you prefer to skip this patch, run "git am -i --skip" instead.
To restore the original branch and stop patching, run "git am -i --abort".
alex@polaris:~/slurm/source$ vi NEWS
alex@polaris:~/slurm/source$ man git add
alex@polaris:~/slurm/source$ git add -u
alex@polaris:~/slurm/source$ git am -i --continue
Applying: Handle duplicate archive file names.
error: object 861d3c6f689a3ca5eb5fb5c409d46de0ad5555e1 is a commit, not a tree
BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree
Aborted (core dumped)
alex@polaris:~/slurm/source$

Re: Abort (core dumped)

[Jeff King]

On Mon, May 20, 2019 at 10:35:53AM +0200, Alejandro Sanchez wrote:

> alex@polaris:~/slurm/source$ git add -u
> alex@polaris:~/slurm/source$ git am -i --continue
> Applying: Handle duplicate archive file names.
> error: object 861d3c6f689a3ca5eb5fb5c409d46de0ad5555e1 is a commit, not a tree
> BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree
> Aborted (core dumped)

Hmm. So I think the interesting error is probably that first line: some
code expects to look up a tree but sees a commit, and then as a result
it probably feeds too few items to run_diff_index(), triggering the
assertion failure.

Just grepping around, this looks quite suspicious:

  $ git grep -hW get_oid_tree builtin/am.c
  static void write_index_patch(const struct am_state *state)
  ...
  if (!get_oid_tree("HEAD", &head))
	tree = lookup_tree(the_repository, &head);
  else
	tree = lookup_tree(the_repository,
			   the_repository->hash_algo->empty_tree);

Using get_oid_tree() does not actually return a tree; it just
prioritizes trees when disambiguating names (which is pointless here,
since we're not feeding an ambiguous oid). HEAD will always be a commit,
and then lookup_tree() similarly does not peel that down to an actual
tree. And this whole function is called only in interactive-mode, so
it's possible that it's simply not used much and nobody noticed.

I haven't tried to reproduce yet. Is the repository (and patch) that you
used to demonstrate this publicly available? Or alternatively, is it
possible to show a backtrace from the coredump?

If my blind guess is right, then something like this probably fixes it:

diff --git a/builtin/am.c b/builtin/am.c
index bdd1bbc35d..93305560c1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state)
 	struct rev_info rev_info;
 	FILE *fp;
 
-	if (!get_oid_tree("HEAD", &head))
-		tree = lookup_tree(the_repository, &head);
-	else
+	if (!get_oid("HEAD", &head)) {
+		struct object *obj;
+		struct commit *commit;
+
+		obj = parse_object_or_die(&head, NULL);
+		commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);
+		if (!commit)
+			die("unable to parse HEAD as a commit");
+
+		tree = get_commit_tree(commit);
+	} else
 		tree = lookup_tree(the_repository,
 				   the_repository->hash_algo->empty_tree);
 

-Peff

[PATCH 0/4] fix BUG() with "git am -i --resolved"

[Jeff King]

On Mon, May 20, 2019 at 06:02:11AM -0400, Jeff King wrote:

> I haven't tried to reproduce yet. Is the repository (and patch) that you
> used to demonstrate this publicly available? Or alternatively, is it
> possible to show a backtrace from the coredump?

Never mind. It reproduces quite easily, because AFAICT "am -i
--resolved" has been totally broken since it was converted to C (in
2015!). I guess it does not have many users. :)

> If my blind guess is right, then something like this probably fixes it:

This is indeed the problem. Here are some patches. The actual fix is
what I showed already. The other three are just making it possible to
cover this with the test suite (until now, we had no coverage of "am
--interactive" at all).

  [1/4]: am: simplify prompt response handling
  [2/4]: am: read interactive input from stdin
  [3/4]: am: drop tty requirement for --interactive
  [4/4]: am: fix --interactive HEAD tree resolution

 builtin/am.c              | 30 +++++++++++++++---------
 t/t4257-am-interactive.sh | 49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 11 deletions(-)
 create mode 100755 t/t4257-am-interactive.sh

-Peff

[PATCH 1/4] am: simplify prompt response handling

[Jeff King]

We'll never see a NULL returned from git_prompt(); if it can't produce
any input for us (e.g., because the terminal got EOF) then it will just
die().

So there's no need for us to handle NULL here. And even if there was, it
doesn't make sense to continue; on a true terminal hangup we'd just loop
infinitely trying to get input that will never come.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 912d9821b1..644bb11f6c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1661,9 +1661,7 @@ static int do_interactive(struct am_state *state)
 		 */
 		reply = git_prompt(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "), PROMPT_ECHO);
 
-		if (!reply) {
-			continue;
-		} else if (*reply == 'y' || *reply == 'Y') {
+		if (*reply == 'y' || *reply == 'Y') {
 			return 0;
 		} else if (*reply == 'a' || *reply == 'A') {
 			state->interactive = 0;
-- 
2.22.0.rc1.539.g7bfcdfe86d

[PATCH 2/4] am: read interactive input from stdin

[Jeff King]

In the conversion of git-am from shell script to C, we switched to using
git_prompt(). Unlike the original shell command "read reply", this
doesn't read from stdin at all, but rather from /dev/tty.

In most cases this distinction wouldn't matter. We require (as the shell
script did) that stdin is a tty, so they would generally be the same
thing. But one important exception is our test suite: even with
test_terminal, we cannot test "am --interactive" because it insists on
reading from /dev/tty, not the pseudo-tty we've set up in the test
script.

Fixing this clears the way to adding tests in a future patch.

Signed-off-by: Jeff King <peff@peff.net>
---
Part of me dies inside when I look at adding the magical "64". But we
expect this input to be single-line (or at most, somebody might actually
type out "edit", etc), and avoiding strbuf_getline() saves us from
having to free the strbuf in the many early return paths. I dunno. If
it's too ugly it would not be too hard to switch it over.

 builtin/am.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 644bb11f6c..47ad7a5a70 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1647,7 +1647,7 @@ static int do_interactive(struct am_state *state)
 		die(_("cannot be interactive without stdin connected to a terminal."));
 
 	for (;;) {
-		const char *reply;
+		char reply[64];
 
 		puts(_("Commit Body is:"));
 		puts("--------------------------");
@@ -1659,7 +1659,9 @@ static int do_interactive(struct am_state *state)
 		 * in your translation. The program will only accept English
 		 * input at this point.
 		 */
-		reply = git_prompt(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "), PROMPT_ECHO);
+		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");
 
 		if (*reply == 'y' || *reply == 'Y') {
 			return 0;
-- 
2.22.0.rc1.539.g7bfcdfe86d

[PATCH 3/4] am: drop tty requirement for --interactive

[Jeff King]

We have required that the stdin of "am --interactive" be a tty since
a1451104ac (git-am: interactive should fail gracefully., 2005-10-12).
However, this isn't strictly necessary, and makes the tool harder to
test (and is unlike all of our other --interactive commands).

The goal of that commit was to make sure that somebody does not do:

  git am --interactive <mbox

and cause us to read commands from the mbox. But we can simply check
up front for this case and complain before entering the interactive
loop.

Technically this disallows:

  git am --interactive </dev/null

where our lack of patches means we would never prompt for anything, and
so the old code would not notice our lack of tty (and now we'd die
early). But since such a command is totally pointless, it's no loss.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 47ad7a5a70..ea16b844f1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1643,9 +1643,6 @@ static int do_interactive(struct am_state *state)
 {
 	assert(state->msg);
 
-	if (!isatty(0))
-		die(_("cannot be interactive without stdin connected to a terminal."));
-
 	for (;;) {
 		char reply[64];
 
@@ -2334,6 +2331,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 				argv_array_push(&paths, mkpath("%s/%s", prefix, argv[i]));
 		}
 
+		if (state.interactive && !paths.argc)
+			die(_("interactive mode requires patches on the command line"));
+
 		am_setup(&state, patch_format, paths.argv, keep_cr);
 
 		argv_array_clear(&paths);
-- 
2.22.0.rc1.539.g7bfcdfe86d

Re: [PATCH 3/4] am: drop tty requirement for --interactive

[Jeff King]

On Mon, May 20, 2019 at 08:11:13AM -0400, Jeff King wrote:

> We have required that the stdin of "am --interactive" be a tty since
> a1451104ac (git-am: interactive should fail gracefully., 2005-10-12).
> However, this isn't strictly necessary, and makes the tool harder to
> test (and is unlike all of our other --interactive commands).

I think this is worth doing for simplicity and consistency. But as you
might guess, my ulterior motive was making it easier to add tests.

In theory we _should_ be able to use test_terminal for this, but it
seems to be racy, because it will quickly read all input and close the
descriptor (to give the reader EOF). But after that close, isatty() will
no longer report it correctly. E.g., if I run this:

  perl test-terminal.perl sh -c '
	for i in 0 1 2; do
		echo $i is $(test -t $i || echo not) a tty
	done
  ' </dev/null

it _usually_ says "0 is a tty", but racily may say "not a tty". If you
put a sleep into the beginning of the shell, then it will basically
always lose the race and say "not".

It might be possible to overcome this by making test-terminal more
clever (i.e., is there a way for us to send an "EOF" over the pty
without actually _closing_ it? That would behave like a real terminal,
where you can hit ^D to generate an EOF but then type more).

But barring that, this works by just avoiding it entirely. :)

Curiously, my script above also reports consistently that stdout is not
a tty, but that stderr is. I'm not sure why this is, but it no tests
seem to care either way.

-Peff

Re: [PATCH 3/4] am: drop tty requirement for --interactive

[Johannes Schindelin]

Hi Peff,

On Mon, 20 May 2019, Jeff King wrote:

> On Mon, May 20, 2019 at 08:11:13AM -0400, Jeff King wrote:
>
> > We have required that the stdin of "am --interactive" be a tty since
> > a1451104ac (git-am: interactive should fail gracefully., 2005-10-12).
> > However, this isn't strictly necessary, and makes the tool harder to
> > test (and is unlike all of our other --interactive commands).
>
> I think this is worth doing for simplicity and consistency. But as you
> might guess, my ulterior motive was making it easier to add tests.
>
> In theory we _should_ be able to use test_terminal for this, but it
> seems to be racy, because it will quickly read all input and close the
> descriptor (to give the reader EOF). But after that close, isatty() will
> no longer report it correctly. E.g., if I run this:
>
>   perl test-terminal.perl sh -c '
> 	for i in 0 1 2; do
> 		echo $i is $(test -t $i || echo not) a tty
> 	done
>   ' </dev/null
>
> it _usually_ says "0 is a tty", but racily may say "not a tty". If you
> put a sleep into the beginning of the shell, then it will basically
> always lose the race and say "not".

This is just another nail in the coffin for `test-terminal.perl`, as far
as I am concerned.

In the built-in `add -i` patch series, I followed a strategy where I move
totally away from `test-terminal`, in favor of using some knobs to force
Git into thinking that we are in a terminal.

But at the same time, I *also* remove the limitation (for most cases) of
"read from /dev/tty", in favor of reading from stdin, and making things
testable, and more importantly: scriptable.

So I am *very* much in favor of this here patch.

Thanks,
Dscho

P.S.: There are even more reasons to get rid of `test-terminal`, of
course: it is an unnecessary dependency on Perl, works only when certain
Perl modules are installed (that are *not* installed on Ubuntu by default,
for example), and it requires pseudo terminals, so it will *never* work on
Windows.

> It might be possible to overcome this by making test-terminal more
> clever (i.e., is there a way for us to send an "EOF" over the pty
> without actually _closing_ it? That would behave like a real terminal,
> where you can hit ^D to generate an EOF but then type more).
>
> But barring that, this works by just avoiding it entirely. :)
>
> Curiously, my script above also reports consistently that stdout is not
> a tty, but that stderr is. I'm not sure why this is, but it no tests
> seem to care either way.
>
> -Peff
>

Re: [PATCH 3/4] am: drop tty requirement for --interactive

[Jeff King]

On Thu, May 23, 2019 at 08:44:31AM +0200, Johannes Schindelin wrote:

> >   perl test-terminal.perl sh -c '
> > 	for i in 0 1 2; do
> > 		echo $i is $(test -t $i || echo not) a tty
> > 	done
> >   ' </dev/null
> >
> > it _usually_ says "0 is a tty", but racily may say "not a tty". If you
> > put a sleep into the beginning of the shell, then it will basically
> > always lose the race and say "not".
> 
> This is just another nail in the coffin for `test-terminal.perl`, as far
> as I am concerned.

I think it's only broken for stdin, but yeah, it's not great. I think
the fact that test-terminal is not available everywhere (and thus many
people are skipping a bunch of tests) is much more damning. :)

> In the built-in `add -i` patch series, I followed a strategy where I move
> totally away from `test-terminal`, in favor of using some knobs to force
> Git into thinking that we are in a terminal.

I'm in favor of this. The current "add -i" is pretty accepting of
reading from stdin, and I think we can do that in most places. The main
use of test_terminal has been to check color and progress decisions. I'd
be just as happy to see something like this:

  int git_isatty(int fd)
  {
	static int override[3];
	static int initialized;
	if (!initialized) {
		const char *x = getenv("GIT_PRETEND_TTY");
		if (x) {
			for (; *x; x++) {
				int n = *x - '0';
				if (n > 0 && n < ARRAY_SIZE(override)
					override[n] = 1;
			}
		}
		initialized = 1;
	}
	if (fd > 0 && fd < ARRAY_SIZE(override) && override[fd])
		return 1;
	return isatty(fd);
  }

> But at the same time, I *also* remove the limitation (for most cases) of
> "read from /dev/tty", in favor of reading from stdin, and making things
> testable, and more importantly: scriptable.

As far as I know, apart from this git-am fix, the only thing that reads
from the terminal is the credential prompt. That one has to be a bit
picky, because:

  - we need to prompt from processes which have no stdio connected to
    the user (e.g., remote-curl).

  - we need to put the terminal into no-echo mode for passwords (and
    probably should bail if that fails, to be paranoid)

In the case of credentials we already have multiple mechanisms for
scripting the input (credential helpers and askpass). It would be nice
to be able to test the terminal-level code automatically, but I'm just
not sure how that would work.

-Peff

[PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN

[Ævar Arnfjörð Bjarmason]

I had a stupid bug in another series of mine[1] breaking code around
git_prompt() in bisect--helper.c, and in trying to follow-up to add
testing to that discovered that doing so was rather hard, due to how
git_prompt() behaves.

In another case in 2019 (which is the In-Reply-To here) we added a
seeming for-the-tests workaround for the same issue in "git am
--interacive".

This starts out by splitting the more complex "get a password
interactively" case out of prompt.c into a new prompt-password.c, most
of prompt.c's complexity was to cater to one API user in credential.c.

That being done move these callers to using git_prompt(), and have it
understanda a new GIT_TEST_TERMINAL_PROMPT=true variable so we can
have our cake and it it too in the test suite. If that's set the
prompt function will allow input on stdin.

This then allowed my to add the missing test for "git bisect" for the
code I'd accidentally broken in [1].

As noted in the discussion in 2/2 I'm not 100% sure this is the right
direction in terms of swiching us back to using /dev/tty and not
stdin, but see that commit & commit message for details. In any case,
whatever behavior we go for as far as the UX goes shouldn't be
catering only to the test suite at the cost of trade-offs elsewhere.

1. https://lore.kernel.org/git/cover-v6-0.8-00000000000-20211102T122507Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (2):
  prompt.c: split up the password and non-password handling
  prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true

 Makefile                    |  1 +
 builtin/am.c                |  8 ++--
 builtin/bisect--helper.c    |  8 +---
 credential.c                | 17 ++++----
 help.c                      |  2 +-
 prompt-password.c           | 63 +++++++++++++++++++++++++++++
 prompt-password.h           |  7 ++++
 prompt.c                    | 79 ++++++++-----------------------------
 prompt.h                    |  7 +---
 t/t6030-bisect-porcelain.sh | 41 +++++++++++++++++++
 t/test-lib.sh               |  4 ++
 11 files changed, 151 insertions(+), 86 deletions(-)
 create mode 100644 prompt-password.c
 create mode 100644 prompt-password.h

-- 
2.33.1.1570.g069344fdd45

[PATCH 1/2] prompt.c: split up the password and non-password handling

[Ævar Arnfjörð Bjarmason]

Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt()
let's split it up into git_prompt_echo() and git_prompt_noecho()
functions, the latter only ever needs to be called from credential.c,
and the same goes for the previous password-only codepath in
git_prompt(), which is now exposed as a git_prompt_password().

Doing this paves the way towards addressing some of the issues brought
up in 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                 |  1 +
 builtin/bisect--helper.c |  5 ++-
 credential.c             | 17 ++++++----
 help.c                   |  2 +-
 prompt-password.c        | 63 ++++++++++++++++++++++++++++++++++
 prompt-password.h        |  7 ++++
 prompt.c                 | 73 ++++++----------------------------------
 prompt.h                 |  7 ++--
 8 files changed, 97 insertions(+), 78 deletions(-)
 create mode 100644 prompt-password.c
 create mode 100644 prompt-password.h

diff --git a/Makefile b/Makefile
index 12be39ac497..e8815860e22 100644
--- a/Makefile
+++ b/Makefile
@@ -967,6 +967,7 @@ LIB_OBJS += pretty.o
 LIB_OBJS += prio-queue.o
 LIB_OBJS += progress.o
 LIB_OBJS += promisor-remote.o
+LIB_OBJS += prompt-password.o
 LIB_OBJS += prompt.o
 LIB_OBJS += protocol.o
 LIB_OBJS += protocol-caps.o
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28a2e6a5750..30533a70b53 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -370,7 +370,7 @@ static int decide_next(const struct bisect_terms *terms,
 		 * translation. The program will only accept English input
 		 * at this point.
 		 */
-		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
+		yesno = git_prompt_echo(_("Are you sure [Y/n]? "));
 		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
 			return -1;
 		return 0;
@@ -838,8 +838,7 @@ static int bisect_autostart(struct bisect_terms *terms)
 	 * translation. The program will only accept English input
 	 * at this point.
 	 */
-	yesno = git_prompt(_("Do you want me to do it for you "
-			     "[Y/n]? "), PROMPT_ECHO);
+	yesno = git_prompt_echo(_("Do you want me to do it for you [Y/n]? "));
 	res = tolower(*yesno) == 'n' ?
 		-1 : bisect_start(terms, empty_strvec, 0);
 
diff --git a/credential.c b/credential.c
index e7240f3f636..5509de382a5 100644
--- a/credential.c
+++ b/credential.c
@@ -5,6 +5,7 @@
 #include "run-command.h"
 #include "url.h"
 #include "prompt.h"
+#include "prompt-password.h"
 #include "sigchain.h"
 #include "urlmatch.h"
 
@@ -169,11 +170,11 @@ static void credential_format(struct credential *c, struct strbuf *out)
 	}
 }
 
-static char *credential_ask_one(const char *what, struct credential *c,
-				int flags)
+static char *credential_ask_one(struct credential *c, unsigned int password)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct strbuf prompt = STRBUF_INIT;
+	const char *what = password ? "Password" : "Username";
 	char *r;
 
 	credential_describe(c, &desc);
@@ -182,7 +183,11 @@ static char *credential_ask_one(const char *what, struct credential *c,
 	else
 		strbuf_addf(&prompt, "%s: ", what);
 
-	r = git_prompt(prompt.buf, flags);
+	/* We'll try "askpass" for both usernames and passwords */
+	r = git_prompt_askpass(prompt.buf);
+	if (!r)
+		r = password ? git_prompt_noecho(prompt.buf)
+			     : git_prompt_echo(prompt.buf);
 
 	strbuf_release(&desc);
 	strbuf_release(&prompt);
@@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c,
 static void credential_getpass(struct credential *c)
 {
 	if (!c->username)
-		c->username = credential_ask_one("Username", c,
-						 PROMPT_ASKPASS|PROMPT_ECHO);
+		c->username = credential_ask_one(c, 0);
 	if (!c->password)
-		c->password = credential_ask_one("Password", c,
-						 PROMPT_ASKPASS);
+		c->password = credential_ask_one(c, 1);
 }
 
 int credential_read(struct credential *c, FILE *fp)
diff --git a/help.c b/help.c
index 973e47cdc30..6eebc26fbeb 100644
--- a/help.c
+++ b/help.c
@@ -644,7 +644,7 @@ const char *help_unknown_cmd(const char *cmd)
 			char *answer;
 			struct strbuf msg = STRBUF_INIT;
 			strbuf_addf(&msg, _("Run '%s' instead? (y/N)"), assumed);
-			answer = git_prompt(msg.buf, PROMPT_ECHO);
+			answer = git_prompt_echo(msg.buf);
 			strbuf_release(&msg);
 			if (!(starts_with(answer, "y") ||
 			      starts_with(answer, "Y")))
diff --git a/prompt-password.c b/prompt-password.c
new file mode 100644
index 00000000000..5fa00363d6c
--- /dev/null
+++ b/prompt-password.c
@@ -0,0 +1,63 @@
+#include "cache.h"
+#include "prompt-password.h"
+#include "strbuf.h"
+#include "run-command.h"
+#include "prompt.h"
+
+static char *do_askpass(const char *cmd, const char *prompt)
+{
+	struct child_process pass = CHILD_PROCESS_INIT;
+	const char *args[3];
+	static struct strbuf buffer = STRBUF_INIT;
+	int err = 0;
+
+	args[0] = cmd;
+	args[1]	= prompt;
+	args[2] = NULL;
+
+	pass.argv = args;
+	pass.out = -1;
+
+	if (start_command(&pass))
+		return NULL;
+
+	strbuf_reset(&buffer);
+	if (strbuf_read(&buffer, pass.out, 20) < 0)
+		err = 1;
+
+	close(pass.out);
+
+	if (finish_command(&pass))
+		err = 1;
+
+	if (err) {
+		error("unable to read askpass response from '%s'", cmd);
+		strbuf_release(&buffer);
+		return NULL;
+	}
+
+	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
+
+	return buffer.buf;
+}
+
+char *git_prompt_askpass(const char *prompt)
+{
+	const char *askpass;
+	char *r = NULL;
+
+	askpass = getenv("GIT_ASKPASS");
+	if (!askpass)
+		askpass = askpass_program;
+	if (!askpass)
+		askpass = getenv("SSH_ASKPASS");
+	if (askpass && *askpass)
+		r = do_askpass(askpass, prompt);
+
+	return r;
+}
+
+char *git_prompt_noecho(const char *prompt)
+{
+	return git_prompt(prompt, 0);
+}
diff --git a/prompt-password.h b/prompt-password.h
new file mode 100644
index 00000000000..84933d2b7c5
--- /dev/null
+++ b/prompt-password.h
@@ -0,0 +1,7 @@
+#ifndef PROMPT_PASSWORD_H
+#define PROMPT_PASSWORD_H
+
+char *git_prompt_askpass(const char *prompt);
+char *git_prompt_noecho(const char *prompt);
+
+#endif
diff --git a/prompt.c b/prompt.c
index 5ded21a017f..458d6637506 100644
--- a/prompt.c
+++ b/prompt.c
@@ -1,78 +1,27 @@
 #include "cache.h"
 #include "config.h"
-#include "run-command.h"
 #include "strbuf.h"
 #include "prompt.h"
 #include "compat/terminal.h"
 
-static char *do_askpass(const char *cmd, const char *prompt)
+char *git_prompt(const char *prompt, unsigned int echo)
 {
-	struct child_process pass = CHILD_PROCESS_INIT;
-	const char *args[3];
-	static struct strbuf buffer = STRBUF_INIT;
-	int err = 0;
-
-	args[0] = cmd;
-	args[1]	= prompt;
-	args[2] = NULL;
-
-	pass.argv = args;
-	pass.out = -1;
-
-	if (start_command(&pass))
-		return NULL;
-
-	strbuf_reset(&buffer);
-	if (strbuf_read(&buffer, pass.out, 20) < 0)
-		err = 1;
-
-	close(pass.out);
-
-	if (finish_command(&pass))
-		err = 1;
+	char *r = NULL;
 
-	if (err) {
-		error("unable to read askpass response from '%s'", cmd);
-		strbuf_release(&buffer);
-		return NULL;
+	if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
+		r = git_terminal_prompt(prompt, echo);
+		if (!r)
+			die_errno("could not read");
+	} else {
+		die("could not read terminal prompts disabled");
 	}
 
-	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
-
-	return buffer.buf;
+	return r;
 }
 
-char *git_prompt(const char *prompt, int flags)
+char *git_prompt_echo(const char *prompt)
 {
-	char *r = NULL;
-
-	if (flags & PROMPT_ASKPASS) {
-		const char *askpass;
-
-		askpass = getenv("GIT_ASKPASS");
-		if (!askpass)
-			askpass = askpass_program;
-		if (!askpass)
-			askpass = getenv("SSH_ASKPASS");
-		if (askpass && *askpass)
-			r = do_askpass(askpass, prompt);
-	}
-
-	if (!r) {
-		const char *err;
-
-		if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
-			r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
-			err = strerror(errno);
-		} else {
-			err = "terminal prompts disabled";
-		}
-		if (!r) {
-			/* prompts already contain ": " at the end */
-			die("could not read %s%s", prompt, err);
-		}
-	}
-	return r;
+	return git_prompt(prompt, 1);
 }
 
 int git_read_line_interactively(struct strbuf *line)
diff --git a/prompt.h b/prompt.h
index e294e93541c..f4d38178bc4 100644
--- a/prompt.h
+++ b/prompt.h
@@ -1,11 +1,8 @@
 #ifndef PROMPT_H
 #define PROMPT_H
 
-#define PROMPT_ASKPASS (1<<0)
-#define PROMPT_ECHO    (1<<1)
-
-char *git_prompt(const char *prompt, int flags);
-
+char *git_prompt(const char *prompt, unsigned int echo);
+char *git_prompt_echo(const char *prompt);
 int git_read_line_interactively(struct strbuf *line);
 
 #endif /* PROMPT_H */
-- 
2.33.1.1570.g069344fdd45

Re: [PATCH 1/2] prompt.c: split up the password and non-password handling

[Jeff King]

On Tue, Nov 02, 2021 at 05:48:09PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt()
> let's split it up into git_prompt_echo() and git_prompt_noecho()
> functions, the latter only ever needs to be called from credential.c,
> and the same goes for the previous password-only codepath in
> git_prompt(), which is now exposed as a git_prompt_password().

I'm pretty "meh" on this direction. Moving from a flag field to separate
functions means a potential combinatoric explosion if new callers are
added later, or new flags are added.

And callers get more awkward, as the choices are no longer expressed
as data, so you need new conditionals like:

> -	r = git_prompt(prompt.buf, flags);
> +	/* We'll try "askpass" for both usernames and passwords */
> +	r = git_prompt_askpass(prompt.buf);
> +	if (!r)
> +		r = password ? git_prompt_noecho(prompt.buf)
> +			     : git_prompt_echo(prompt.buf);

And we lose the human-readable names for the flags in favor of ad-hoc
booleans:

> @@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c,
>  static void credential_getpass(struct credential *c)
>  {
>  	if (!c->username)
> -		c->username = credential_ask_one("Username", c,
> -						 PROMPT_ASKPASS|PROMPT_ECHO);
> +		c->username = credential_ask_one(c, 0);
>  	if (!c->password)
> -		c->password = credential_ask_one("Password", c,
> -						 PROMPT_ASKPASS);
> +		c->password = credential_ask_one(c, 1);

The only thing I do like here is that askpass can be split into its own
function (as in the first hunk above), and callers can simply decide
whether they need to go on to prompt at the terminal. But even there it
feels like closing a potential door around PROMPT_ECHO. The standard
askpass interface doesn't have a way for us to ask for it to show the
contents to the user (which is useful for the username prompt). But it
would be a reasonable extension to add one. Say, a GIT_ASKPASS_ECHO
that, if set, is preferred for the username prompt.

So I dunno. This seems somewhere between churn and making things worse,
and I don't see what it's buying us at all.

-Peff

Re: [PATCH 1/2] prompt.c: split up the password and non-password handling

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I dunno. This seems somewhere between churn and making things worse,
> and I don't see what it's buying us at all.

Unfortunately I have to agree with the summary.  Not a material I
want to spend time on this time in the cycle anyway X-<.

Thanks for reading.

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

[Ævar Arnfjörð Bjarmason]

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

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

[Jeff King]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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

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

[Jeff King]

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

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

[Jeff King]

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

[PATCH 4/4] am: fix --interactive HEAD tree resolution

[Jeff King]

In interactive mode, "git am -i --resolved" will try to generate a patch
based on what is in the index, so that it can prompt "apply this
patch?". To do so it needs the tree of HEAD, which it tries to get with
get_oid_tree(). However, this doesn't yield a tree oid; the "tree" part
just means "if you must disambiguate short oids, then prefer trees" (and
we do not need to disambiguate at all, since we are feeding a ref name).

Instead, we must parse the oid as a commit (which should always be true
in a non-corrupt repository), and access its tree pointer manually.

This has been broken since the conversion to C in 7ff2683253
(builtin-am: implement -i/--interactive, 2015-08-04), but there was no
test coverage because of interactive-mode's insistence on having a tty.
That was lifted in the previous commit, so we can now add a test for
this case.

Note that before this patch, the test would result in a BUG() which
comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty
tree, 2018-07-11). But before that, we'd have simply segfaulted (and in
fact this is the exact type of case the BUG() added there was trying to
catch!).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c              | 14 ++++++++---
 t/t4257-am-interactive.sh | 49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100755 t/t4257-am-interactive.sh

diff --git a/builtin/am.c b/builtin/am.c
index ea16b844f1..33bd7a6eab 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state)
 	struct rev_info rev_info;
 	FILE *fp;
 
-	if (!get_oid_tree("HEAD", &head))
-		tree = lookup_tree(the_repository, &head);
-	else
+	if (!get_oid("HEAD", &head)) {
+		struct object *obj;
+		struct commit *commit;
+
+		obj = parse_object_or_die(&head, NULL);
+		commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);
+		if (!commit)
+			die("unable to parse HEAD as a commit");
+
+		tree = get_commit_tree(commit);
+	} else
 		tree = lookup_tree(the_repository,
 				   the_repository->hash_algo->empty_tree);
 
diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh
new file mode 100755
index 0000000000..6989bf7aba
--- /dev/null
+++ b/t/t4257-am-interactive.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='am --interactive tests'
+. ./test-lib.sh
+
+test_expect_success 'set up patches to apply' '
+	test_commit unrelated &&
+	test_commit no-conflict &&
+	test_commit conflict-patch file patch &&
+	git format-patch --stdout -2 >mbox &&
+
+	git reset --hard unrelated &&
+	test_commit conflict-master file master base
+'
+
+# Sanity check our setup.
+test_expect_success 'applying all patches generates conflict' '
+	test_must_fail git am mbox &&
+	echo resolved >file &&
+	git add -u &&
+	git am --resolved
+'
+
+test_expect_success 'interactive am can apply a single patch' '
+	git reset --hard base &&
+	printf "%s\n" y n | git am -i mbox &&
+
+	echo no-conflict >expect &&
+	git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'interactive am can resolve conflict' '
+	git reset --hard base &&
+	printf "%s\n" y y | test_must_fail git am -i mbox &&
+	echo resolved >file &&
+	git add -u &&
+	printf "%s\n" v y | git am -i --resolved &&
+
+	echo conflict-patch >expect &&
+	git log -1 --format=%s >actual &&
+	test_cmp expect actual &&
+
+	echo resolved >expect &&
+	git cat-file blob HEAD:file >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.22.0.rc1.539.g7bfcdfe86d

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Johannes Schindelin]

Hi Peff,

On Mon, 20 May 2019, Jeff King wrote:

> In interactive mode, "git am -i --resolved" will try to generate a patch
> based on what is in the index, so that it can prompt "apply this
> patch?". To do so it needs the tree of HEAD, which it tries to get with
> get_oid_tree(). However, this doesn't yield a tree oid; the "tree" part
> just means "if you must disambiguate short oids, then prefer trees" (and
> we do not need to disambiguate at all, since we are feeding a ref name).
>
> Instead, we must parse the oid as a commit (which should always be true
> in a non-corrupt repository), and access its tree pointer manually.
>
> This has been broken since the conversion to C in 7ff2683253
> (builtin-am: implement -i/--interactive, 2015-08-04), but there was no
> test coverage because of interactive-mode's insistence on having a tty.
> That was lifted in the previous commit, so we can now add a test for
> this case.
>
> Note that before this patch, the test would result in a BUG() which
> comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty
> tree, 2018-07-11). But before that, we'd have simply segfaulted (and in
> fact this is the exact type of case the BUG() added there was trying to
> catch!).

What an old breakage! Thanks for analyzing and fixing it.

> diff --git a/builtin/am.c b/builtin/am.c
> index ea16b844f1..33bd7a6eab 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state)
>  	struct rev_info rev_info;
>  	FILE *fp;
>
> -	if (!get_oid_tree("HEAD", &head))
> -		tree = lookup_tree(the_repository, &head);
> -	else
> +	if (!get_oid("HEAD", &head)) {
> +		struct object *obj;
> +		struct commit *commit;
> +
> +		obj = parse_object_or_die(&head, NULL);
> +		commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);
> +		if (!commit)
> +			die("unable to parse HEAD as a commit");

Wouldn't this be easier to read like this:

		struct commit *commit =
			lookup_commit_reference(the_repository, &head);

> +
> +		tree = get_commit_tree(commit);
> +	} else
>  		tree = lookup_tree(the_repository,
>  				   the_repository->hash_algo->empty_tree);
>
> diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh
> new file mode 100755
> index 0000000000..6989bf7aba
> --- /dev/null
> +++ b/t/t4257-am-interactive.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='am --interactive tests'
> +. ./test-lib.sh
> +
> +test_expect_success 'set up patches to apply' '
> +	test_commit unrelated &&
> +	test_commit no-conflict &&
> +	test_commit conflict-patch file patch &&
> +	git format-patch --stdout -2 >mbox &&
> +
> +	git reset --hard unrelated &&
> +	test_commit conflict-master file master base
> +'
> +
> +# Sanity check our setup.
> +test_expect_success 'applying all patches generates conflict' '
> +	test_must_fail git am mbox &&
> +	echo resolved >file &&
> +	git add -u &&
> +	git am --resolved
> +'
> +
> +test_expect_success 'interactive am can apply a single patch' '
> +	git reset --hard base &&
> +	printf "%s\n" y n | git am -i mbox &&

Since we want contributors to copy-edit our test cases (even if they do
not happen to be Unix shell scripting experts), it would be better to
write

	test_write_lines y n | git am -i mbox &&

here. Same for similar `printf` invocations further down.

> +
> +	echo no-conflict >expect &&
> +	git log -1 --format=%s >actual &&
> +	test_cmp expect actual

I would prefer

	test no-conflict = "$(git show -s --format=%s HEAD)"

or even better:

test_cmp_head_oneline () {
	if test "$1" != "$(git show -s --format=%s HEAD)"
	then
		echo >&4 "HEAD's oneline is '$(git show -s \
			--format=%s HEAD)'; expected '$1'"
		return 1
	fi
}

> +'
> +
> +test_expect_success 'interactive am can resolve conflict' '
> +	git reset --hard base &&
> +	printf "%s\n" y y | test_must_fail git am -i mbox &&
> +	echo resolved >file &&
> +	git add -u &&
> +	printf "%s\n" v y | git am -i --resolved &&

Maybe a comment, to explain to the casual reader what the "v" and the "y"
are supposed to do?

> +
> +	echo conflict-patch >expect &&
> +	git log -1 --format=%s >actual &&
> +	test_cmp expect actual &&
> +
> +	echo resolved >expect &&
> +	git cat-file blob HEAD:file >actual &&
> +	test_cmp expect actual
> +'

After wrapping my head around the intentions of these commands, I agree
that they test for the right thing.

Thanks!
Dscho

> +
> +test_done
> --
> 2.22.0.rc1.539.g7bfcdfe86d
>

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Jeff King]

On Thu, May 23, 2019 at 09:12:27AM +0200, Johannes Schindelin wrote:

> > +	if (!get_oid("HEAD", &head)) {
> > +		struct object *obj;
> > +		struct commit *commit;
> > +
> > +		obj = parse_object_or_die(&head, NULL);
> > +		commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);
> > +		if (!commit)
> > +			die("unable to parse HEAD as a commit");
> 
> Wouldn't this be easier to read like this:
> 
> 		struct commit *commit =
> 			lookup_commit_reference(the_repository, &head);

Just the first two lines, I assume you mean; we still have to die
ourselves. There is a lookup_commit_or_die(), but weirdly it warns if
there was any tag dereferencing. I guess that would never happen here,
since we're reading HEAD (and most of the existing calls appear to be
for HEAD). So I'll go with that.

> > +test_expect_success 'interactive am can apply a single patch' '
> > +	git reset --hard base &&
> > +	printf "%s\n" y n | git am -i mbox &&
> 
> Since we want contributors to copy-edit our test cases (even if they do
> not happen to be Unix shell scripting experts), it would be better to
> write
> 
> 	test_write_lines y n | git am -i mbox &&
> 
> here. Same for similar `printf` invocations further down.

I think test_write_lines is mostly about avoiding echo chains, but it's
probably a little more readable to avoid having to say "\n". I'll adopt
that.

> > +	echo no-conflict >expect &&
> > +	git log -1 --format=%s >actual &&
> > +	test_cmp expect actual
> 
> I would prefer
> 
> 	test no-conflict = "$(git show -s --format=%s HEAD)"
> 
> or even better:
> 
> test_cmp_head_oneline () {
> 	if test "$1" != "$(git show -s --format=%s HEAD)"
> 	then
> 		echo >&4 "HEAD's oneline is '$(git show -s \
> 			--format=%s HEAD)'; expected '$1'"
> 		return 1
> 	fi
> }

This, I disagree with. IMHO comparing command output using "test" is
harder to read and produces worse debugging output (unless you do a
helper as you showed, which I think makes the readability even worse).
Not to mention that it raises questions of the shell's whitespace
handling (though that does not matter for this case).

What's your complaint with test_cmp? Is it the extra process? Could we
perhaps deal with that by having it use `read` for the happy-path?

Or do you prefer having a one-liner? I'd rather come up with a more
generic helper to cover this case, that can run any command and compare
it to a single argument (or stdin). E.g.,:

  test_cmp_cmd no-conflict git log -1 --format=%s

or

  test_cmp_cmd - git foo <<-\EOF
  multi-line
  expectation
  EOF

But I'd rather approach those issues separately and systematically, and
not hold up this bug fix.

> > +test_expect_success 'interactive am can resolve conflict' '
> > +	git reset --hard base &&
> > +	printf "%s\n" y y | test_must_fail git am -i mbox &&
> > +	echo resolved >file &&
> > +	git add -u &&
> > +	printf "%s\n" v y | git am -i --resolved &&
> 
> Maybe a comment, to explain to the casual reader what the "v" and the "y"
> are supposed to do?

OK. The "v" is actually optional, but I figured it would not hurt to
have us print the patch we just generated. I'll add a comment.

> After wrapping my head around the intentions of these commands, I agree
> that they test for the right thing.

Thanks!

-Peff

[PATCH v2 4/4] am: fix --interactive HEAD tree resolution

[Jeff King]

In --interactive mode, "git am --resolved" will try to generate a patch
based on what is in the index, so that it can prompt "apply this
patch?". To do so it needs the tree of HEAD, which it tries to get with
get_oid_tree(). However, this doesn't yield a tree object; the "tree"
part just means "if you must disambiguate short oids, then prefer trees"
(and we do not need to disambiguate at all, since we are feeding a ref).

Instead, we must parse the oid as a commit (which should always be true
in a non-corrupt repository), and access its tree pointer manually.

This has been broken since the conversion to C in 7ff2683253
(builtin-am: implement -i/--interactive, 2015-08-04), but there was no
test coverage because of interactive-mode's insistence on having a tty.
That was lifted in the previous commit, so we can now add a test for
this case.

Note that before this patch, the test would result in a BUG() which
comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty
tree, 2018-07-11). But before that, we'd have simply segfaulted (and in
fact this is the exact type of case the BUG() added there was trying to
catch!).

Signed-off-by: Jeff King <peff@peff.net>
---
This addresses points raised in Dscho's review (except for the test_cmp
thing). Since there were no changes to patches 1-3, I didn't bother
re-posting. Diff against v1 below.

  diff --git a/builtin/am.c b/builtin/am.c
  index 33bd7a6eab..78389d08b6 100644
  --- a/builtin/am.c
  +++ b/builtin/am.c
  @@ -1340,14 +1340,7 @@ static void write_index_patch(const struct am_state *state)
   	FILE *fp;
   
   	if (!get_oid("HEAD", &head)) {
  -		struct object *obj;
  -		struct commit *commit;
  -
  -		obj = parse_object_or_die(&head, NULL);
  -		commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);
  -		if (!commit)
  -			die("unable to parse HEAD as a commit");
  -
  +		struct commit *commit = lookup_commit_or_die(&head, "HEAD");
   		tree = get_commit_tree(commit);
   	} else
   		tree = lookup_tree(the_repository,
  diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh
  index 6989bf7aba..5344bd248a 100755
  --- a/t/t4257-am-interactive.sh
  +++ b/t/t4257-am-interactive.sh
  @@ -23,7 +23,8 @@ test_expect_success 'applying all patches generates conflict' '
   
   test_expect_success 'interactive am can apply a single patch' '
   	git reset --hard base &&
  -	printf "%s\n" y n | git am -i mbox &&
  +	# apply the first, but not the second
  +	test_write_lines y n | git am -i mbox &&
   
   	echo no-conflict >expect &&
   	git log -1 --format=%s >actual &&
  @@ -32,10 +33,12 @@ test_expect_success 'interactive am can apply a single patch' '
   
   test_expect_success 'interactive am can resolve conflict' '
   	git reset --hard base &&
  -	printf "%s\n" y y | test_must_fail git am -i mbox &&
  +	# apply both; the second one will conflict
  +	test_write_lines y y | test_must_fail git am -i mbox &&
   	echo resolved >file &&
   	git add -u &&
  -	printf "%s\n" v y | git am -i --resolved &&
  +	# interactive "--resolved" will ask us if we want to apply the result
  +	echo y | git am -i --resolved &&
   
   	echo conflict-patch >expect &&
   	git log -1 --format=%s >actual &&

 builtin/am.c              |  7 +++---
 t/t4257-am-interactive.sh | 52 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100755 t/t4257-am-interactive.sh

diff --git a/builtin/am.c b/builtin/am.c
index ea16b844f1..78389d08b6 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1339,9 +1339,10 @@ static void write_index_patch(const struct am_state *state)
 	struct rev_info rev_info;
 	FILE *fp;
 
-	if (!get_oid_tree("HEAD", &head))
-		tree = lookup_tree(the_repository, &head);
-	else
+	if (!get_oid("HEAD", &head)) {
+		struct commit *commit = lookup_commit_or_die(&head, "HEAD");
+		tree = get_commit_tree(commit);
+	} else
 		tree = lookup_tree(the_repository,
 				   the_repository->hash_algo->empty_tree);
 
diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh
new file mode 100755
index 0000000000..5344bd248a
--- /dev/null
+++ b/t/t4257-am-interactive.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='am --interactive tests'
+. ./test-lib.sh
+
+test_expect_success 'set up patches to apply' '
+	test_commit unrelated &&
+	test_commit no-conflict &&
+	test_commit conflict-patch file patch &&
+	git format-patch --stdout -2 >mbox &&
+
+	git reset --hard unrelated &&
+	test_commit conflict-master file master base
+'
+
+# Sanity check our setup.
+test_expect_success 'applying all patches generates conflict' '
+	test_must_fail git am mbox &&
+	echo resolved >file &&
+	git add -u &&
+	git am --resolved
+'
+
+test_expect_success 'interactive am can apply a single patch' '
+	git reset --hard base &&
+	# apply the first, but not the second
+	test_write_lines y n | git am -i mbox &&
+
+	echo no-conflict >expect &&
+	git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'interactive am can resolve conflict' '
+	git reset --hard base &&
+	# apply both; the second one will conflict
+	test_write_lines y y | test_must_fail git am -i mbox &&
+	echo resolved >file &&
+	git add -u &&
+	# interactive "--resolved" will ask us if we want to apply the result
+	echo y | git am -i --resolved &&
+
+	echo conflict-patch >expect &&
+	git log -1 --format=%s >actual &&
+	test_cmp expect actual &&
+
+	echo resolved >expect &&
+	git cat-file blob HEAD:file >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.22.0.rc1.549.gadb183c4cb

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Johannes Schindelin]

Hi Peff,

On Fri, 24 May 2019, Jeff King wrote:

> On Thu, May 23, 2019 at 09:12:27AM +0200, Johannes Schindelin wrote:
>
> > > +	echo no-conflict >expect &&
> > > +	git log -1 --format=%s >actual &&
> > > +	test_cmp expect actual
> >
> > I would prefer
> >
> > 	test no-conflict = "$(git show -s --format=%s HEAD)"
> >
> > or even better:
> >
> > test_cmp_head_oneline () {
> > 	if test "$1" != "$(git show -s --format=%s HEAD)"
> > 	then
> > 		echo >&4 "HEAD's oneline is '$(git show -s \
> > 			--format=%s HEAD)'; expected '$1'"
> > 		return 1
> > 	fi
> > }
>
> This, I disagree with. IMHO comparing command output using "test" is
> harder to read and produces worse debugging output (unless you do a
> helper as you showed, which I think makes the readability even worse).
> Not to mention that it raises questions of the shell's whitespace
> handling (though that does not matter for this case).
>
> What's your complaint with test_cmp? Is it the extra process? Could we
> perhaps deal with that by having it use `read` for the happy-path?

I would prefer it if we adopted a more descriptive style in the test
suite, as I always found that style to be much easier to work with (you
might have guessed that I am spending a lot of time chasing test
failures).

Succinctness is just one benefit of that.

A more important benefit is that you can teach a helper that verifies
onelines to show very useful information in case of a failure, something
that `test_cmp` cannot do because it is totally agnostic to what it
compares.

> Or do you prefer having a one-liner? I'd rather come up with a more
> generic helper to cover this case, that can run any command and compare
> it to a single argument (or stdin). E.g.,:
>
>   test_cmp_cmd no-conflict git log -1 --format=%s
>
> or
>
>   test_cmp_cmd - git foo <<-\EOF
>   multi-line
>   expectation
>   EOF

I guess that you and me go into completely opposite directions here. I
want something *less* general. Because I want to optimize for the
unfortunate times when a test fails and most likely somebody else than the
original author of the test case is tasked with figuring out what the heck
goes wrong.

You seem to want to optimize for writing test cases. Which I find -- with
all due respect -- the wrong thing to optimize for. It is already dirt
easy to write new test cases. But *good* test cases (i.e. easy to debug
ones)? Not so much.

> But I'd rather approach those issues separately and systematically, and
> not hold up this bug fix.

Sure.

> > > +test_expect_success 'interactive am can resolve conflict' '
> > > +	git reset --hard base &&
> > > +	printf "%s\n" y y | test_must_fail git am -i mbox &&
> > > +	echo resolved >file &&
> > > +	git add -u &&
> > > +	printf "%s\n" v y | git am -i --resolved &&
> >
> > Maybe a comment, to explain to the casual reader what the "v" and the "y"
> > are supposed to do?
>
> OK. The "v" is actually optional, but I figured it would not hurt to
> have us print the patch we just generated. I'll add a comment.

Thank you.

Ciao,
Dscho

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Jeff King]

On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote:

> > Or do you prefer having a one-liner? I'd rather come up with a more
> > generic helper to cover this case, that can run any command and compare
> > it to a single argument (or stdin). E.g.,:
> >
> >   test_cmp_cmd no-conflict git log -1 --format=%s
> >
> > or
> >
> >   test_cmp_cmd - git foo <<-\EOF
> >   multi-line
> >   expectation
> >   EOF
> 
> I guess that you and me go into completely opposite directions here. I
> want something *less* general. Because I want to optimize for the
> unfortunate times when a test fails and most likely somebody else than the
> original author of the test case is tasked with figuring out what the heck
> goes wrong.
> 
> You seem to want to optimize for writing test cases. Which I find -- with
> all due respect -- the wrong thing to optimize for. It is already dirt
> easy to write new test cases. But *good* test cases (i.e. easy to debug
> ones)? Not so much.

Hmm. I too want the test output to be useful to people other than the
test author. But I find the output from test_cmp perfectly fine there.
My first step in digging into a failure is usually to look at what
commands the test is running, which generally makes it obvious why we
are expecting one thing and seeing another (or at least, just as obvious
as a hand-written message).

So to me the two are equal on that front, which makes me want to go with
the thing that is shorter to write, as it makes it more likely the test
writer will write it. The _worst_ option IMHO is a straight-up use of
"test" which provides no output at all in the test log of what value we
_did_ see. That requires the person looking into the failure to re-run
the test, which is hard if it's a remote CI, or if the failure does not
always reproduce.

-Peff

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Johannes Schindelin]

Hi Peff,

On Tue, 28 May 2019, Jeff King wrote:

> On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote:
>
> > > Or do you prefer having a one-liner? I'd rather come up with a more
> > > generic helper to cover this case, that can run any command and compare
> > > it to a single argument (or stdin). E.g.,:
> > >
> > >   test_cmp_cmd no-conflict git log -1 --format=%s
> > >
> > > or
> > >
> > >   test_cmp_cmd - git foo <<-\EOF
> > >   multi-line
> > >   expectation
> > >   EOF
> >
> > I guess that you and me go into completely opposite directions here. I
> > want something *less* general. Because I want to optimize for the
> > unfortunate times when a test fails and most likely somebody else than the
> > original author of the test case is tasked with figuring out what the heck
> > goes wrong.
> >
> > You seem to want to optimize for writing test cases. Which I find -- with
> > all due respect -- the wrong thing to optimize for. It is already dirt
> > easy to write new test cases. But *good* test cases (i.e. easy to debug
> > ones)? Not so much.
>
> Hmm. I too want the test output to be useful to people other than the
> test author. But I find the output from test_cmp perfectly fine there.
> My first step in digging into a failure is usually to look at what
> commands the test is running, which generally makes it obvious why we
> are expecting one thing and seeing another (or at least, just as obvious
> as a hand-written message).
>
> So to me the two are equal on that front, which makes me want to go with
> the thing that is shorter to write, as it makes it more likely the test
> writer will write it. The _worst_ option IMHO is a straight-up use of
> "test" which provides no output at all in the test log of what value we
> _did_ see. That requires the person looking into the failure to re-run
> the test, which is hard if it's a remote CI, or if the failure does not
> always reproduce.

If you think your version is easier to debug, then I won't object.

Thanks,
Dscho

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Alejandro Sanchez]

Hi,

Are there any updates to this problem?

Thank you,
Alex


On Wed, May 29, 2019 at 1:57 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Peff,
>
> On Tue, 28 May 2019, Jeff King wrote:
>
> > On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote:
> >
> > > > Or do you prefer having a one-liner? I'd rather come up with a more
> > > > generic helper to cover this case, that can run any command and compare
> > > > it to a single argument (or stdin). E.g.,:
> > > >
> > > >   test_cmp_cmd no-conflict git log -1 --format=%s
> > > >
> > > > or
> > > >
> > > >   test_cmp_cmd - git foo <<-\EOF
> > > >   multi-line
> > > >   expectation
> > > >   EOF
> > >
> > > I guess that you and me go into completely opposite directions here. I
> > > want something *less* general. Because I want to optimize for the
> > > unfortunate times when a test fails and most likely somebody else than the
> > > original author of the test case is tasked with figuring out what the heck
> > > goes wrong.
> > >
> > > You seem to want to optimize for writing test cases. Which I find -- with
> > > all due respect -- the wrong thing to optimize for. It is already dirt
> > > easy to write new test cases. But *good* test cases (i.e. easy to debug
> > > ones)? Not so much.
> >
> > Hmm. I too want the test output to be useful to people other than the
> > test author. But I find the output from test_cmp perfectly fine there.
> > My first step in digging into a failure is usually to look at what
> > commands the test is running, which generally makes it obvious why we
> > are expecting one thing and seeing another (or at least, just as obvious
> > as a hand-written message).
> >
> > So to me the two are equal on that front, which makes me want to go with
> > the thing that is shorter to write, as it makes it more likely the test
> > writer will write it. The _worst_ option IMHO is a straight-up use of
> > "test" which provides no output at all in the test log of what value we
> > _did_ see. That requires the person looking into the failure to re-run
> > the test, which is hard if it's a remote CI, or if the failure does not
> > always reproduce.
>
> If you think your version is easier to debug, then I won't object.
>
> Thanks,
> Dscho

Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution

[Jeff King]

On Thu, Sep 26, 2019 at 04:20:05PM +0200, Alejandro Sanchez wrote:

> Are there any updates to this problem?

The fix for the original bug went into Git v2.22.1.

-Peff