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

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
May 23, 2019, 07:12 UTC
Message-ID
<nycvar.QRO.7.76.6.1905230858570.46@tvgsbejvaqbjf.bet>
In-Reply-To
<20190520121301.GD11212@sigill.intra.peff.net>
Hi Peff,
On Mon, 20 May 2019, Jeff King wrote:
Show 21 quoted lines
> 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.
Show 19 quoted lines
> 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);
Show 38 quoted lines
> +
> +		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
}
Show 8 quoted lines
> +'
> +
> +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?

Show 9 quoted lines
> +
> +	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

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

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

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