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

[Johannes Schindelin]

Hi,

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

> Stefan Beller <sbel...@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