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
