patchclone: teach --single-branch and --branch during --recurse

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 and its friend, --branch, through the submodule helper framework to make it to 'clone' later on.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Note that 'branch' was already in use in git-submodules.sh, so
"submodule branch" aka 'sm_branch' was used to disambiguate the two.

 Documentation/git-submodule.txt    |  6 +++++-
 builtin/clone.c                    |  6 ++++++
 builtin/submodule--helper.c        | 28 +++++++++++++++++++++++++---
 git-submodule.sh                   | 17 ++++++++++++++++-
 t/t5617-clone-submodules-remote.sh | 26 ++++++++++++++++++++++++--
 5 files changed, 76 insertions(+), 7 deletions(-)

On Wed, Jan 08, 2020 at 03:19:00PM -0800, Emily Shaffer wrote:

Bah, in my attempt to keep the subject brief I was aiming to write "--recursive" but instead just wrote "--recurse" which is wrong. I can push another version with the fix if desired.

On Wed, Jan 08, 2020 at 03:19:00PM -0800, Emily Shaffer wrote:

A minor nit, but this subject confused me for a moment. I think we'd usually say "teach" for a new feature being implemented, and this is really just about passing along the existing features. Something like:

  clone: pass --single-branch and --branch when recursing submodules

would have been a bit more obvious (to me anyway).

Since I don't really use submodules, I don't have much data or even intuition to go on. But could this be a regression for some situations? E.g., imagine I have a repo "parent" whose branch "foo" has a submodule "child", but "child" only has a branch "bar". What happens now if I "git clone --recurse-submodules --single-branch -b foo parent", and what will happen after this patch?

I think it works before, but doesn't now.

Setting up like this:

  git init child
  (
  	cd child
  	git checkout -b bar
  	echo whatever >file
  	git add file
  	git commit -m 'child commit'
  )
  
  git init parent
  cd parent
  git checkout -b foo
  git submodule add $PWD/../child
  git commit -m 'add submodule'

if I use the current tip of master, I get:

  $ git clone --recurse-submodules --single-branch -b foo parent cur
  Cloning into 'cur'...
  done.
  Submodule 'child' (/home/peff/tmp/parent/../child) registered for path 'child'
  Cloning into '/home/peff/tmp/cur/child'...
  done.
  Submodule path 'child': checked out 'b5cbfcc9fec3b7d67e305468624fed2ba1aa4758'

  $ git -C cur/child log -1 --oneline | cat
  b5cbfcc child commit

with your patch, I get:

  $ git.compile clone --recurse-submodules --single-branch -b foo parent new
  Cloning into 'new'...
  done.
  Submodule 'child' (/home/peff/tmp/parent/../child) registered for path 'child'
  Cloning into '/home/peff/tmp/new/child'...
  warning: Could not find remote branch foo to clone.
  fatal: Remote branch foo not found in upstream origin
  fatal: clone of '/home/peff/tmp/parent/../child' into submodule path '/home/peff/tmp/new/child' failed
  Failed to clone 'child'. Retry scheduled
  Cloning into '/home/peff/tmp/new/child'...
  warning: Could not find remote branch foo to clone.
  fatal: Remote branch foo not found in upstream origin
  fatal: clone of '/home/peff/tmp/parent/../child' into submodule path '/home/peff/tmp/new/child' failed
  Failed to clone 'child' a second time, aborting

  $ git -C new/child log -1 --oneline | cat
  11acb3a add submodule

(there's nothing checked out in the submodule).

I have no idea how common this kind of thing would be, and I expect in most cases your patch would do what people want. But we might need to be better about retrying without those options when the first clone fails.

-Peff

On Thu, Jan 09, 2020 at 03:11:50AM -0500, Jeff King wrote:

Yeah, that's interesting. A retry sounds like a pretty solid approach, although if someone's being cautious and using --single-branch I wonder if that's really something they want (since that's avoiding grabbing extraneous branches).

I suppose at the very least, --single-branch without --branch should become recursive. Whether --branch should become recursive, I'm not totally sure.

The two scenarios I see in conflict are this:
- Superproject branch "foo"
  - submodule A branch "foo"
  - submodule B branch "foo"
and
- Superproject branch "foo"
  - submodule A branch "bar"
  - submodule B branch "baz"

If we propagate --branch, the first scenario Just Works, and the second scenario requires something like:

 git clone --recurse-submodules=no --single-branch --branch foo https://superproject
 git submodule update --init --single-branch --branch bar A/
 git submodule update --init --single-branch --branch baz B/

(I guess if the superproject knows what branch it wants for all the submodules, you could also just do "git submodule update --init --single-branch" and have it go and ask for all of them.)

If we don't propagate --branch, the second scenario Just Works, and the first scenario requires something like:

 git clone --recurse-submodules=no --single-branch --branch foo https://superproject
 git submodule update --init --single-branch --branch foo

(That is, I think as long as 'update' takes --branch, even if it's not passed along by 'clone', it should still work OK when delegating to everyone.)

Let me know if I misunderstood what you were worried about.

I don't use submodules heavily for myself either. I'll try and ask around a little to see what folks want, at least here. The ergonomics seem pretty similar, so I guess it comes down to having explicit documentation.

On Thu, Jan 16, 2020 at 02:38:00PM -0800, Emily Shaffer wrote:

Yeah, I think it may be worth separating out how to think about the two options. It's a lot more plausible to me that --single-branch would want to recurse than --branch.

Right, that's exactly my concern. You're making one case work but breaking the other.

It really seems like there's no particular reason to assume that the superproject branch corresponds to the submodule branch (or even submodules of submodules). I imagine it would in some cases (like trying to replace the use of "repo" in Android), but that's just one model.

It would make more sense to me to either (or both):

  - make sure that .gitmodules has enough information about which branch
    to use for each submodule

  - offer an extra option for the default branch to use for any
    submodules. This is still not general enough to cover all situations
    (e.g., the bar/baz you showed above), but it at least makes it
    relatively easy to cover the simple cases, without breaking any
    existing ones.

-Peff

On Fri, Jan 17, 2020 at 04:03:19PM -0500, Jeff King wrote:

Oops, you saw right through us ;)

Hum. I don't work with them day to day, but aren't we already in that state? Is that not what the 'branch' option for each submodule means?

Yeah, this is sort of the direction my mind went too - "not --branch recursively, but --submodule-branch". But that breaks down when you've got a nontrivial number of submodules, at which point you're gonna have a hard time unless you've got the .gitmodules configured correctly.

Well, as for this patch, let me try it with just --single-branch and see whether that works for the case the user reported. I can head back to the drawing board if not.

 - Emily

On Mon, Jan 27, 2020 at 02:20:19PM -0800, Emily Shaffer wrote:

I've been corrected off-list that the 'branch' in .gitmodules is used during 'git submodule update --remote', but not during 'git submodule init' or 'git clone --recurse-submodules'. Then, for the problem in discussion for this thread, it seems like a better choice is something like 'git clone --recurse-submdoules --use-gitmodules' or whatever we want to call it - e.g., rather than fetching the branch where the server knows HEAD, ask the .gitmodules to figure out which branch?

It seems like that ought to live separately from --single-branch. In the case where you very strictly only want to fetch one branch (not two branches) I suppose you'd want something like 'git clone --recurse-submodules --single-branch --branch=mysuperprojectbranch --use-gitmodules' to make sure that only one branch per repo comes down.

With n submodules of various naming schemas, provenance, etc., I don't think there's a good case for recursing --branch one way or another; it seems like filling out some config is the way to go.

I guess we could also teach it to take some input like --submodule-branch-spec=foo.txt, and/or a multiply provided --submodule-branch foo=foobranch --submodule-branch bar/baz=bazbranch.

  [foo.txt]
  foo=foobranch
  bar/baz=bazbranch

With that approach, then someone gets a little more flexibility than relying on what the .gitmodules has set up.

With only half the rework of my patch done, I'm starting to convince myself it's not actually going to work :) Well, I'll still try and see.

 - Emily

On Mon, Jan 27, 2020 at 02:49:14PM -0800, Emily Shaffer wrote:

Oof, I should have read this message before responding to the other one. ;)

Yeah, I do still think that it makes sense for clone to pass along --single-branch, regardless, and then deal with branch selection problem separately on top.

Yeah, I agree that the most general form is being able to specify the mapping for each individually. At first I wondered why you'd ever _not_ want to just use the branches specified in .gitmodules. But I suppose that gets embedded in the superproject history, which gets awkward as those commits move between branches. E.g., for an android-like project, you don't want to make a commit that says "use branch devel for all submodules" on the devel branch of your superproject. Eventually that will get merged to master, and you'd have to remember to switch it back to "master".

So for the simple case, you probably do want to be able to say "use this branch for cloning all submodules".

For the complex cases, you'd need that full mapping. But I think it may be worth it to punt on that for now. Even if we eventually added such a feature, I think we'd still want the simpler version anyway (because when you _can_ use it, it's going to be much easier). So there's nothing lost by doing the minimal thing now and waiting to see if more complex use cases even show up.

Another thing occurs to me, though: should the binding of this submodule default branch be written to disk (e.g., a config option)? I'm thinking specifically that if you do:

  git clone --submodule-branch=devel -b devel superproject

and then later, you "git fetch" and find that somebody has added a new submodule, you'd presumably want the devel branch of that, too?

-Peff

On Mon, Jan 27, 2020 at 06:10:07PM -0500, Jeff King wrote:

Sure; I've got that ready to send shortly. It seems to grab HEAD of the remote for each submodule, and then checkout the specific commit ID the superproject wants - in my test case, that commit ID was a direct ancestor of 'master', so the single branch only got 'master'. I'm not sure how it would work with a commit ID which doesn't exist in the single branch that was fetched; I'll write a test and have a look.

Yeah, or I suppose I might be doing something weird, like wanting to run integration tests for the whole project on changes in just one submodule, or something.

I think it still makes sense to call this out explicitly, yes? Or do you think that should just be the default?

This made me think - I wonder if it makes sense to take --submodule-branch as a wildcarded spec instead. So in your case, I could say,

  git clone --submodule-branch *=devel -b devel superproject

And then I don't need to do anything differently for 'git fetch' later. This also opens the door for some repos getting special treatment:

  git clone --submodule-branch-file=foo.txt -b dev example

  foo.txt:
  curl=stable-1.2.3
  nlohmann=v2.28
  example-*=dev
  *=master

(specifying specific versions for some source dependencies, dev branches for submodules which are associated with with 'example' superproject and might be getting active development, and a wild guess for everything else)

I think that also tends to match the glob-expansion configs we use for other things. One thing sticking out to me about the idea of providing --submodule-branch is that you need to know what's in the repo before you clone it the first time, which being able to use globbing like this kind of helps with. But then, I suppose if you don't know what you're looking for, you're not also looking for a very precise filter on your clone ;)

 - Emily

On Mon, Jan 27, 2020 at 05:08:41PM -0800, Emily Shaffer wrote:

Yeah, it's definitely worth exploring how that works. I thought we had some kind of fallback for when we didn't manage to fetch the object. But maybe I am confusing it with the fallback for "we tried to fetch this specific object, but the other side doesn't allow that, so we grabbed a branch instead".

Yes, I think it should be a separate option from "--branch".

If we write it all as config, I think things may get simpler. IIRC, there is already submodule.*.foo config in .git/config (that can mirror and override what's in .gitmodules).

So if we had some config option for "clone this branch for the submodule instead of HEAD", then that means you can do:

  git clone -c submodule.foo.clonehead=devel ...

and the result would be used by the submodule code, but also saved for future invocations. Likewise, if there's no "clonehead" config for a particular submodule, if we fall back to submodule.defaultclonehead, then you could do:

  git clone -c submodule.defaultclonehead=devel ...

and it would also be saved as the default for future submodules. And all without having to invent a new submodule-branch-file format.

The name "clonehead" isn't great. I'm not sure if this ought to be submodule.*.branch (since I don't quite know what that's used for). I think you'll have to explore that a bit.

Yeah; the scheme I outlined above only allow specifying the value for one submodule, or the fallback default. It wouldn't allow arbitrary globbing. But I also suspect nobody wants that. If you know what the submodules are, then you can set up config for each. If you don't, then "everything" is the only glob that makes sense.

-Peff

On Mon, Jan 27, 2020 at 08:31:39PM -0500, Jeff King wrote:

Ok, so I gave it a try. Some well-trimmed trace output:

1) git clone --recurse-submodules --single-branch <url> (the branch in
question is remote's HEAD)
 - Normal clone of superproject
 - git submodule--helper update-clone --progress --require-init
   --single-branch --
 - ultimately...
 - git clone --no-checkout --progress --separate-git-dir
   '/.../super_clone/.git/modules/sub'
   --single-branch --
   '/path/to/submodule/source'
   '/path/to/submodule/destination'
 - git checkout -f -q <ID of submodule's HEAD>

2) git clone --recurse-submodules --single-branch --branch other <url>
  'other' points to a commit of 'sub' which is not an ancestor of 'sub''s
  current HEAD.
 - Normal clone of superproject identical to 1)
 - git submodule--helper update-clone --progress --require-init
   --single-branch --
 - ultimately...
 - git clone --no-checkout --progress --separate-git-dir
   '/.../super_clone/.git/modules/sub'
   --single-branch --
   '/path/to/submodule/source'
   '/path/to/submodule/destination'
 - git fetch origin <ID of submodule's other branch>
 - git checkout -f -q <ID of submodule's other branch>

So, somewhere in the submodule machinery, it looks like we check if we have the commit in question, and if not, we do another fetch. So in this case, we reach to the server twice per submodule.

On the bright side, it doesn't fall over; on the dim side, I'd think we could ask for this ref up front along with whatever branch HEAD is. I thought there was a way we could tell the server we want 'master' as well as '58c34ed'?

Hm. But at clone time, there is no .git/config yet, which is why I proposed a file passed in at the command line. Although it does seem to make sense to write down those preferences in the .git/config after.

I guess you could pass in configs at the command line, though, and then you don't have to massage it to write your config after fetch.

Au contraire - it might be my new go-to insult. ;)

Yeah, I suspect you're right and this fancy globbing falls under YAGNI.

 - Emily

On Mon, Jan 27, 2020 at 02:20:19PM -0800, Emily Shaffer wrote:

Probably? :) I've never used the feature myself, but it does seem like the right thing (and should definitely take precedence over any "-b" option passed to the superproject).

Right. Probably the right answer for that bar/baz case is to complain to the superproject owner that they didn't put branch fields into their .gitmodules. So...

Yeah, that makes perfect sense to me.

-Peff

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.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Removed references to --branch since v1.

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.

To limit submodules to a single branch on clone, we need to know which branch we want for each submodule; unfortunately it seems that just propagating --branch doesn't necessarily help as there's not much guarantee that the superproject branch name matches the submodule branch name, or that all submodule branch names are the same. So, we need to give a little more thought there.

 - Emily

 Documentation/git-submodule.txt    |  6 +++++-
 builtin/clone.c                    |  3 +++
 builtin/submodule--helper.c        | 17 ++++++++++++++---
 git-submodule.sh                   |  7 ++++++-
 t/t5617-clone-submodules-remote.sh | 13 ++++++++++++-
 5 files changed, 40 insertions(+), 6 deletions(-)

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

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.

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:

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

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.

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

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

On Thu, Jan 30, 2020 at 05:23:03AM -0500, Jeff King wrote:

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

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.

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

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

Took it verbatim, I agree.

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

ooof

Sure, done.

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

 - Emily

On Thu, Feb 20, 2020 at 06:53:06PM -0800, Emily Shaffer wrote:

That's OK. I forgot about, too. :)

Certainly putting it in front of more users gives us more confidence. But I have a feeling the gotcha here would be that it does what _some_ set of users wants, but not so much for others. And your audience may not be a representative sample. So certainly it doesn't hurt, but I'll leave it you whether you think it's worthwhile.

I'm having trouble even coming up with a devil's advocate example for the "others" in this case, though. Getting general exposure in "next" and then "master" leading up to the release might also help, but I get the impression that those audiences aren't necessarily comprehensive, either.

I think your test just covers the "happy path" that what we want to fetch is on the tip of that single branch. But do we test a case where the commit we need for a submodule isn't on the HEAD branch of that submodule, and we "clone --single-branch --recurse-submodules" the superproject and it works? I.e., from what you describe above it should work, but it might be nice to make sure it continues to do so.

Hmm, yeah. Personally I think it would be better as:

  update [options] [--] [<path>...]::
     ...
     The supported options are:

       --init::
         ...here's what init does...

but I guess some of them are repeated between commands, which would need addressing. Anyway, it's clear this is way beyond the scope of your patch, so let's forget about it for now.

-Peff

Since v2, I replied to Peff's comments, and I also added support to propagate --no-single-branch. I'm pretty sure I followed the idiom right to to treat options as "yes, no, unset" but not sure of the best way to test to feel assured that they're being propagated correctly.

 - Emily

Emily Shaffer (2):
  submodule--helper: use C99 named initializer
  clone: pass --single-branch during --recurse-submodules

 Documentation/git-submodule.txt    |  6 +++++-
 builtin/clone.c                    |  5 +++++
 builtin/submodule--helper.c        | 32 +++++++++++++++++++++++-------
 git-submodule.sh                   | 10 +++++++++-
 t/t5617-clone-submodules-remote.sh | 13 +++++++++++-
 5 files changed, 56 insertions(+), 10 deletions(-)

-:  ---------- > 1:  d1d69cc9ce submodule--helper: use C99 named initializer
1:  e064e805da ! 2:  adfe55b18f clone: pass --single-branch during --recurse-submodules
    @@ Documentation/git-submodule.txt: If you really want to remove a submodule from t
      options.
      
     -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>...]::
    ++update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--[no-]single-branch] [--] [<path>...]::
      +
      --
      Update the registered submodules to match what the superproject
    @@ Documentation/git-submodule.txt: options carefully.
      	Clone new submodules in parallel with as many jobs.
      	Defaults to the `submodule.fetchJobs` option.
      
    -+--single-branch::
    ++--[no-]single-branch::
     +	This option is only valid for the update command.
    -+	Clone only one branch during update, HEAD or --branch.
    ++	Clone only one branch during update: HEAD or one specified by --branch.
     +
      <path>...::
      	Paths to submodule(s). When specified this will restrict the command
    @@ builtin/clone.c: static int checkout(int submodule_progress)
      			argv_array_push(&args, "--no-fetch");
      		}
      
    -+		if (option_single_branch)
    -+			argv_array_push(&args, "--single-branch");
    ++		if (option_single_branch >= 0)
    ++			argv_array_push(&args, option_single_branch ?
    ++					       "--single-branch" :
    ++					       "--no-single-branch");
     +
      		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
      		argv_array_clear(&args);
    @@ builtin/submodule--helper.c: static int clone_submodule(const char *path, const
      		argv_array_push(&cp.args, "--dissociate");
      	if (gitdir && *gitdir)
      		argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
    -+	if (single_branch)
    -+		argv_array_push(&cp.args, "--single-branch");
    ++	if (single_branch >= 0)
    ++		argv_array_push(&cp.args, single_branch ?
    ++					  "--single-branch" :
    ++					  "--no-single-branch");
      
      	argv_array_push(&cp.args, "--");
      	argv_array_push(&cp.args, url);
    @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv
      	struct string_list reference = STRING_LIST_INIT_NODUP;
      	int dissociate = 0, require_init = 0;
      	char *sm_alternate = NULL, *error_strategy = NULL;
    -+	int single_branch = 0;
    ++	int single_branch = -1;
      
      	struct option module_clone_options[] = {
      		OPT_STRING(0, "prefix", &prefix,
    @@ builtin/submodule--helper.c: struct submodule_update_clone {
      	/* to be consumed by git-submodule.sh */
      	struct update_clone_data *update_clone;
     @@ builtin/submodule--helper.c: struct submodule_update_clone {
    - };
    - #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}
    - 
    - 
    + 	.update = SUBMODULE_UPDATE_STRATEGY_INIT, \
    + 	.recommend_shallow = -1, \
    + 	.references = STRING_LIST_INIT_DUP, \
    ++	.single_branch = -1, \
    + 	.max_jobs = 1 \
    + }
    + 
    +-
    + static void next_submodule_warn_missing(struct submodule_update_clone *suc,
    + 		struct strbuf *out, const char *displaypath)
    + {
     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
      		argv_array_push(&child->args, "--dissociate");
      	if (suc->depth)
      		argv_array_push(&child->args, suc->depth);
    -+	if (suc->single_branch)
    -+		argv_array_push(&child->args, "--single-branch");
    ++	if (suc->single_branch >= 0)
    ++		argv_array_push(&child->args, suc->single_branch ?
    ++					      "--single-branch" :
    ++					      "--no-single-branch");
      
      cleanup:
      	strbuf_reset(&displaypath_sb);
    @@ git-submodule.sh: USAGE="[--quiet] [--cached]
         or: $dashless [--quiet] init [--] [<path>...]
         or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)
     -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]
    -+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--single-branch] [--] [<path>...]
    ++   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--[no-]single-branch] [--] [<path>...]
         or: $dashless [--quiet] set-branch (--default|--branch <branch>) [--] <path>
         or: $dashless [--quiet] set-url [--] <path> <newurl>
         or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    @@ git-submodule.sh: cmd_update()
      			jobs=$1
      			;;
     +		--single-branch)
    -+			single_branch=1
    ++			single_branch="--single-branch"
    ++			;;
    ++		--no-single-branch)
    ++			single_branch="--no-single-branch"
     +			;;
      		--)
      			shift
    @@ git-submodule.sh: cmd_update()
      		${dissociate:+"--dissociate"} \
      		${depth:+--depth "$depth"} \
      		${require_init:+--require-init} \
    -+		${single_branch:+--single-branch} \
    ++		$single_branch \
      		$recommend_shallow \
      		$jobs \
      		-- \
    @@ t/t5617-clone-submodules-remote.sh: test_expect_success 'check the default is --
     +	git clone --recurse-submodules --single-branch "file://$pwd/." super_clone &&
     +	(
     +		cd super_clone/sub &&
    -+		git branch -a >branches &&
    -+		test_must_fail grep other branches
    ++		git rev-parse --verify origin/master &&
    ++		test_must_fail git rev-parse --verify origin/other
     +	)
     +'
     +

Start using a named initializer list for SUBMODULE_UPDATE_CLONE_INIT, as the struct is becoming cumbersome for a typical struct initializer list.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 builtin/submodule--helper.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

On Thu, Feb 20, 2020 at 07:10:26PM -0800, Emily Shaffer wrote:

Much nicer. :) I manually compared the struct declaration to the existing initializer to double-check that you counted the offsets correctly, and all looks good.

-Peff

Emily Shaffer <emilyshaffer@google.com> writes:

The feature has its own name. Instead of coming up with your own name for it, let's say "designated initializers".

This does make it read more easily. If you ended the last field with trailing comma, that would have been even better as a future proofing ;-)

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.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-submodule.txt    |  6 +++++-
 builtin/clone.c                    |  5 +++++
 builtin/submodule--helper.c        | 21 ++++++++++++++++++---
 git-submodule.sh                   | 10 +++++++++-
 t/t5617-clone-submodules-remote.sh | 13 ++++++++++++-
 5 files changed, 49 insertions(+), 6 deletions(-)

On Thu, Feb 20, 2020 at 07:10:27PM -0800, Emily Shaffer wrote:

This looks good to me, with the exception of the open question I had earlier about testing the case where the needed commit isn't covered by the single branch.

-Peff