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