Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 1, 2017, 12:26 UTC
Message-ID: <alpine.DEB.2.21.1.1711011316590.6482@virtualbox>
In-Reply-To: <xmqqr2tiveqp.fsf@gitster.mtv.corp.google.com>

Hi,

On Wed, 1 Nov 2017, Junio C Hamano wrote:

Show 31 quoted lines

> Stefan Beller <sbeller@google.com> writes:
> 
> > diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
> > new file mode 100755
> > index 0000000000..651666979b
> > --- /dev/null
> > +++ b/t/t6100-rev-list-in-order.sh
> > @@ -0,0 +1,46 @@
> > +#!/bin/sh
> > +
> > +test_description='miscellaneous rev-list tests'
> > +
> > +. ./test-lib.sh
> > +
> > +
> > +test_expect_success 'setup' '
> > +	for x in one two three four
> > +	do
> > +		echo $x >$x &&
> > +		git add $x &&
> > +		git commit -m "add file $x"
> > +	done &&
> > +	for x in four three
> > +	do
> > +		git rm $x &&
> > +		git commit -m "remove $x"
> > +	done &&
> 
> When "git commit -m 'remove four'" fails, this loop would still
> continue, so the &&-chain in "done &&" would still be rendered
> ineffetive.

... so the proper fix is to append an "|| break", as in:

	...
	for x in four three
	do
		git rm $x &&
		git commit -m "remote $x" ||
		break
	done &&
	...

But that is still incorrect, as the "break" statement will succeed, breaking (pun intended) the && chain in a different way. The canonical way to do it is to wrap the loop in a subshell, *and also* test $? (because `(exit 1) && echo something` still prints `something`):

	...
	(
		for x in four three
		do
			git rm $x &&
			git commit -m "remote $x" ||
			exit
		done
	) &&
	test 0 -eq $? &&
	...

Ugly? Yes. Correct? Also yes.

Ciao, Dscho