git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Alejandro Sanchez <asanchez1987@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution
Date: Fri, 24 May 2019 02:39:55 -0400
Message-ID: <20190524063955.GD25694@sigill.intra.peff.net> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1905230858570.46@tvgsbejvaqbjf.bet>

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

  reply	other threads:[~2019-05-24  6:39 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
2019-05-24  6:39         ` Jeff King [this message]
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=20190524063955.GD25694@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=asanchez1987@gmail.com \
    --cc=git@vger.kernel.org \
    /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