Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Wed, Oct 5, 2016 at 7:40 AM, Jeff King  wrote:
> On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:
>
>> I would prefer the following:
>>
>> #   A --> B --> C --> D --> E --> F --> G --> H
>> #  0 1 2 3 4 5 6
>
> Yeah, that is also more visually pleasing.
>
> Here's a squashable update that uses that and clarifies the points in
> the discussion with Jacob.
>
> Junio, do you mind squashing this in to jk/alt-odb-cleanup?
>
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index b393613..62170b7 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '
> )
>  '
>
> -# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
> start
> -# the depth at 0 and count links, not repositories, so in a chain like:
> +# Note: These tests depend on the hard-coded value of 5 as the maximum depth
> +# we will follow recursion. We start the depth at 0 and count links, not
> +# repositories. This means that in a chain like:
>  #
> -#   A -> B -> C -> D -> E -> F -> G -> H
> -#  0123456
> +#   A --> B --> C --> D --> E --> F --> G --> H
> +#  0 1 2 3 4 5 6

Yea this looks much better (when I view it locally, gmail still looks
aweful here but...)

>  #
> -# we are OK at "G", but break at "H".
> +# we are OK at "G", but break at "H", even though "H" is actually the 8th
> +# repository, not the 6th, which you might expect. Counting the links allows
> +# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.
>  #

... This is much more clear wording that helps me understand this a
lot more. Thanks!

Regards,
Jake

>  # Note also that we must use "--bare -l" to make the link to H. The "-l"
>  # ensures we do not do a connectivity check, and the "--bare" makes sure
> @@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '
> git clone --bare -l -s G H
>  '
>
> -test_expect_success 'validity of fifth-deep repository' '
> +test_expect_success 'validity of seventh repository' '
> git -C G fsck
>  '
>
> -test_expect_success 'invalidity of sixth-deep repository' '
> +test_expect_success 'invalidity of eighth repository' '
> test_must_fail git -C H fsck
>  '
>

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Junio C Hamano]

Jeff King  writes:

> On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:
>
>> I would prefer the following:
>> 
>> #   A --> B --> C --> D --> E --> F --> G --> H
>> #  0 1 2 3 4 5 6
>
> Yeah, that is also more visually pleasing.
>
> Here's a squashable update that uses that and clarifies the points in
> the discussion with Jacob.
>
> Junio, do you mind squashing this in to jk/alt-odb-cleanup?

No, I don't.

> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index b393613..62170b7 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '
>   )
>  '
>  
> -# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
> start
> -# the depth at 0 and count links, not repositories, so in a chain like:
> +# Note: These tests depend on the hard-coded value of 5 as the maximum depth
> +# we will follow recursion. We start the depth at 0 and count links, not
> +# repositories. This means that in a chain like:
>  #
> -#   A -> B -> C -> D -> E -> F -> G -> H
> -#  0123456
> +#   A --> B --> C --> D --> E --> F --> G --> H
> +#  0 1 2 3 4 5 6
>  #
> -# we are OK at "G", but break at "H".
> +# we are OK at "G", but break at "H", even though "H" is actually the 8th
> +# repository, not the 6th, which you might expect. Counting the links allows
> +# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.
>  #
>  # Note also that we must use "--bare -l" to make the link to H. The "-l"
>  # ensures we do not do a connectivity check, and the "--bare" makes sure
> @@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '
>   git clone --bare -l -s G H
>  '
>  
> -test_expect_success 'validity of fifth-deep repository' '
> +test_expect_success 'validity of seventh repository' '
>   git -C G fsck
>  '
>  
> -test_expect_success 'invalidity of sixth-deep repository' '
> +test_expect_success 'invalidity of eighth repository' '
>   test_must_fail git -C H fsck
>  '
>  

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:

> I would prefer the following:
> 
> #   A --> B --> C --> D --> E --> F --> G --> H
> #  0 1 2 3 4 5 6

Yeah, that is also more visually pleasing.

Here's a squashable update that uses that and clarifies the points in
the discussion with Jacob.

Junio, do you mind squashing this in to jk/alt-odb-cleanup?

diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index b393613..62170b7 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '
)
 '
 
-# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
-# the depth at 0 and count links, not repositories, so in a chain like:
+# Note: These tests depend on the hard-coded value of 5 as the maximum depth
+# we will follow recursion. We start the depth at 0 and count links, not
+# repositories. This means that in a chain like:
 #
-#   A -> B -> C -> D -> E -> F -> G -> H
-#  0123456
+#   A --> B --> C --> D --> E --> F --> G --> H
+#  0 1 2 3 4 5 6
 #
-# we are OK at "G", but break at "H".
+# we are OK at "G", but break at "H", even though "H" is actually the 8th
+# repository, not the 6th, which you might expect. Counting the links allows
+# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.
 #
 # Note also that we must use "--bare -l" to make the link to H. The "-l"
 # ensures we do not do a connectivity check, and the "--bare" makes sure
@@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '
git clone --bare -l -s G H
 '
 
-test_expect_success 'validity of fifth-deep repository' '
+test_expect_success 'validity of seventh repository' '
git -C G fsck
 '
 
-test_expect_success 'invalidity of sixth-deep repository' '
+test_expect_success 'invalidity of eighth repository' '
test_must_fail git -C H fsck
 '
 

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jakub Narębski]

W dniu 04.10.2016 o 22:58, Stefan Beller pisze:
> On Tue, Oct 4, 2016 at 1:55 PM, Jeff King  wrote:
>> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
>>
 +# Note: These tests depend on the hard-coded value of 5 as "too 
 deep". We start
 +# the depth at 0 and count links, not repositories, so in a chain 
 like:
 +#
 +#   A -> B -> C -> D -> E -> F -> G -> H
 +#  0123456
 +#

> 
> Input from a self-claimed design expert for ASCII art. ;)
> What about this?
> 
> #   A  -0->  B  -1->  C  -2->  ...

I would prefer the following:

#   A --> B --> C --> D --> E --> F --> G --> H
#  0 1 2 3 4 5 6

that is, the number below the middle of the arrow
(which could have been even longer)

#   A ---> B ---> C ---> D ---> E ---> F ---> G ---> H
#  0  1  2  3  4  5  6


Let's paint this bikeshed _plaid_ ;-
-- 
Jakub Narębski

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Tue, Oct 4, 2016 at 2:49 PM, Jeff King  wrote:
> On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:
>
>> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
>> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
>> > the spacing somehow so that isn't how they look to you? Or are you just
>> > saying you would prefer them under the "-" of the arrow?
>>
>> I bet they line up in a monospace font and I just happened to be
>> viewing this from GMail which isn't showing it in monospace and so it
>> doesn't line up. Ignore me and carry on
>
> Oh, good. I was wondering if I was going crazy. :)
>
> -Peff

Only one of us is going crazy, but I'm not sure who ;)

-Jake

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:

> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
> > the spacing somehow so that isn't how they look to you? Or are you just
> > saying you would prefer them under the "-" of the arrow?
>
> I bet they line up in a monospace font and I just happened to be
> viewing this from GMail which isn't showing it in monospace and so it
> doesn't line up. Ignore me and carry on

Oh, good. I was wondering if I was going crazy. :)

-Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Tue, Oct 4, 2016 at 1:55 PM, Jeff King  wrote:
> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
>
>> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too 
>> >> >> > deep". We start
>> >> >> > +# the depth at 0 and count links, not repositories, so in a chain 
>> >> >> > like:
>> >> >> > +#
>> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H
>> >> >> > +#  0123456
>> >> >> > +#
>> [...]
>> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
>> > is "1", and so on.
>>
>> If you need to re-roll for some other reason I would add some spaces
>> around the numbers so they line up better with the links so that this
>> becomes more clear.
>
> Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> this clear. I put the numbers under the ">" of the arrow. Did I screw up
> the spacing somehow so that isn't how they look to you? Or are you just
> saying you would prefer them under the "-" of the arrow?
>
> -Peff

I bet they line up in a monospace font and I just happened to be
viewing this from GMail which isn't showing it in monospace and so it
doesn't line up. Ignore me and carry on

Thanks,
Jake

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Tue, Oct 04, 2016 at 01:58:54PM -0700, Stefan Beller wrote:

> On Tue, Oct 4, 2016 at 1:55 PM, Jeff King  wrote:
> > On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
> >
> >> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too 
> >> >> >> > deep". We start
> >> >> >> > +# the depth at 0 and count links, not repositories, so in a chain 
> >> >> >> > like:
> >> >> >> > +#
> >> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H
> >> >> >> > +#  0123456
> >> >> >> > +#
> >> [...]
> >> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
> >> > is "1", and so on.
> >>
> >> If you need to re-roll for some other reason I would add some spaces
> >> around the numbers so they line up better with the links so that this
> >> becomes more clear.
> >
> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
> > the spacing somehow so that isn't how they look to you? Or are you just
> > saying you would prefer them under the "-" of the arrow?
> >
> > -Peff
> 
> Input from a self-claimed design expert for ASCII art. ;)
> What about this?
> 
> #   A  -0->  B  -1->  C  -2->  ...
> 
> (Double space between letter and arrow, number included in the arrow)

I actually find that quite confusing, as it looks like "-1", "-2", etc.

This has got to be my favorite bikeshed discussion of all time, though. :)

-Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Stefan Beller]

On Tue, Oct 4, 2016 at 1:55 PM, Jeff King  wrote:
> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:
>
>> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too 
>> >> >> > deep". We start
>> >> >> > +# the depth at 0 and count links, not repositories, so in a chain 
>> >> >> > like:
>> >> >> > +#
>> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H
>> >> >> > +#  0123456
>> >> >> > +#
>> [...]
>> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
>> > is "1", and so on.
>>
>> If you need to re-roll for some other reason I would add some spaces
>> around the numbers so they line up better with the links so that this
>> becomes more clear.
>
> Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> this clear. I put the numbers under the ">" of the arrow. Did I screw up
> the spacing somehow so that isn't how they look to you? Or are you just
> saying you would prefer them under the "-" of the arrow?
>
> -Peff

Input from a self-claimed design expert for ASCII art. ;)
What about this?

#   A  -0->  B  -1->  C  -2->  ...

(Double space between letter and arrow, number included in the arrow)

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:

> >> >> > +# Note: These tests depend on the hard-coded value of 5 as "too 
> >> >> > deep". We start
> >> >> > +# the depth at 0 and count links, not repositories, so in a chain 
> >> >> > like:
> >> >> > +#
> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H
> >> >> > +#  0123456
> >> >> > +#
> [...]
> > No, we count links, not repositories. So the "A->B" link is "0", "B->C"
> > is "1", and so on.
> 
> If you need to re-roll for some other reason I would add some spaces
> around the numbers so they line up better with the links so that this
> becomes more clear.

Hmm. Now I am puzzled, because I _did_ line up them specifically to make
this clear. I put the numbers under the ">" of the arrow. Did I screw up
the spacing somehow so that isn't how they look to you? Or are you just
saying you would prefer them under the "-" of the arrow?

-Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Tue, Oct 4, 2016 at 1:49 PM, Jeff King  wrote:
> On Tue, Oct 04, 2016 at 01:44:23PM -0700, Jacob Keller wrote:
>
>> On Tue, Oct 4, 2016 at 6:48 AM, Jeff King  wrote:
>> > On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
>> >
>> >> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
>> >> > index 7bc1c3c..b393613 100755
>> >> > --- a/t/t5613-info-alternate.sh
>> >> > +++ b/t/t5613-info-alternate.sh
>> >> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
>> >> > )
>> >> >  '
>> >> >
>> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". 
>> >> > We start
>> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
>> >> > +#
>> >> > +#   A -> B -> C -> D -> E -> F -> G -> H
>> >> > +#  0123456
>> >> > +#
>> >>
>> >> Ok so we count links, but wouldn't we have 5 links when we hit F, and
>> >> not G? Or am I missing something here?
>> >
>> > This is what I was trying to get at with the "start the depth at 0". We
>> > disallow a depth greater than 5, but because we start at 0-counting,
>> > it's really six links. I guess saying "5 as too deep" is really the
>> > misleading part. It should be "5 as the maximum depth".
>> >
>> > -Peff
>>
>> Right, but if A is 0, then:
>>
>> B = 1
>> C = 2
>> D = 3
>> E = 4
>> F = 5
>> G = 6  (UhOh??)
>> H = 7
>>
>> So do you mean that *B* = 0, and C = 1??? That is not clear from this 
>> commment.
>
> No, we count links, not repositories. So the "A->B" link is "0", "B->C"
> is "1", and so on.
>

If you need to re-roll for some other reason I would add some spaces
around the numbers so they line up better with the links so that this
becomes more clear.

>> So either way it still feels like "6" links is what is allowed? Or the
>> first link has to not count? That's really confusing.
>
> Right, 6 links _are_ allowed. Because we count links, and because we
> start the link-counting at "0" and allow through "5". The link labeled
> "6" (which is really the seventh link!) is the one that is forbidden.

Right. Ok this makes more sense now.

>
>> Basically I G is the 7th letter, not the 6th, so even if we're
>> subtractnig 1 it's still 6 which is 1 too deep? That means we not only
>> discard 0 (the first repository) but we discount the 2nd one as well?
>
> It's basically two off-by-ones from what you might think is correct.  I
> agree it's unintuitive, but I'm just documenting what's there. We could
> change it; it's not like anybody cares about the exact value except
> "deep enough", but _since_ nobody cares, I preferred not to modify the
> code.
>

I agree I don't think changing code is necessary, I was just confused
by the comment that tried to make it clear.

Thanks,
Jake

> -Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Tue, Oct 4, 2016 at 6:48 AM, Jeff King  wrote:
> On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
>
>> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
>> > index 7bc1c3c..b393613 100755
>> > --- a/t/t5613-info-alternate.sh
>> > +++ b/t/t5613-info-alternate.sh
>> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
>> > )
>> >  '
>> >
>> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
>> > start
>> > +# the depth at 0 and count links, not repositories, so in a chain like:
>> > +#
>> > +#   A -> B -> C -> D -> E -> F -> G -> H
>> > +#  0123456
>> > +#
>>
>> Ok so we count links, but wouldn't we have 5 links when we hit F, and
>> not G? Or am I missing something here?
>
> This is what I was trying to get at with the "start the depth at 0". We
> disallow a depth greater than 5, but because we start at 0-counting,
> it's really six links. I guess saying "5 as too deep" is really the
> misleading part. It should be "5 as the maximum depth".
>
> -Peff

Right, but if A is 0, then:

B = 1
C = 2
D = 3
E = 4
F = 5
G = 6  (UhOh??)
H = 7

So do you mean that *B* = 0, and C = 1??? That is not clear from this commment.

So either way it still feels like "6" links is what is allowed? Or the
first link has to not count? That's really confusing.

Basically I G is the 7th letter, not the 6th, so even if we're
subtractnig 1 it's still 6 which is 1 too deep? That means we not only
discard 0 (the first repository) but we discount the 2nd one as well?

Thanks,
Jake

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Tue, Oct 04, 2016 at 01:44:23PM -0700, Jacob Keller wrote:

> On Tue, Oct 4, 2016 at 6:48 AM, Jeff King  wrote:
> > On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
> >
> >> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> >> > index 7bc1c3c..b393613 100755
> >> > --- a/t/t5613-info-alternate.sh
> >> > +++ b/t/t5613-info-alternate.sh
> >> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
> >> > )
> >> >  '
> >> >
> >> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". 
> >> > We start
> >> > +# the depth at 0 and count links, not repositories, so in a chain like:
> >> > +#
> >> > +#   A -> B -> C -> D -> E -> F -> G -> H
> >> > +#  0123456
> >> > +#
> >>
> >> Ok so we count links, but wouldn't we have 5 links when we hit F, and
> >> not G? Or am I missing something here?
> >
> > This is what I was trying to get at with the "start the depth at 0". We
> > disallow a depth greater than 5, but because we start at 0-counting,
> > it's really six links. I guess saying "5 as too deep" is really the
> > misleading part. It should be "5 as the maximum depth".
> >
> > -Peff
> 
> Right, but if A is 0, then:
> 
> B = 1
> C = 2
> D = 3
> E = 4
> F = 5
> G = 6  (UhOh??)
> H = 7
> 
> So do you mean that *B* = 0, and C = 1??? That is not clear from this 
> commment.

No, we count links, not repositories. So the "A->B" link is "0", "B->C"
is "1", and so on.

> So either way it still feels like "6" links is what is allowed? Or the
> first link has to not count? That's really confusing.

Right, 6 links _are_ allowed. Because we count links, and because we
start the link-counting at "0" and allow through "5". The link labeled
"6" (which is really the seventh link!) is the one that is forbidden.

> Basically I G is the 7th letter, not the 6th, so even if we're
> subtractnig 1 it's still 6 which is 1 too deep? That means we not only
> discard 0 (the first repository) but we discount the 2nd one as well?

It's basically two off-by-ones from what you might think is correct.  I
agree it's unintuitive, but I'm just documenting what's there. We could
change it; it's not like anybody cares about the exact value except
"deep enough", but _since_ nobody cares, I preferred not to modify the
code.

-Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:

> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> > index 7bc1c3c..b393613 100755
> > --- a/t/t5613-info-alternate.sh
> > +++ b/t/t5613-info-alternate.sh
> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
> > )
> >  '
> >
> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
> > start
> > +# the depth at 0 and count links, not repositories, so in a chain like:
> > +#
> > +#   A -> B -> C -> D -> E -> F -> G -> H
> > +#  0123456
> > +#
> 
> Ok so we count links, but wouldn't we have 5 links when we hit F, and
> not G? Or am I missing something here?

This is what I was trying to get at with the "start the depth at 0". We
disallow a depth greater than 5, but because we start at 0-counting,
it's really six links. I guess saying "5 as too deep" is really the
misleading part. It should be "5 as the maximum depth".

-Peff

Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jacob Keller]

On Mon, Oct 3, 2016 at 1:34 PM, Jeff King  wrote:
> These tests are just trying to show that we allow recursion
> up to a certain depth, but not past it. But the counting is
> a bit non-intuitive, and rather than test at the edge of the
> breakage, we test "OK" cases in the middle of the chain.
> Let's explain what's going on, and explicitly test the
> switch between "OK" and "too deep".
>

Makes sense to actually test the edge case here instead of just in the middle.

> Signed-off-by: Jeff King 
> ---
>  t/t5613-info-alternate.sh | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index 7bc1c3c..b393613 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
> )
>  '
>
> +# Note: These tests depend on the hard-coded value of 5 as "too deep". We 
> start
> +# the depth at 0 and count links, not repositories, so in a chain like:
> +#
> +#   A -> B -> C -> D -> E -> F -> G -> H
> +#  0123456
> +#

Ok so we count links, but wouldn't we have 5 links when we hit F, and
not G? Or am I missing something here?

> +# we are OK at "G", but break at "H".
> +#

Seems like from the wording of this comment that we'd break at G and
not H..? Obviously the test below shows G is ok. Aren't the numbers
here off by 1?

Regards,
Jake

> +# Note also that we must use "--bare -l" to make the link to H. The "-l"
> +# ensures we do not do a connectivity check, and the "--bare" makes sure
> +# we do not try to checkout the result (which needs objects), either of
> +# which would cause the clone to fail.



>  test_expect_success 'creating too deep nesting' '
> git clone -l -s C D &&
> git clone -l -s D E &&
> @@ -47,16 +59,12 @@ test_expect_success 'creating too deep nesting' '
> git clone --bare -l -s G H
>  '
>
> -test_expect_success 'invalidity of deepest repository' '
> -   test_must_fail git -C H fsck
> -'
> -
> -test_expect_success 'validity of third repository' '
> -   git -C C fsck
> +test_expect_success 'validity of fifth-deep repository' '
> +   git -C G fsck
>  '
>
> -test_expect_success 'validity of fourth repository' '
> -   git -C D fsck
> +test_expect_success 'invalidity of sixth-deep repository' '
> +   test_must_fail git -C H fsck
>  '
>
>  test_expect_success 'breaking of loops' '
> --
> 2.10.0.618.g82cc264
>

[PATCH 06/18] t5613: clarify "too deep" recursion tests

[Jeff King]

These tests are just trying to show that we allow recursion
up to a certain depth, but not past it. But the counting is
a bit non-intuitive, and rather than test at the edge of the
breakage, we test "OK" cases in the middle of the chain.
Let's explain what's going on, and explicitly test the
switch between "OK" and "too deep".

Signed-off-by: Jeff King 
---
 t/t5613-info-alternate.sh | 24 
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 7bc1c3c..b393613 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
)
 '
 
+# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
+# the depth at 0 and count links, not repositories, so in a chain like:
+#
+#   A -> B -> C -> D -> E -> F -> G -> H
+#  0123456
+#
+# we are OK at "G", but break at "H".
+#
+# Note also that we must use "--bare -l" to make the link to H. The "-l"
+# ensures we do not do a connectivity check, and the "--bare" makes sure
+# we do not try to checkout the result (which needs objects), either of
+# which would cause the clone to fail.
 test_expect_success 'creating too deep nesting' '
git clone -l -s C D &&
git clone -l -s D E &&
@@ -47,16 +59,12 @@ test_expect_success 'creating too deep nesting' '
git clone --bare -l -s G H
 '
 
-test_expect_success 'invalidity of deepest repository' '
-   test_must_fail git -C H fsck
-'
-
-test_expect_success 'validity of third repository' '
-   git -C C fsck
+test_expect_success 'validity of fifth-deep repository' '
+   git -C G fsck
 '
 
-test_expect_success 'validity of fourth repository' '
-   git -C D fsck
+test_expect_success 'invalidity of sixth-deep repository' '
+   test_must_fail git -C H fsck
 '
 
 test_expect_success 'breaking of loops' '
-- 
2.10.0.618.g82cc264