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

[Jeff King <peff@peff.net>]

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:

> @@ -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).

> -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.

-Peff