Re: [PATCH 1/2] t7406: correct depth test in shallow test

[Stefan Beller <sbeller@google.com>]

On Thu, Jul 28, 2016 at 11:47 AM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Jul 28, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> We used to ask for 3 changes and tested for having 1, so the test
>>> seems broken.
>>
>> I am not sure what to think of "seems broken".
>
> When asking for depth 3, I would expect a result of 1,2, or 3 commits.
>
> But when testing the depth argument with a history less than 3, and then
> implying: "I got 1, which is less than 3, so the depth works", seems
> to be a logical shortcut to me.
>
> I would have expected a history of >3, then ask for 3 and confirm we did not
> get 4 or 5 or 6, but 3 only.
>
>>
>> Asking for 3 and having 1 is broken in what way?  Should we be
>> expecting for 3 because we asked for that many?  Should we expect
>> less than three even though we asked for three because the upstream
>> side does not even have that many?  If the current test that asks
>> for 3 and gets only 1 is not failing, why should we expect that
>> asking for 2 would get 2?  In other words, why is it sane that
>> asking for fewer number of commits gives us more?
>
> I think there is a subtle thing going on, that I did not examine properly but
> it is hidden in the modernization from
>
>     test 1 = $(something)
>  to test_line_count = 2
>
> I'll investigate the actual reason.

So when I place a test_pause just before that check for the lines
we have the upstream submodule:
$ git log --oneline
6355002 upstream line4
820877d upstream line3
4301fd3 Commit 2
0c90624 upstream

which then allows fetching
6355002 upstream line4
820877d upstream line3
4301fd3 Commit 2

and "Commit 2" is recorded as the sha1, i.e.
when checking out "Commit 2" and running
(git log --oneline | wc -l) we get 1 as it cuts
off after that.

When adding a test (in the next patch) that adds
more commits to the submodule upstream, we
will fetch with depth 3 but will no longer see the sha1,
so we try a different approach. Current approach:
try fetching again with no depth, and then try again with sha1
given.

So one could say there is a bug as the fetching should
use the depth argument as well.

The depth of 2 I chose originally turns out to be a lucky
accident too, as the depth from "Commit 2" is 2,
so that we would observe the same depth no matter if
a --depth 2 was given and working or not.

I'll redo this test (as 2 tests, one is testing the situation as
we have now, i.e. the desired tip is reachable from within
the depth argument, the second test will test if it is not
reachable.)

>
>>
>> Also most of the lines in this subshell seem to be breaking
>> &&-chain.
>
> Thanks for pointing that out, will fix while at it.
>
>>
>>
>>
>>> Correct the test by using test_line_count that exists in the test suite.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>  t/t7406-submodule-update.sh | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>>> index 88e9750..bd261ac 100755
>>> --- a/t/t7406-submodule-update.sh
>>> +++ b/t/t7406-submodule-update.sh
>>> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '
>>>       (cd super3 &&
>>>        sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
>>>        mv -f .gitmodules.tmp .gitmodules &&
>>> -      git submodule update --init --depth=3
>>> +      git submodule update --init --depth=2
>>>        (cd submodule &&
>>> -       test 1 = $(git log --oneline | wc -l)
>>> +       git log --oneline >lines
>>> +       test_line_count = 2 lines
>>>        )
>>>  )
>>>  '