Re: [PATCH v2 2/2] merge-base: "--reflog" mode finds fork point from reflog entries

[John Keeping <john@keeping.me.uk>]

On Fri, Oct 25, 2013 at 09:12:10AM +0200, Johannes Sixt wrote:
> Am 10/25/2013 0:21, schrieb Junio C Hamano:
> > +test_expect_success 'using reflog to find the fork point' '
> > +	git reset --hard &&
> > +	git checkout -b base $E &&
> > +
> > +	for count in 1 2 3 4 5
> > +	do
> > +		git commit --allow-empty -m "Base commit #$count" &&
> > +		git rev-parse HEAD >expect$count &&
> > +		git checkout -B derived &&
> > +		git commit --allow-empty -m "Derived #$count" &&
> > +		git rev-parse HEAD >derived$count &&
> > +		git checkout base || break
> > +	done &&
> > +
> > +	for count in 1 2 3 4 5
> > +	do
> > +		git merge-base --reflog base $(cat derived$count) >actual &&
> > +		test_cmp expect$count actual || break
> > +	done &&
> 
> This does not work as intended because the exit code of 'break' is always
> zero. Unlike 'exit' and 'return', it does *not* pick up the exit code of
> the last command that was executed.
> 
> That's annoying, but makes some sense because 'break 2' does not mean to
> apply exit code 2 to the command, either, but to break out of 2 levels of
> loops.
> 
> You could put the loops into a function from which you 'return', but that
> is obscure in this case. The first iteration was better, IMO.

Wouldn't it be simpler to just return from the test?  That is, replace
the "break" in the above patch with "return 1".

There's code in several other test cases (e.g. t3311) which handles the
same problem like that.