Re: [PATCH v2] clone: pass --single-branch during --recurse-submodules

From: Jeff King <peff@peff.net>
Date: Jan 30, 2020, 10:23 UTC
Message-ID: <20200130102303.GD840531@coredump.intra.peff.net>
In-Reply-To: <20200128221736.9217-1-emilyshaffer@google.com>

On Tue, Jan 28, 2020 at 02:17:36PM -0800, Emily Shaffer wrote:

> Previously, performing "git clone --recurse-submodules --single-branch"
> resulted in submodules cloning all branches even though the superproject
> cloned only one branch. Pipe --single-branch through the submodule
> helper framework to make it to 'clone' later on.

This makes sense to me, bearing in mind that I'm not at all a good person to point out subtleties with submodules that could bite us.

Show 8 quoted lines

> As discussed in the thread for v1 of this patch, in cases when the
> submodule commit referenced by the specified superproject branch isn't
> the same as the HEAD of the submodule repo known by the server side,
> this still works in kind of a non-obvious way. In these cases, first we
> fetch the single branch that is the ancestor of the server's HEAD; then
> we fetch the commit needed by the superproject (and its ancestry). So
> while this change prevents us from fetching *all* branches on clone, it
> doesn't necessarily limit us to a single branch as described.

Is it worth adding a test that we do the right thing here? Not so much to prove that it works now, but to protect us against future changes. It seems like the sort of thing that could get subtly broken.

The patch looks mostly good to me (my, that was a lot of plumbing through that option); here are a few minor comments:

> -update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]::
> +update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--single-branch] [--] [<path>...]::

This line is horrendously long. Not new in your patch, but I wonder if the time might have come to break it up.

> +--single-branch::
> +	This option is only valid for the update command.
> +	Clone only one branch during update, HEAD or --branch.

For some reason my brain insists on parsing this second sentence as a 3-item list without an Oxford comma. I wonder if it would be more clear as:

  Clone only one branch during update: HEAD or one specified by
  --branch.

or similar.

Show 5 quoted lines

>  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
>  	SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, 0, \
> -	NULL, NULL, NULL, \
> +	NULL, NULL, NULL, 0,\
>  	NULL, 0, 0, 0, NULL, 0, 0, 1}

Wow. Also not new in your patch, but I think we're moving towards the use of C99 named initializers, which would make this a bit less daunting (all of the NULL/0 items could be omitted!).

Show 9 quoted lines

> +test_expect_success 'clone with --single-branch' '
> +	test_when_finished "rm -rf super_clone" &&
> +	git clone --recurse-submodules --single-branch "file://$pwd/." super_clone &&
> +	(
> +		cd super_clone/sub &&
> +		git branch -a >branches &&
> +		test_must_fail grep other branches
> +	)
> +'

Don't use test_must_fail with non-Git commands; you can just say "!  grep".

We usually try to avoid scripting around git-branch output (although I find it pretty unlikely that future changes would break this particular case). git-for-each-ref would be a better pick, but I wonder if:

  git rev-parse --verify origin/master &&
  test_must_fail git rev-parse --verify origin/other

might express the expectation more clearly.

-Peff