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

From: Emily Shaffer <emilyshaffer@google.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2] clone: pass --single-branch during --recurse-submodules
Date: Thu, 20 Feb 2020 18:53:06 -0800	[thread overview]
Message-ID: <20200221025306.GK2447@google.com> (raw)
In-Reply-To: <20200130102303.GD840531@coredump.intra.peff.net>

On Thu, Jan 30, 2020 at 05:23:03AM -0500, Jeff King wrote:
> On Tue, Jan 28, 2020 at 02:17:36PM -0800, Emily Shaffer wrote:

Ouch. I forgot about this review for some time. Sorry :)

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

I wonder if it makes sense to ship this to our submodule-using masses
here for a little while and see how it works / whether anybody yells?
This might be too small for that kind of thing.

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

What did you have in mind beyond the test I added already?

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

I dug around in Asciidoc doc and couldn't find a good way to do so. The
trailing :: means this command listing is done as a "definition list",
and I just didn't see any way to multiline an entry for such a thing. :(

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

Took it verbatim, I agree.

> 
> >  #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!).

Hrm. Yeah, I'll add a quick patch before this one to do so. It's pretty
gross :)

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

ooof

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

Sure, done.

Sorry again for the long wait, and thanks for the effort on the review.
New revision coming momentarily.

 - Emily