[PATCH] clone: add `--remote-submodules` flag

[PATCH] clone: add `--remote-submodules` flag

[Ben Avison]

When using `git clone --recurse-submodules` there was previously no way to
pass a `--remote` switch to the implicit `git submodule update` command for
any use case where you want the submodules to be checked out on their
remote-tracking branch rather than with the SHA-1 recorded in the superproject.

This patch rectifies this situation. It actually passes `--no-fetch` to
`git submodule update` as well on the grounds they the submodule has only just
been cloned, so fetching from the remote again only serves to slow things down.

Signed-off-by: Ben Avison <bavison@riscosopen.org>
---
 Documentation/git-clone.txt        |  9 ++++-
 builtin/clone.c                    |  8 +++++
 t/t5617-clone-submodules-remote.sh | 54 ++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100755 t/t5617-clone-submodules-remote.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index a0f14b51f2..5fc97f14de 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,8 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--jobs <n>] [--] <repository> [<directory>]
+	  [--[no-]remote-submodules] [--jobs <n>] [--] <repository>
+	  [<directory>]
 
 DESCRIPTION
 -----------
@@ -260,6 +261,12 @@ or `--mirror` is given)
 --[no-]shallow-submodules::
 	All submodules which are cloned will be shallow with a depth of 1.
 
+--[no-]remote-submodules::
+	All submodules which are cloned will use the status of the submodule’s
+	remote-tracking branch to update the submodule, rather than the
+	superproject’s recorded SHA-1. Equivalent to passing `--remote` to
+	`git submodule update`.
+
 --separate-git-dir=<git dir>::
 	Instead of placing the cloned repository where it is supposed
 	to be, place the cloned repository at the specified directory,
diff --git a/builtin/clone.c b/builtin/clone.c
index 31a47d190a..9e5752ee41 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -67,6 +67,7 @@ static int max_jobs = -1;
 static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
 static struct list_objects_filter_options filter_options;
 static struct string_list server_options = STRING_LIST_INIT_NODUP;
+static int option_remote_submodules;
 
 static int recurse_submodules_cb(const struct option *opt,
 				 const char *arg, int unset)
@@ -145,6 +146,8 @@ static struct option builtin_clone_options[] = {
 	OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
 			TRANSPORT_FAMILY_IPV6),
 	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
+	OPT_BOOL(0, "remote-submodules", &option_remote_submodules,
+		    N_("any cloned submodules will use their remote-tracking branch")),
 	OPT_END()
 };
 
@@ -792,6 +795,11 @@ static int checkout(int submodule_progress)
 		if (option_verbosity < 0)
 			argv_array_push(&args, "--quiet");
 
+		if (option_remote_submodules == 1) {
+			argv_array_push(&args, "--remote");
+			argv_array_push(&args, "--no-fetch");
+		}
+
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
new file mode 100755
index 0000000000..3ba9a55341
--- /dev/null
+++ b/t/t5617-clone-submodules-remote.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='Test cloning repos with submodules using remote-tracking branches'
+
+. ./test-lib.sh
+
+pwd=$(pwd)
+
+test_expect_success 'setup' '
+	git checkout -b master &&
+	test_commit commit1 &&
+	mkdir sub &&
+	(
+		cd sub &&
+		git init &&
+		test_commit subcommit1 &&
+		git tag sub_when_added_to_super
+	) &&
+	git submodule add "file://$pwd/sub" sub &&
+	git commit -m "add submodule" &&
+	(
+		cd sub &&
+		test_commit subcommit2
+	)
+'
+
+test_expect_success 'clone with --no-remote-submodules' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules --no-remote-submodules "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git diff --exit-code sub_when_added_to_super
+	)
+'
+
+test_expect_success 'clone with --remote-submodules' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules --remote-submodules "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git diff --exit-code remotes/origin/master
+	)
+'
+
+test_expect_success 'check the default is --no-remote-submodules' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules --no-remote-submodules "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git diff --exit-code sub_when_added_to_super
+	)
+'
+
+test_done
-- 
2.21.0.896.g6a6c0f10a7.dirty

Re: [PATCH] clone: add `--remote-submodules` flag

[Ævar Arnfjörð Bjarmason]

On Mon, May 13 2019, Ben Avison wrote:

> @@ -792,6 +795,11 @@ static int checkout(int submodule_progress)
>  		if (option_verbosity < 0)
>  			argv_array_push(&args, "--quiet");
>
> +		if (option_remote_submodules == 1) {

I see you copied this from code above the context, but to check a bool
variable just use "if (var)" not "if (var == 1)".

> +test_expect_success 'check the default is --no-remote-submodules' '
> +	test_when_finished "rm -rf super_clone" &&
> +	git clone --recurse-submodules --no-remote-submodules "file://$pwd/." super_clone &&
> +	(
> +		cd super_clone/sub &&
> +		git diff --exit-code sub_when_added_to_super
> +	)
> +'

This isn't testing the default, it just tests --no-remote-submodules,
i.e. if you change the "static int" assignment to "1" (to make this new
option the default) all these tests will still pass.

Just fixing that means we finally test for this (the entire test suite
would pass before with this on, showing a concerning lack of test
coverage in the submodule tests...), but with just
s/--no-remote-submodules // here we'll test for it.

You might want to have another test to explicitly check what happens
with --no-remote-submodules, but I don't think it's worth the effort, at
that point you're just testing the getopt machinery.

Re: [PATCH] clone: add `--remote-submodules` flag

[Ben Avison]

On 13/05/2019 22:12, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, May 13 2019, Ben Avison wrote:
> 
>> +		if (option_remote_submodules == 1) {
> 
> I see you copied this from code above the context, but to check a bool
> variable just use "if (var)" not "if (var == 1)".

OK. Would you prefer I edit the line above it as well? I simply assumed 
that was the way it was preferred to perform the test for some reason.

>> +test_expect_success 'check the default is --no-remote-submodules' '
>> +	test_when_finished "rm -rf super_clone" &&
>> +	git clone --recurse-submodules --no-remote-submodules "file://$pwd/." super_clone &&
> 
> This isn't testing the default, it just tests --no-remote-submodules,
> i.e. if you change the "static int" assignment to "1" (to make this new
> option the default) all these tests will still pass.

That was a cut-and-paste error - I meant to omit both new options as you 
suggest! Good spot.

Ben

[PATCH v2] clone: add `--remote-submodules` flag

[Ben Avison]

When using `git clone --recurse-submodules` there was previously no way to
pass a `--remote` switch to the implicit `git submodule update` command for
any use case where you want the submodules to be checked out on their
remote-tracking branch rather than with the SHA-1 recorded in the superproject.

This patch rectifies this situation. It actually passes `--no-fetch` to
`git submodule update` as well on the grounds they the submodule has only just
been cloned, so fetching from the remote again only serves to slow things down.

Signed-off-by: Ben Avison <bavison@riscosopen.org>
---
 Documentation/git-clone.txt        |  9 ++++-
 builtin/clone.c                    |  8 +++++
 t/t5617-clone-submodules-remote.sh | 54 ++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100755 t/t5617-clone-submodules-remote.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index a0f14b51f2..5fc97f14de 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,8 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--jobs <n>] [--] <repository> [<directory>]
+	  [--[no-]remote-submodules] [--jobs <n>] [--] <repository>
+	  [<directory>]
 
 DESCRIPTION
 -----------
@@ -260,6 +261,12 @@ or `--mirror` is given)
 --[no-]shallow-submodules::
 	All submodules which are cloned will be shallow with a depth of 1.
 
+--[no-]remote-submodules::
+	All submodules which are cloned will use the status of the submodule’s
+	remote-tracking branch to update the submodule, rather than the
+	superproject’s recorded SHA-1. Equivalent to passing `--remote` to
+	`git submodule update`.
+
 --separate-git-dir=<git dir>::
 	Instead of placing the cloned repository where it is supposed
 	to be, place the cloned repository at the specified directory,
diff --git a/builtin/clone.c b/builtin/clone.c
index 31a47d190a..b1ae087427 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -67,6 +67,7 @@ static int max_jobs = -1;
 static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
 static struct list_objects_filter_options filter_options;
 static struct string_list server_options = STRING_LIST_INIT_NODUP;
+static int option_remote_submodules;
 
 static int recurse_submodules_cb(const struct option *opt,
 				 const char *arg, int unset)
@@ -145,6 +146,8 @@ static struct option builtin_clone_options[] = {
 	OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
 			TRANSPORT_FAMILY_IPV6),
 	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
+	OPT_BOOL(0, "remote-submodules", &option_remote_submodules,
+		    N_("any cloned submodules will use their remote-tracking branch")),
 	OPT_END()
 };
 
@@ -792,6 +795,11 @@ static int checkout(int submodule_progress)
 		if (option_verbosity < 0)
 			argv_array_push(&args, "--quiet");
 
+		if (option_remote_submodules) {
+			argv_array_push(&args, "--remote");
+			argv_array_push(&args, "--no-fetch");
+		}
+
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
new file mode 100755
index 0000000000..37fcce9c40
--- /dev/null
+++ b/t/t5617-clone-submodules-remote.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='Test cloning repos with submodules using remote-tracking branches'
+
+. ./test-lib.sh
+
+pwd=$(pwd)
+
+test_expect_success 'setup' '
+	git checkout -b master &&
+	test_commit commit1 &&
+	mkdir sub &&
+	(
+		cd sub &&
+		git init &&
+		test_commit subcommit1 &&
+		git tag sub_when_added_to_super
+	) &&
+	git submodule add "file://$pwd/sub" sub &&
+	git commit -m "add submodule" &&
+	(
+		cd sub &&
+		test_commit subcommit2
+	)
+'
+
+test_expect_success 'clone with --no-remote-submodules' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules --no-remote-submodules "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git diff --exit-code sub_when_added_to_super
+	)
+'
+
+test_expect_success 'clone with --remote-submodules' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules --remote-submodules "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git diff --exit-code remotes/origin/master
+	)
+'
+
+test_expect_success 'check the default is --no-remote-submodules' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --recurse-submodules "file://$pwd/." super_clone &&
+	(
+		cd super_clone/sub &&
+		git diff --exit-code sub_when_added_to_super
+	)
+'
+
+test_done
-- 
2.21.0.896.g6a6c0f10a7.dirty

Re: [PATCH] clone: add `--remote-submodules` flag

[Duy Nguyen]

On Tue, May 14, 2019 at 2:46 AM Ben Avison <bavison@riscosopen.org> wrote:
>
> When using `git clone --recurse-submodules` there was previously no way to
> pass a `--remote` switch to the implicit `git submodule update` command for
> any use case where you want the submodules to be checked out on their
> remote-tracking branch rather than with the SHA-1 recorded in the superproject.

Are there any other submodule options that could be useful passing
from git-clone as well? What I'm getting at is, if there are multiple
of them, perhaps we need a different/generic approach than introducing
a bunch of --<something>-submodules. But of course if --remote is the
only useful one left, then it's moot.
-- 
Duy

Re: [PATCH] clone: add `--remote-submodules` flag

[Ben Avison]

On 16/05/2019 12:31, Duy Nguyen wrote:
> On Tue, May 14, 2019 at 2:46 AM Ben Avison <bavison@riscosopen.org> wrote:
>>
>> When using `git clone --recurse-submodules` there was previously no way to
>> pass a `--remote` switch to the implicit `git submodule update` command for
>> any use case where you want the submodules to be checked out on their
>> remote-tracking branch rather than with the SHA-1 recorded in the superproject.
> 
> Are there any other submodule options that could be useful passing
> from git-clone as well? What I'm getting at is, if there are multiple
> of them, perhaps we need a different/generic approach than introducing
> a bunch of --<something>-submodules. But of course if --remote is the
> only useful one left, then it's moot.

That's an interesting point. However, for many of the switches, it only 
makes sense to set them one way when you're calling `git submodule 
update` within `git clone --recurse-submodules`.


--quiet: already inherited from `git clone`

--progress: already inherited from `git clone`

--force: wouldn't have any effect since there cannot be any local 
changes to override yet

--remote: this is what my patch is adding support for

--no-fetch: since any submodules will be freshly cloned, there is no 
need to fetch from them again already, so you'd always want this on (as 
far as I'm aware, this only has effect when you also use --remote, so 
I've made it conditional on that)

--checkout/--rebase/--merge: since there cannot be any local changes to 
rebased or merged yet, these wouldn't have any effect, and it's fine to 
leave them as the default (--checkout)

--init: you always want this in this case

--reference/--dissociate: I suppose you might want these in theory? 
However, as far as I understand, it's only really useful for `git 
submodule update` if the superproject only contains a single submodule, 
since each submodule would require a different reference repository.

--recursive: at present this is applied unconditionally. I suppose you 
might only want to recurse to one level, but I'd think that's rare?

--depth: at present you only get to set this to a depth of 1, by passing 
`--shallow-submodules` to `git clone`. I suppose you might occasionally 
want a different depth, but again I'd expect that to be rare.

--no-recommend-shallow: there may be an argument for letting you set 
this one, and you can't at present

--jobs: already inherited from `git clone`


In summary, the most significant omission is --remote IMHO, though there 
may be an argument for adding a small number of others.

Ben

Re: [PATCH] clone: add `--remote-submodules` flag

[Duy Nguyen]

On Fri, May 17, 2019 at 12:50 AM Ben Avison <bavison@riscosopen.org> wrote:
>
> On 16/05/2019 12:31, Duy Nguyen wrote:
> > On Tue, May 14, 2019 at 2:46 AM Ben Avison <bavison@riscosopen.org> wrote:
> >>
> >> When using `git clone --recurse-submodules` there was previously no way to
> >> pass a `--remote` switch to the implicit `git submodule update` command for
> >> any use case where you want the submodules to be checked out on their
> >> remote-tracking branch rather than with the SHA-1 recorded in the superproject.
> >
> > Are there any other submodule options that could be useful passing
> > from git-clone as well? What I'm getting at is, if there are multiple
> > of them, perhaps we need a different/generic approach than introducing
> > a bunch of --<something>-submodules. But of course if --remote is the
> > only useful one left, then it's moot.
>
> That's an interesting point. However, for many of the switches, it only
> makes sense to set them one way when you're calling `git submodule
> update` within `git clone --recurse-submodules`.
>
> ...
>
>
> In summary, the most significant omission is --remote IMHO, though there
> may be an argument for adding a small number of others.

Great! Thanks for checking.
-- 
Duy>>