[PATCH] clone: teach --single-branch and --branch during --recurse

[PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

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(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 22425cbc76..8c516a9670 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -133,7 +133,7 @@ If you really want to remove a submodule from the repository and commit
 that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal
 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] [-b|--branch <name>] [--] [<path>...]::
 +
 --
 Update the registered submodules to match what the superproject
@@ -430,6 +430,10 @@ options carefully.
 	Clone new submodules in parallel with as many jobs.
 	Defaults to the `submodule.fetchJobs` option.
 
+--single-branch::
+	This option is only valid for the update command.
+	Clone only one branch during update, HEAD or --branch.
+
 <path>...::
 	Paths to submodule(s). When specified this will restrict the command
 	to only operate on the submodules found at the specified paths.
diff --git a/builtin/clone.c b/builtin/clone.c
index 6dee265cc9..293cef8b30 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -808,6 +808,12 @@ static int checkout(int submodule_progress)
 			argv_array_push(&args, "--no-fetch");
 		}
 
+		if (option_single_branch)
+			argv_array_push(&args, "--single-branch");
+
+		if (option_branch)
+			argv_array_pushf(&args, "--branch=%s", option_branch);
+
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931ecd7..92bd823d38 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1225,7 +1225,8 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference, int dissociate,
-			   int quiet, int progress)
+			   int quiet, int progress, int single_branch,
+			   const char *branch)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
@@ -1247,6 +1248,10 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 		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 (branch)
+		argv_array_pushl(&cp.args, "--branch", branch, NULL);
 
 	argv_array_push(&cp.args, "--");
 	argv_array_push(&cp.args, url);
@@ -1373,6 +1378,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	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;
+	char *branch = NULL;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -1400,12 +1407,17 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &require_init,
 			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
+		OPT_STRING('b', "branch", &branch, "<branch>",
+			   N_("checkout <branch> instead of the remote's HEAD")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
+		   "[--single-branch] [-b | --branch <name>] "
 		   "--url <url> --path <path>"),
 		NULL
 	};
@@ -1438,7 +1450,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 		prepare_possible_alternates(name, &reference);
 
 		if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
-				    quiet, progress))
+				    quiet, progress, single_branch, branch))
 			die(_("clone of '%s' into submodule path '%s' failed"),
 			    url, path);
 	} else {
@@ -1562,6 +1574,8 @@ struct submodule_update_clone {
 	const char *depth;
 	const char *recursive_prefix;
 	const char *prefix;
+	int single_branch;
+	const char *branch;
 
 	/* to be consumed by git-submodule.sh */
 	struct update_clone_data *update_clone;
@@ -1578,7 +1592,7 @@ 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,\
 	NULL, 0, 0, 0, NULL, 0, 0, 1}
 
 
@@ -1718,6 +1732,10 @@ 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->branch)
+		argv_array_pushl(&child->args, "--branch", suc->branch, NULL);
 
 cleanup:
 	strbuf_reset(&displaypath_sb);
@@ -1897,6 +1915,10 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &suc.require_init,
 			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &suc.single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
+		OPT_STRING('b', "branch", &suc.branch, "<branch>",
+			   N_("checkout <branch> instead of the remote's HEAD")),
 		OPT_END()
 	};
 
diff --git a/git-submodule.sh b/git-submodule.sh
index aaa1809d24..c2eadbb930 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -10,7 +10,7 @@ USAGE="[--quiet] [--cached]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    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] [-b|--branch <name>] [--] [<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>...]
@@ -47,6 +47,8 @@ custom_name=
 depth=
 progress=
 dissociate=
+single_branch=
+sm_branch=
 
 die_if_unmatched ()
 {
@@ -526,6 +528,17 @@ cmd_update()
 		--jobs=*)
 			jobs=$1
 			;;
+		--single-branch)
+			single_branch=1
+			;;
+		-b|--branch)
+			case "$2" in '') usage ;; esac
+			sm_branch="--branch=$2"
+			shift
+			;;
+		-b=*|--branch=*)
+			sm_branch=$1
+			;;
 		--)
 			shift
 			break
@@ -555,6 +568,8 @@ cmd_update()
 		${dissociate:+"--dissociate"} \
 		${depth:+--depth "$depth"} \
 		${require_init:+--require-init} \
+		${single_branch:+--single-branch} \
+		${sm_branch:+"$sm_branch"} \
 		$recommend_shallow \
 		$jobs \
 		-- \
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
index 37fcce9c40..390645897d 100755
--- a/t/t5617-clone-submodules-remote.sh
+++ b/t/t5617-clone-submodules-remote.sh
@@ -14,14 +14,16 @@ test_expect_success 'setup' '
 		cd sub &&
 		git init &&
 		test_commit subcommit1 &&
-		git tag sub_when_added_to_super
+		git tag sub_when_added_to_super &&
+		git branch other
 	) &&
 	git submodule add "file://$pwd/sub" sub &&
 	git commit -m "add submodule" &&
 	(
 		cd sub &&
 		test_commit subcommit2
-	)
+	) &&
+	git branch other
 '
 
 test_expect_success 'clone with --no-remote-submodules' '
@@ -51,4 +53,24 @@ test_expect_success 'check the default is --no-remote-submodules' '
 	)
 '
 
+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
+	)
+'
+
+test_expect_success 'clone with --single-branch and --branch' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules --single-branch --branch other "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git branch -a >branches &&
+		test_must_fail grep master branches
+	)
+'
+
 test_done
-- 
2.25.0.rc1.283.g88dfdc4193-goog

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

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.

> 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(-)

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Jeff King]

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

> Subject: clone: teach --single-branch and --branch during --recurse

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

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

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

On Thu, Jan 09, 2020 at 03:11:50AM -0500, Jeff King wrote:
> On Wed, Jan 08, 2020 at 03:19:00PM -0800, Emily Shaffer wrote:
> 
> > Subject: clone: teach --single-branch and --branch during --recurse
> 
> 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).
> 
> > 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.
> 
> 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.

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.

> 
> -Peff

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Jeff King]

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

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

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.

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

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

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

> (like trying to replace the use of "repo" in Android)

Oops, you saw right through us ;)

> 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

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?

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

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

On Mon, Jan 27, 2020 at 02:20:19PM -0800, Emily Shaffer wrote:
> On Fri, Jan 17, 2020 at 04:03:19PM -0500, Jeff King wrote:
> 
> > (like trying to replace the use of "repo" in Android)
> 
> Oops, you saw right through us ;)
> 
> > 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
> 
> 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?

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.

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

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Jeff King]

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

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

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

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

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

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

Yeah, that makes perfect sense to me.

-Peff

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Jeff King]

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

> > >   - make sure that .gitmodules has enough information about which branch
> > >     to use for each submodule
> > 
> > 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?
> 
> 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?

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

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

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.

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

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

On Mon, Jan 27, 2020 at 06:10:07PM -0500, Jeff King wrote:
> On Mon, Jan 27, 2020 at 02:49:14PM -0800, Emily Shaffer wrote:
> 
> > > >   - make sure that .gitmodules has enough information about which branch
> > > >     to use for each submodule
> > > 
> > > 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?
> > 
> > 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?
> 
> Oof, I should have read this message before responding to the other one. ;)
> 
> > 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.
> 
> 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.

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.

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

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.

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

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

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

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Jeff King]

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

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

> > So for the simple case, you probably do want to be able to say "use this
> > branch for cloning all submodules".
> 
> I think it still makes sense to call this out explicitly, yes? Or do you
> think that should just be the default?

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

> 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

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.

> 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 ;)

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

Re: [PATCH] clone: teach --single-branch and --branch during --recurse

[Emily Shaffer]

On Mon, Jan 27, 2020 at 08:31:39PM -0500, Jeff King wrote:
> On Mon, Jan 27, 2020 at 05:08:41PM -0800, Emily Shaffer wrote:
> 
> > > 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.
> > 
> > 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, 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".

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'?

> 
> > > So for the simple case, you probably do want to be able to say "use this
> > > branch for cloning all submodules".
> > 
> > I think it still makes sense to call this out explicitly, yes? Or do you
> > think that should just be the default?
> 
> Yes, I think it should be a separate option from "--branch".
> 
> > 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
> 
> 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).

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.

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

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

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

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

 - Emily

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

[Emily Shaffer]

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(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 5232407f68..10c42b752a 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -133,7 +133,7 @@ If you really want to remove a submodule from the repository and commit
 that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal
 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 the registered submodules to match what the superproject
@@ -430,6 +430,10 @@ options carefully.
 	Clone new submodules in parallel with as many jobs.
 	Defaults to the `submodule.fetchJobs` option.
 
+--single-branch::
+	This option is only valid for the update command.
+	Clone only one branch during update, HEAD or --branch.
+
 <path>...::
 	Paths to submodule(s). When specified this will restrict the command
 	to only operate on the submodules found at the specified paths.
diff --git a/builtin/clone.c b/builtin/clone.c
index 0fc89ae2b9..ec749a86c1 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -832,6 +832,9 @@ static int checkout(int submodule_progress)
 			argv_array_push(&args, "--no-fetch");
 		}
 
+		if (option_single_branch)
+			argv_array_push(&args, "--single-branch");
+
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931ecd7..50c9e23db1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1225,7 +1225,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference, int dissociate,
-			   int quiet, int progress)
+			   int quiet, int progress, int single_branch)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
@@ -1247,6 +1247,8 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 		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");
 
 	argv_array_push(&cp.args, "--");
 	argv_array_push(&cp.args, url);
@@ -1373,6 +1375,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	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;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -1400,12 +1403,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &require_init,
 			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
+		   "[--single-branch] "
 		   "--url <url> --path <path>"),
 		NULL
 	};
@@ -1438,7 +1444,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 		prepare_possible_alternates(name, &reference);
 
 		if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
-				    quiet, progress))
+				    quiet, progress, single_branch))
 			die(_("clone of '%s' into submodule path '%s' failed"),
 			    url, path);
 	} else {
@@ -1562,6 +1568,7 @@ struct submodule_update_clone {
 	const char *depth;
 	const char *recursive_prefix;
 	const char *prefix;
+	int single_branch;
 
 	/* to be consumed by git-submodule.sh */
 	struct update_clone_data *update_clone;
@@ -1578,7 +1585,7 @@ 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}
 
 
@@ -1718,6 +1725,8 @@ 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");
 
 cleanup:
 	strbuf_reset(&displaypath_sb);
@@ -1897,6 +1906,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &suc.require_init,
 			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &suc.single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
 
diff --git a/git-submodule.sh b/git-submodule.sh
index aaa1809d24..95f73ab775 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -10,7 +10,7 @@ USAGE="[--quiet] [--cached]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    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] set-branch (--default|--branch <branch>) [--] <path>
    or: $dashless [--quiet] set-url [--] <path> <newurl>
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
@@ -47,6 +47,7 @@ custom_name=
 depth=
 progress=
 dissociate=
+single_branch=
 
 die_if_unmatched ()
 {
@@ -526,6 +527,9 @@ cmd_update()
 		--jobs=*)
 			jobs=$1
 			;;
+		--single-branch)
+			single_branch=1
+			;;
 		--)
 			shift
 			break
@@ -555,6 +559,7 @@ cmd_update()
 		${dissociate:+"--dissociate"} \
 		${depth:+--depth "$depth"} \
 		${require_init:+--require-init} \
+		${single_branch:+--single-branch} \
 		$recommend_shallow \
 		$jobs \
 		-- \
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
index 37fcce9c40..67f2b48659 100755
--- a/t/t5617-clone-submodules-remote.sh
+++ b/t/t5617-clone-submodules-remote.sh
@@ -14,7 +14,8 @@ test_expect_success 'setup' '
 		cd sub &&
 		git init &&
 		test_commit subcommit1 &&
-		git tag sub_when_added_to_super
+		git tag sub_when_added_to_super &&
+		git branch other
 	) &&
 	git submodule add "file://$pwd/sub" sub &&
 	git commit -m "add submodule" &&
@@ -51,4 +52,14 @@ test_expect_success 'check the default is --no-remote-submodules' '
 	)
 '
 
+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
+	)
+'
+
 test_done
-- 
2.25.0.341.g760bfbb309-goog

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

[Jeff King]

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.

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

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

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

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

[Emily Shaffer]

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

[PATCH v3 0/2] propagate --single-branch during clone --recurse-submodules

[Emily Shaffer]

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

-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v3 1/2] submodule--helper: use C99 named initializer

[Emily Shaffer]

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(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b6e4c79968..1737a91890 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1586,10 +1586,13 @@ struct submodule_update_clone {
 
 	int max_jobs;
 };
-#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, 0, 0, 0, NULL, 0, 0, 1}
+#define SUBMODULE_UPDATE_CLONE_INIT { \
+	.list = MODULE_LIST_INIT, \
+	.update = SUBMODULE_UPDATE_STRATEGY_INIT, \
+	.recommend_shallow = -1, \
+	.references = STRING_LIST_INIT_DUP, \
+	.max_jobs = 1 \
+}
 
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v3 2/2] clone: pass --single-branch during --recurse-submodules

[Emily Shaffer]

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(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 218942acd1..c9ed2bf3d5 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -133,7 +133,7 @@ If you really want to remove a submodule from the repository and commit
 that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal
 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>] [--[no-]single-branch] [--] [<path>...]::
 +
 --
 Update the registered submodules to match what the superproject
@@ -430,6 +430,10 @@ options carefully.
 	Clone new submodules in parallel with as many jobs.
 	Defaults to the `submodule.fetchJobs` option.
 
+--[no-]single-branch::
+	This option is only valid for the update command.
+	Clone only one branch during update: HEAD or one specified by --branch.
+
 <path>...::
 	Paths to submodule(s). When specified this will restrict the command
 	to only operate on the submodules found at the specified paths.
diff --git a/builtin/clone.c b/builtin/clone.c
index 4f6150c55c..1ad26f4d8c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -833,6 +833,11 @@ static int checkout(int submodule_progress)
 			argv_array_push(&args, "--no-fetch");
 		}
 
+		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);
 	}
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1737a91890..11c38369bd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1235,7 +1235,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference, int dissociate,
-			   int quiet, int progress)
+			   int quiet, int progress, int single_branch)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
@@ -1257,6 +1257,10 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 		argv_array_push(&cp.args, "--dissociate");
 	if (gitdir && *gitdir)
 		argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
+	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);
@@ -1383,6 +1387,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	struct string_list reference = STRING_LIST_INIT_NODUP;
 	int dissociate = 0, require_init = 0;
 	char *sm_alternate = NULL, *error_strategy = NULL;
+	int single_branch = -1;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -1410,12 +1415,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &require_init,
 			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
+		   "[--single-branch] "
 		   "--url <url> --path <path>"),
 		NULL
 	};
@@ -1448,7 +1456,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 		prepare_possible_alternates(name, &reference);
 
 		if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
-				    quiet, progress))
+				    quiet, progress, single_branch))
 			die(_("clone of '%s' into submodule path '%s' failed"),
 			    url, path);
 	} else {
@@ -1572,6 +1580,7 @@ struct submodule_update_clone {
 	const char *depth;
 	const char *recursive_prefix;
 	const char *prefix;
+	int single_branch;
 
 	/* to be consumed by git-submodule.sh */
 	struct update_clone_data *update_clone;
@@ -1591,10 +1600,10 @@ struct submodule_update_clone {
 	.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)
 {
@@ -1731,6 +1740,10 @@ 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 >= 0)
+		argv_array_push(&child->args, suc->single_branch ?
+					      "--single-branch" :
+					      "--no-single-branch");
 
 cleanup:
 	strbuf_reset(&displaypath_sb);
@@ -1910,6 +1923,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &suc.require_init,
 			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &suc.single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
 
diff --git a/git-submodule.sh b/git-submodule.sh
index afcb4c0948..89f915cae9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -10,7 +10,7 @@ USAGE="[--quiet] [--cached]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    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] [--[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>...]
@@ -47,6 +47,7 @@ custom_name=
 depth=
 progress=
 dissociate=
+single_branch=
 
 die_if_unmatched ()
 {
@@ -528,6 +529,12 @@ cmd_update()
 		--jobs=*)
 			jobs=$1
 			;;
+		--single-branch)
+			single_branch="--single-branch"
+			;;
+		--no-single-branch)
+			single_branch="--no-single-branch"
+			;;
 		--)
 			shift
 			break
@@ -557,6 +564,7 @@ cmd_update()
 		${dissociate:+"--dissociate"} \
 		${depth:+--depth "$depth"} \
 		${require_init:+--require-init} \
+		$single_branch \
 		$recommend_shallow \
 		$jobs \
 		-- \
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
index 37fcce9c40..1a041df10b 100755
--- a/t/t5617-clone-submodules-remote.sh
+++ b/t/t5617-clone-submodules-remote.sh
@@ -14,7 +14,8 @@ test_expect_success 'setup' '
 		cd sub &&
 		git init &&
 		test_commit subcommit1 &&
-		git tag sub_when_added_to_super
+		git tag sub_when_added_to_super &&
+		git branch other
 	) &&
 	git submodule add "file://$pwd/sub" sub &&
 	git commit -m "add submodule" &&
@@ -51,4 +52,14 @@ test_expect_success 'check the default is --no-remote-submodules' '
 	)
 '
 
+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 rev-parse --verify origin/master &&
+		test_must_fail git rev-parse --verify origin/other
+	)
+'
+
 test_done
-- 
2.25.0.265.gbab2e86ba0-goog

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

[Jeff King]

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

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

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

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

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.

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

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.

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

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

Re: [PATCH v3 1/2] submodule--helper: use C99 named initializer

[Jeff King]

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

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

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

Re: [PATCH v3 2/2] clone: pass --single-branch during --recurse-submodules

[Jeff King]

On Thu, Feb 20, 2020 at 07:10:27PM -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 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

Re: [PATCH v3 1/2] submodule--helper: use C99 named initializer

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Start using a named initializer list for SUBMODULE_UPDATE_CLONE_INIT, as

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

> -#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, 0, 0, 0, NULL, 0, 0, 1}
> +#define SUBMODULE_UPDATE_CLONE_INIT { \
> +	.list = MODULE_LIST_INIT, \
> +	.update = SUBMODULE_UPDATE_STRATEGY_INIT, \
> +	.recommend_shallow = -1, \
> +	.references = STRING_LIST_INIT_DUP, \
> +	.max_jobs = 1 \
> +}

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 ;-)