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

From: Jeff King <peff@peff.net>
Date: Jun 20, 2016, 00:13 UTC
Message-ID: <20160620001332.GA10101@sigill.intra.peff.net>
In-Reply-To: <xmqq7fdkx5oz.fsf@gitster.mtv.corp.google.com>

On Sun, Jun 19, 2016 at 01:51:56PM -0700, Junio C Hamano wrote:

Show 17 quoted lines

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

Show 8 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).

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

-Peff