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

[Junio C Hamano]

Johannes Schindelin  writes:

>> > Given that the test_expect_* functions evaluate the code, it makes me
>> > wonder whether those `return` statements really return appropriately, or
>> > one call level too low.
>> 
>> The test_expect functions eval the actual snippets inside a dummy
>> function. This is intentional exactly to allow them to call "return" at
>> will.
>
> Tricksy. And a bit unintuitive for script kings such as myself, but okay.

Exactly.

The hidden assumption on the way how "return" interacts with
the way we use "eval" is the reason why I said "Ugly? Yes, Correct?
Questionable" in the first place.

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

[Stefan Beller]

On Wed, Nov 1, 2017 at 3:39 PM, Johannes Schindelin
 wrote:
>> not ok 1 - witty title
>>
>> That is all we want to care about here?
>
> We care about the loop body being executed successfully *each time*. A
> better counter example:

Good point. I'll use return in that case. Thanks!

Stefan

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

[Johannes Schindelin]

Hi Stefan,

On Wed, 1 Nov 2017, Stefan Beller wrote:

> We do not care about the internal state, aborting early, we rather
> care only if the whole loop body was executed. Running the test
> 
> test_expect_success 'witty title' '
> for a in 1 2 3; do echo $a && false; done && echo done
> '
> 
> not ok 1 - witty title
> 
> That is all we want to care about here?

We care about the loop body being executed successfully *each time*. A
better counter example:

$ for a in 1 2 3; do echo $a && test 2 != $a || false; done && echo done
1
2
3
done

(even if the second of three runs failed)

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Wed, 1 Nov 2017, Jeff King wrote:

> On Wed, Nov 01, 2017 at 10:36:02PM +0100, Johannes Schindelin wrote:
> 
> > On Wed, 1 Nov 2017, Junio C Hamano wrote:
> > 
> > > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin
> > >  wrote:
> > > >
> > > > ...
> > > > (
> > > > for x in four three
> > > > do
> > > > git rm $x &&
> > > > git commit -m "remote $x" ||
> > > > exit
> > > > done
> > > > ) &&
> > > > test 0 -eq $? &&
> > > > ...
> > > >
> > > > Ugly? Yes. Correct? Also yes.
> > > 
> > > I think returning non-zero with "return" is how other tests avoid an
> > > extra level of subshell.
> > > Ugly? Yes. Correct? Questionable but it seems to work for those who
> > > wrote them ;-)
> > 
> > Given that the test_expect_* functions evaluate the code, it makes me
> > wonder whether those `return` statements really return appropriately, or
> > one call level too low.
> 
> The test_expect functions eval the actual snippets inside a dummy
> function. This is intentional exactly to allow them to call "return" at
> will.

Tricksy. And a bit unintuitive for script kings such as myself, but okay.

Ciao,
Dscho

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

[Stefan Beller]

>> I think I'll go without the extra subshell and with s/return 1/false/ as
>> the exact value doesn't matter.
>
> You mean
>
> for ...
> do
> xyz ||
> false
> done

Yes, I do.

> ? That does not work. Try this:
>
> for a in 1 2 3; do echo $a && false; done && echo done

...   && echo $?
 1

> While it does not print `done`, it will print all of 1, 2 and 3.

We do not care about the internal state, aborting early, we rather
care only if the whole loop body was executed. Running the test

test_expect_success 'witty title' '
for a in 1 2 3; do echo $a && false; done && echo done
'

not ok 1 - witty title

That is all we want to care about here?
Otherwise I may still have a misunderstanding.

Thanks,
Stefan

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

[Johannes Schindelin]

Hi Stefan,

On Wed, 1 Nov 2017, Stefan Beller wrote:

> On Wed, Nov 1, 2017 at 5:37 AM, Junio C Hamano  wrote:
> > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin
> >  wrote:
> >>
> >> ...
> >> (
> >> for x in four three
> >> do
> >> git rm $x &&
> >> git commit -m "remote $x" ||
> >> exit
> >> done
> >> ) &&
> >> test 0 -eq $? &&
> >> ...
> >>
> >> Ugly? Yes. Correct? Also yes.
> >
> > I think returning non-zero with "return" is how other tests avoid an
> > extra level of subshell.
> > Ugly? Yes. Correct? Questionable but it seems to work for those who
> > wrote them ;-)
> 
> I think I'll go without the extra subshell and with s/return 1/false/ as
> the exact value doesn't matter.

You mean

for ...
do
xyz ||
false
done

? That does not work. Try this:

for a in 1 2 3; do echo $a && false; done && echo done

While it does not print `done`, it will print all of 1, 2 and 3.

Ciao,
Dscho

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

[Jeff King]

On Wed, Nov 01, 2017 at 10:36:02PM +0100, Johannes Schindelin wrote:

> Hi Junio,
> 
> On Wed, 1 Nov 2017, Junio C Hamano wrote:
> 
> > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin
> >  wrote:
> > >
> > > ...
> > > (
> > > for x in four three
> > > do
> > > git rm $x &&
> > > git commit -m "remote $x" ||
> > > exit
> > > done
> > > ) &&
> > > test 0 -eq $? &&
> > > ...
> > >
> > > Ugly? Yes. Correct? Also yes.
> > 
> > I think returning non-zero with "return" is how other tests avoid an
> > extra level of subshell.
> > Ugly? Yes. Correct? Questionable but it seems to work for those who
> > wrote them ;-)
> 
> Given that the test_expect_* functions evaluate the code, it makes me
> wonder whether those `return` statements really return appropriately, or
> one call level too low.

The test_expect functions eval the actual snippets inside a dummy
function. This is intentional exactly to allow them to call "return" at
will.

-Peff

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

[Johannes Schindelin]

Hi Junio,

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

> On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin
>  wrote:
> >
> > ...
> > (
> > for x in four three
> > do
> > git rm $x &&
> > git commit -m "remote $x" ||
> > exit
> > done
> > ) &&
> > test 0 -eq $? &&
> > ...
> >
> > Ugly? Yes. Correct? Also yes.
> 
> I think returning non-zero with "return" is how other tests avoid an
> extra level of subshell.
> Ugly? Yes. Correct? Questionable but it seems to work for those who
> wrote them ;-)

Given that the test_expect_* functions evaluate the code, it makes me
wonder whether those `return` statements really return appropriately, or
one call level too low.

Ciao,
Dscho

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

[Stefan Beller]

On Wed, Nov 1, 2017 at 5:37 AM, Junio C Hamano  wrote:
> On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin
>  wrote:
>>
>> ...
>> (
>> for x in four three
>> do
>> git rm $x &&
>> git commit -m "remote $x" ||
>> exit
>> done
>> ) &&
>> test 0 -eq $? &&
>> ...
>>
>> Ugly? Yes. Correct? Also yes.
>
> I think returning non-zero with "return" is how other tests avoid an
> extra level of subshell.
> Ugly? Yes. Correct? Questionable but it seems to work for those who
> wrote them ;-)

I think I'll go without the extra subshell and with s/return 1/false/ as
the exact value doesn't matter.

Thanks for hinting at correct implementations

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

[Junio C Hamano]

On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin
 wrote:
>
> ...
> (
> for x in four three
> do
> git rm $x &&
> git commit -m "remote $x" ||
> exit
> done
> ) &&
> test 0 -eq $? &&
> ...
>
> Ugly? Yes. Correct? Also yes.

I think returning non-zero with "return" is how other tests avoid an
extra level of subshell.
Ugly? Yes. Correct? Questionable but it seems to work for those who
wrote them ;-)

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  writes:
> 
> > diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
> > new file mode 100755
> > index 00..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

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

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
> new file mode 100755
> index 00..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.

> + git rev-list --in-commit-order --objects HEAD >actual.raw &&
> + cut -c 1-40 >actual  +
> + git cat-file --batch-check="%(objectname)" >expect.raw <<-\EOF &&
> + HEAD^{commit}
> + HEAD^{tree}
> + HEAD^{tree}:one
> + HEAD^{tree}:two
> + HEAD~1^{commit}
> + HEAD~1^{tree}
> + HEAD~1^{tree}:three
> + HEAD~2^{commit}
> + HEAD~2^{tree}
> + HEAD~2^{tree}:four
> + HEAD~3^{commit}
> + # HEAD~3^{tree} skipped
> + HEAD~4^{commit}
> + # HEAD~4^{tree} skipped
> + HEAD~5^{commit}
> + HEAD~5^{tree}
> + EOF
> + grep -v "#" >expect  +
> + test_cmp expect actual
> +'
> +
> +test_done