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
>
