From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jeff King <peff@peff.net>
Cc: Alejandro Sanchez <asanchez1987@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution
Date: Thu, 23 May 2019 09:12:27 +0200 (CEST) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.1905230858570.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190520121301.GD11212@sigill.intra.peff.net>
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
>
next prev parent reply other threads:[~2019-05-23 7:12 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-20 8:35 Abort (core dumped) Alejandro Sanchez
2019-05-20 10:02 ` Jeff King
2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King
2019-05-20 12:07 ` [PATCH 1/4] am: simplify prompt response handling Jeff King
2019-05-20 12:09 ` [PATCH 2/4] am: read interactive input from stdin Jeff King
2019-05-20 12:11 ` [PATCH 3/4] am: drop tty requirement for --interactive Jeff King
2019-05-20 12:50 ` Jeff King
2019-05-23 6:44 ` Johannes Schindelin
2019-05-24 6:27 ` Jeff King
2021-11-02 16:48 ` [PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN Ævar Arnfjörð Bjarmason
2021-11-02 16:48 ` [PATCH 1/2] prompt.c: split up the password and non-password handling Ævar Arnfjörð Bjarmason
2021-11-03 11:53 ` Jeff King
2021-11-03 17:28 ` Junio C Hamano
2021-11-02 16:48 ` [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true Ævar Arnfjörð Bjarmason
2021-11-03 11:57 ` Jeff King
2021-11-03 15:12 ` Ævar Arnfjörð Bjarmason
2021-11-04 9:58 ` Jeff King
2021-11-03 17:42 ` Junio C Hamano
2021-11-04 8:48 ` Johannes Schindelin
2021-11-04 9:47 ` Jeff King
2021-11-04 9:53 ` Jeff King
2019-05-20 12:13 ` [PATCH 4/4] am: fix --interactive HEAD tree resolution Jeff King
2019-05-23 7:12 ` Johannes Schindelin [this message]
2019-05-24 6:39 ` Jeff King
2019-05-24 6:46 ` [PATCH v2 " Jeff King
2019-05-28 11:06 ` [PATCH " Johannes Schindelin
2019-05-28 21:35 ` Jeff King
2019-05-29 11:56 ` Johannes Schindelin
2019-09-26 14:20 ` Alejandro Sanchez
2019-09-26 21:11 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.1905230858570.46@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=asanchez1987@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).