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

[Stefan Beller <sbeller@google.com>]

On Wed, Apr 19, 2017 at 4:30 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Sun, Jun 19, 2016 at 10:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> Stefan, I think it might be worth revisiting the default set by d22eb04
>>> to propagate shallowness from the super-project clone. In an ideal
>>> world, we would be asking each submodule for the actual commit we are
>>> interested in, and shallowness would not matter. But until
>>> uploadpack.allowReachableSHA1InWant works everywhere, I suspect this is
>>> going to be a problem.
>>
>> 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.
>>
>> Noticed-by: Vadim Eisenberg <VADIME@il.ibm.com>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  Documentation/git-clone.txt | 5 ++---
>>  builtin/clone.c             | 5 ++---
>>  t/t5614-clone-submodules.sh | 4 ++--
>>  3 files changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>> index e1a21b7..c5a1ce2 100644
>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -192,9 +192,8 @@ objects from the source repository into a pack in the cloned repository.
>>         Create a 'shallow' clone with a history truncated to the
>>         specified number of revisions. Implies `--single-branch` unless
>>         `--no-single-branch` is given to fetch the histories near the
>> -       tips of all branches. This implies `--shallow-submodules`. If
>> -       you want to have a shallow superproject clone, but full submodules,
>> -       also pass `--no-shallow-submodules`.
>> +       tips of all branches. If you want to clone submodules shallowly,
>> +       also pass `--shallow-submodules`.
>>
>>  --[no-]single-branch::
>>         Clone only the history leading to the tip of a single branch,
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index ecdf308..f267742 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -40,7 +40,7 @@ static const char * const builtin_clone_usage[] = {
>>
>>  static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
>>  static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
>> -static int option_shallow_submodules = -1;
>> +static int option_shallow_submodules;
>>  static char *option_template, *option_depth;
>>  static char *option_origin = NULL;
>>  static char *option_branch = NULL;
>> @@ -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");
>
> Very late reply, since I'm just looking at this now with the --no-tags
> opti,n, but that == 1 makes no sense anymore, and should just be `if
> (option_shallow_submodules)` shouldn't it? I.e. this used to be an int
> for the depth, now is a bool, but the current == 1 check is left over
> probably from an earlier version where the depth was configurable.

Yes we can drop the "== 1" here.

Thanks,
Stefan