Re: [BUG REPORT] git 2.9.0 clone --recursive fails on cloning a submodule

From: Stefan Beller <sbeller@google.com>
Date: Jun 20, 2016, 01:09 UTC
Message-ID: <CAGZ79kZ9NF57EyEZ4PgOhJw50ujt=QmHs+w1ZNFeDO4zitksVQ@mail.gmail.com>
In-Reply-To: <20160620001332.GA10101@sigill.intra.peff.net>

On Sun, Jun 19, 2016 at 5:13 PM, Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

> On Sun, Jun 19, 2016 at 01:51:56PM -0700, Junio C Hamano wrote:
>
>> Yup, something like this on top of d22eb04 to be merged before
>> v2.9.1 for the maintenance track would be necessary.
>>
>> -- >8 --
>> Subject: clone: do not let --depth imply --shallow-submodules
>>
>> In v2.9.0, we prematurely flipped the default to force cloning
>> submodules shallowly, when the superproject is getting cloned
>> shallowly.  This is likely to fail when the upstream repositories
>> submodules are cloned from a repository that is not prepared to
>> serve histories that ends at a commit that is not at the tip of a
>> branch, and we know the world is not yet ready.
>>
>> Use a safer default to clone the submodules fully, unless the user
>> tells us that she knows that the upstream repository of the
>> submodules are willing to cooperate with "--shallow-submodules"
>> option.
>
> Yeah, this looks good. To minor comments:

I agree, but I find the second concern a bit more than just minor.

Show 16 quoted lines

>
>> @@ -730,8 +730,7 @@ static int checkout(void)
>>               struct argv_array args = ARGV_ARRAY_INIT;
>>               argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
>>
>> -             if (option_shallow_submodules == 1
>> -                 || (option_shallow_submodules == -1 && option_depth))
>> +             if (option_shallow_submodules == 1)
>>                       argv_array_push(&args, "--depth=1");
>
> I hadn't paid much attention to this topic originally, but was surprised
> that "--depth 10" in the clone implies "--depth 1" in the submodule.
> This is not really related to your patch (in fact, your patch makes the
> logic go away). But maybe something to consider if it's ever resurrected
> (or possibly if somebody runs "--shallow-submodules --depth 5" we should
> pass --depth=1; I dunno).

How often do we see a depth != 1 in practice? I have the impression (and no data to back up my claim) that a binary switch for nonshallow or depth 1 would serve us just as good, which is why I did not want to ad complexity to the submodule depth. (What if you want submodule A with depth 2 and B with 5? In that case get them all shallow and deepen as appropriate, would be my answer)

Show 14 quoted lines

>
>> -test_expect_success 'shallow clone implies shallow submodule' '
>> +test_expect_success 'shallow clone does not imply shallow submodule' '
>>       test_when_finished "rm -rf super_clone" &&
>> -     git clone --recurse-submodules --depth 2 "file://$pwd/." super_clone &&
>> +     git clone --recurse-submodules --depth 2 --shallow-submodules "file://$pwd/." super_clone &&
>>       (
>>               cd super_clone &&
>>               git log --oneline >lines &&
>
> We are not really testing "does not imply" here, but "passing
> --shallow-submodules works". The "does not imply" test would be cloning
> without the option and checking that the resulting submodules are not
> shallow.

In case we want to be sure that it works for 2.9.1, i.e. we treat it as a regression, we need to test the "does not imply" a bit more I would think. I can send that test on top tomorrow if you'd like to.

Thanks, Stefan

>
> -Peff