git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
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)
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
>

  reply	other threads:[~2019-05-23  7:12 UTC|newest]

Thread overview: 18+ 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
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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git