Re: [PATCH 08/12] t5520: use test_cmp_rev where possible

From: Denton Liu <liu.denton@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 08/12] t5520: use test_cmp_rev where possible
Date: Fri, 18 Oct 2019 11:52:27 -0700	[thread overview]
Message-ID: <20191018185227.GA74877@generichostname> (raw)
In-Reply-To: <CAPig+cRF0taNhmZVu0z+P+Oe3em11Jz33GY8eBZRTGkx3z=TOw@mail.gmail.com>

Hi Eric,

Thanks for the reviews. I have no idea how you always get to my patches
so quickly but I appreciate the prompt reviews whenever I send a
test-related patchset in.

I've fixed up the other concerns you had and I'll send a v2 later but I
wanted to address this one.

On Thu, Oct 17, 2019 at 07:41:44PM -0400, Eric Sunshine wrote:
> On Thu, Oct 17, 2019 at 7:17 PM Denton Liu <liu.denton@gmail.com> wrote:
> > In case an invocation of `git rev-list` fails within the subshell, the
> > failure will be masked. Remove the subshell and use test_cmp_rev() so
> > that failures can be discovered.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> > @@ -597,10 +597,10 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
> >         test_must_fail git pull &&
> > -       test "$COPY" = "$(git rev-parse --verify me/copy)" &&
> > +       test_cmp_rev "$COPY" me/copy &&
> 
> This transformation doesn't look correct. COPY already holds the
> result of a git-rev-parse invocation:
> 
>     COPY="$(git rev-parse --verify me/copy)" &&
> 
> so passing it to test_cmp_rev() -- which applies its own git-rev-parse
> invocation -- doesn't make sense.

I'll annotate the entire test case with my comments:

	# so we'll be testing that pull --rebase dies early
	test_expect_success 'pull --rebase dies early with dirty working directory' '
		git checkout to-rebase &&
		git update-ref refs/remotes/me/copy copy^ &&
		COPY="$(git rev-parse --verify me/copy)" &&
		git rebase --onto $COPY copy &&

		# according to git log --graph, we have this currently:
		#
		# $ git log --pretty=oneline --graph to-rebase copy me/copy
		# * 9366795adb50ef6eb482b610b37cb1fb6edbd3d0 (HEAD -> to-rebase) to-rebase
		# * b3dcf50eea47c4ad85faabb0fb74eded71cc829f file3
		# * faf459539411b4557cf735232a3746be073177a9 new file
		# | * f340b1b8932f7b1e016c06867cbdc3f637eeea2d (copy) conflict
		# |/  
		# * 5e86d50a28977ebee5ea378f81591ea558149272 (me/second, me/copy, second) modified
		# * d25cf184567afad1dc1e018b2b7d793bc1bd2dc1 original

		test_config branch.to-rebase.remote me &&
		test_config branch.to-rebase.merge refs/heads/copy &&
		test_config branch.to-rebase.rebase true &&

		# we make the tree dirty here
		echo dirty >>file &&
		git add file &&

		# and over here, the pull should fail
		test_must_fail git pull &&

		# but since we're testing that it dies early, we want to
		# make sure that the remote ref doesn't change, which is
		# why it should still be equal
		test_cmp_rev "$COPY" me/copy &&

		git checkout HEAD -- file &&

		# but over here, the pull succeeds...
		git pull &&

		# so as a result, the remote ref should now be updated
		test_cmp_rev ! "$COPY" me/copy
	'

So after grokking the test case, it seems like the the transformation is
indeed correct. Maybe we can replace the last line with

	test_cmp_rev copy me/copy

but I think I'll leave it unless you have any strong opinions.

Thanks,

Denton