[PATCH] submodule.sh update --remote: default to oid instead of master

[PATCH] submodule.sh update --remote: default to oid instead of master

[Stefan Beller]

gitmodules(5) sayeth:

   submodule.<name>.branch
       A remote branch name for tracking updates in the upstream
       submodule. If the option is not specified, it defaults to master.

This doesn't allow having a "pinned" submodule that should not be updated
from upstream. We should change this to have no default --- if branch is
not specified, don't update that submodule, just like in Gerrit's
corresponding feature[1].

[1] https://gerrit-review.googlesource.com/Documentation/user-submodules.html#_defining_the_submodule_branch

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/gitmodules.txt | 11 ++++++-----
 git-submodule.sh             | 19 +++++++++++--------
 t/t7406-submodule-update.sh  | 22 ++++++++++++++++++++++
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index 4d63def2069..3b8739f8294 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -50,11 +50,12 @@ submodule.<name>.update::
 
 submodule.<name>.branch::
 	A remote branch name for tracking updates in the upstream submodule.
-	If the option is not specified, it defaults to 'master'.  A special
-	value of `.` is used to indicate that the name of the branch in the
-	submodule should be the same name as the current branch in the
-	current repository.  See the `--remote` documentation in
-	linkgit:git-submodule[1] for details.
+	A special value of `.` is used to indicate that the name of the
+	branch in the submodule should be the same name as the current
+	branch in the current repository.  See the `--remote` documentation
+	in linkgit:git-submodule[1] for details.
+	If the option is not specified, do not update to any branch but
+	the object id of the remote.
 
 submodule.<name>.fetchRecurseSubmodules::
 	This option can be used to control recursive fetching of this
diff --git a/git-submodule.sh b/git-submodule.sh
index f7fd80345cd..342050ae934 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -568,16 +568,19 @@ cmd_update()
 		if test -n "$remote"
 		then
 			branch=$(git submodule--helper remote-branch "$sm_path")
-			if test -z "$nofetch"
+			if test -n "$branch"
 			then
-				# Fetch remote before determining tracking $sha1
-				fetch_in_submodule "$sm_path" $depth ||
-				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
+				if test -z "$nofetch"
+				then
+					# Fetch remote before determining tracking $sha1
+					fetch_in_submodule "$sm_path" $depth ||
+					die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
+				fi
+				remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
+				sha1=$(sanitize_submodule_env; cd "$sm_path" &&
+					git rev-parse --verify "${remote_name}/${branch}") ||
+				die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
 			fi
-			remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
-			sha1=$(sanitize_submodule_env; cd "$sm_path" &&
-				git rev-parse --verify "${remote_name}/${branch}") ||
-			die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
 		fi
 
 		if ! $(git config -f "$(git rev-parse --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 10dc91620a6..f04884743fd 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -260,6 +260,28 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
 	)
 '
 
+test_expect_success 'submodule update --remote should not fetch upstream when no branch is set' '
+	(
+		cd super &&
+		test_might_fail git config --unset -f .gitmodules submodule."submodule".branch &&
+		git add .gitmodules &&
+		git commit --allow-empty -m "submodules: pin in superproject branch"
+	) &&
+	(
+		cd submodule &&
+		echo line4b >>file &&
+		git add file &&
+		test_tick &&
+		git commit -m "upstream line4b"
+	) &&
+	(
+		cd super &&
+		git submodule update --remote --force submodule &&
+		git -C submodule log -1 --oneline >actual &&
+		! grep line4b actual
+	)
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
 	(cd submodule &&
 	 git checkout test-branch &&
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH] submodule.sh update --remote: default to oid instead of master

[Jonathan Nieder]

Stefan Beller wrote:

> Subject: submodule.sh update --remote: default to oid instead of master

Yay!

Nit: it wasn't clear to me at first what default this subject line was
referring to.  Perhaps:

	submodule update --remote: skip GITLINK update when no branch is set

[...]
> --- a/Documentation/gitmodules.txt
> +++ b/Documentation/gitmodules.txt
> @@ -50,11 +50,12 @@ submodule.<name>.update::
>  
>  submodule.<name>.branch::
[...]
> +	If the option is not specified, do not update to any branch but
> +	the object id of the remote.

Likewise: how about something like

	If not set, the default is for `git submodule update --remote`
	to update the submodule to the superproject's recorded SHA-1.

[...]
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -568,16 +568,19 @@ cmd_update()
>  		if test -n "$remote"
>  		then
>  			branch=$(git submodule--helper remote-branch "$sm_path")
> -			if test -z "$nofetch"
> +			if test -n "$branch"
>  			then
> -				# Fetch remote before determining tracking $sha1
> -				fetch_in_submodule "$sm_path" $depth ||
> -				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
> +				if test -z "$nofetch"
> +				then
> +					# Fetch remote before determining tracking $sha1
> +					fetch_in_submodule "$sm_path" $depth ||

Makes sense.  If $sha1 isn't available in the submodule, it will fetch
again later.

[...]
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -260,6 +260,28 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
>  	)
>  '
>  
> +test_expect_success 'submodule update --remote should not fetch upstream when no branch is set' '
> +	(
> +		cd super &&
> +		test_might_fail git config --unset -f .gitmodules submodule."submodule".branch &&

Not about this patch: the quoting here is strange.

> +		git add .gitmodules &&
> +		git commit --allow-empty -m "submodules: pin in superproject branch"
> +	) &&

I wonder if we can do simpler by using -C + some helpers: something like

	git config --unset -f super/.gitmodules ... &&
	test_commit -C submodule ... &&
	git -C super submodule update ... &&
	test_cmp_rev ...

Unfortunately test_cmp_rev doesn't accept a -C argument.

Broader comment: do you think people will be surprised by this new
behavior?  Is there anything special we'd need to do to call it out
(e.g., print a warning or put something in release notes)?

Thanks,
Jonathan

Re: [PATCH] submodule.sh update --remote: default to oid instead of master

[Stefan Beller]

On Wed, Sep 5, 2018 at 4:10 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Stefan Beller wrote:
>
> > Subject: submodule.sh update --remote: default to oid instead of master
>
> Yay!
>
> Nit: it wasn't clear to me at first what default this subject line was
> referring to.  Perhaps:
>
>         submodule update --remote: skip GITLINK update when no branch is set
>
> [...]
> > --- a/Documentation/gitmodules.txt
> > +++ b/Documentation/gitmodules.txt
> > @@ -50,11 +50,12 @@ submodule.<name>.update::
> >
> >  submodule.<name>.branch::
> [...]
> > +     If the option is not specified, do not update to any branch but
> > +     the object id of the remote.
>
> Likewise: how about something like
>
>         If not set, the default is for `git submodule update --remote`
>         to update the submodule to the superproject's recorded SHA-1.

... recorded object id.

sounds good.

> > +             git add .gitmodules &&
> > +             git commit --allow-empty -m "submodules: pin in superproject branch"
> > +     ) &&
>
> I wonder if we can do simpler by using -C + some helpers: something like
>
>         git config --unset -f super/.gitmodules ... &&
>         test_commit -C submodule ... &&
>         git -C super submodule update ... &&
>         test_cmp_rev ...
>
> Unfortunately test_cmp_rev doesn't accept a -C argument.

and the lack of fortune goes further, as test_cmp_rev needs to have
2 revisions in the same repository, i.e. both need to exist,
which is not the case.

> Broader comment: do you think people will be surprised by this new
> behavior?  Is there anything special we'd need to do to call it out
> (e.g., print a warning or put something in release notes)?

I guess. Not sure how to approach this best. Maybe we can
extend the output of 'submodule update' to print that branch names
instead of hashes for the configured case and keep printing hashes
only for this case. Although that would not help someone who relies
on the default solely.

Re: [PATCH] submodule.sh update --remote: default to oid instead of master

[Jonathan Nieder]

Stefan Beller wrote:
> On Wed, Sep 5, 2018 at 4:10 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Broader comment: do you think people will be surprised by this new
>> behavior?  Is there anything special we'd need to do to call it out
>> (e.g., print a warning or put something in release notes)?
>
> I guess. Not sure how to approach this best. Maybe we can
> extend the output of 'submodule update' to print that branch names
> instead of hashes for the configured case and keep printing hashes
> only for this case. Although that would not help someone who relies
> on the default solely.

Thinking more out loud: often the simplest migration path involves
multiple steps:

 1. Warn in the case that is going to change, with no behavior change
    yet.

 2. Treat the case that will change as an error.  This should
    help flush out cases where people were relying on the old behavior.

 3. Introduce the new behavior.  Warn that old versions of Git don't
    support it yet.

 4. Eliminate the warning.  You're all clear now.

Sometimes some of these steps can be combined.

Another possible approach is to measure.  For example, is there some
way to find out how many people are relying in this "git submodule
update --remote" defaulting behavior?  One example of this approach is
to make the change (all in one step) in "next" and deploy to some
relevant subpopulation and see if anyone screams.  By making the
change in "next" instead of something with more stability guarantees,
you get the ability to roll back quickly.

There are other tools at our disposal --- e.g. command-line flags,
config, other kinds of research.

Here my first instinct would be to say this should be a command-line
flag.  To start out, we can keep the historical behavior as a default,
but introduce a command-line option for the new behavior.  This way,
people can pass the negation of that command-line option if they want
the older behavior, throughout the transition.

For example (please ignore names):

 Step 0: introduce

	git submodule update --remote --default-to-master; # current behavior
	git submodule update --remote --no-default-to-master; # new behavior

 and treat plain "git submodule update --remote" as --default-to-master.

 Step 1: when neither --default-to-master nor --no-default-to-master
 has been passed, warn when encountering a submodule with no branch
 and treat it as "master".

 Step 2: when neither --default-to-master nor --no-default-to-master
 has been passed, error out when encountering a submodule with no
 branch.

 Step 3: when neither --default-to-master nor --no-default-to-master
 has been passed, warn when encountering a submodule with no branch
 and treat it as pinned.

 Step 4: eliminate the warning.

What do you think?

Thanks,
Jonathan

[PATCH] submodule update --remote: introduce pinning

[Stefan Beller]

gitmodules(5) sayeth:

   submodule.<name>.branch
       A remote branch name for tracking updates in the upstream
       submodule. If the option is not specified, it defaults to master.

This doesn't allow having a "pinned" submodule that should not be updated
from upstream. We should change this to have no default --- if branch is
not specified, don't update that submodule, just like in Gerrit's
corresponding feature[1].

[1] https://gerrit-review.googlesource.com/Documentation/user-submodules.html#_defining_the_submodule_branch


However changing defaults is difficult as it may surprise the user,
Jonathan came up with a 4 step plan:
Step 0: introduce

	# current behavior:
        git submodule update --remote --remote-default-to-master

	# new behavior:
        git submodule update --remote --remote-pinned

and treat plain "git submodule update --remote" as --default-to-master.

Step 1: when neither --default-to-master nor --no-default-to-master
has been passed, warn when encountering a submodule with no branch
and treat it as "master".

Step 2: when neither --default-to-master nor --no-default-to-master
has been passed, error out when encountering a submodule with no
branch.

Step 3: when neither --default-to-master nor --no-default-to-master
has been passed, warn when encountering a submodule with no branch
and treat it as pinned.

Step 4: eliminate the warning.

This implements step 0 and 1.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Sorry that this took so long, this is a rough patch for steps 0 & 1,
I'd still need to update docs.

Stefan


 Documentation/gitmodules.txt | 11 ++++++-----
 builtin/submodule--helper.c  | 36 +++++++++++++++++++++++++++++-------
 git-submodule.sh             | 34 +++++++++++++++++++++++++---------
 t/t7406-submodule-update.sh  | 29 +++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 21 deletions(-)

diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index 4d63def206..fe42dbdb3e 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -50,11 +50,12 @@ submodule.<name>.update::
 
 submodule.<name>.branch::
 	A remote branch name for tracking updates in the upstream submodule.
-	If the option is not specified, it defaults to 'master'.  A special
-	value of `.` is used to indicate that the name of the branch in the
-	submodule should be the same name as the current branch in the
-	current repository.  See the `--remote` documentation in
-	linkgit:git-submodule[1] for details.
+	A special value of `.` is used to indicate that the name of the
+	branch in the submodule should be the same name as the current
+	branch in the current repository.  See the `--remote` documentation
+	in linkgit:git-submodule[1] for details.
+	If not set, the default for `git submodule update --remote` is
+	to update the submodule to the superproject's recorded object id.
 
 submodule.<name>.fetchRecurseSubmodules::
 	This option can be used to control recursive fetching of this
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 40844870cf..6413f2b410 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1889,7 +1889,7 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix
 	return 0;
 }
 
-static const char *remote_submodule_branch(const char *path)
+static const char *remote_submodule_branch(const char *path, int default_pinned)
 {
 	const struct submodule *sub;
 	const char *branch = NULL;
@@ -1904,8 +1904,12 @@ static const char *remote_submodule_branch(const char *path)
 		branch = sub->branch;
 	free(key);
 
-	if (!branch)
-		return "master";
+	if (!branch) {
+		if (default_pinned)
+			return "";
+		else
+			return "master";
+	}
 
 	if (!strcmp(branch, ".")) {
 		const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
@@ -1932,12 +1936,30 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 {
 	const char *ret;
 	struct strbuf sb = STRBUF_INIT;
-	if (argc != 2)
-		die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
+	int default_pinned = 0;
+
+	struct option remote_options[] = {
+		OPT_SET_INT(0, "default-master", &default_pinned,
+				N_("unconfigured submodules default to master branch"), 0),
+		OPT_SET_INT(0, "default-pinned", &default_pinned,
+				N_("unconfigured submodules default to superproject object id"), 1),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper remote-branch [--default-{master, pinned}]  -- <path>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, remote_options,
+			     git_submodule_helper_usage, 0);
+
+	if (argc != 1)
+		die("submodule--helper remote-branch takes exactly one path, got %d", argc);
 
-	ret = remote_submodule_branch(argv[1]);
+	ret = remote_submodule_branch(argv[0], default_pinned);
 	if (!ret)
-		die("submodule %s doesn't exist", argv[1]);
+		die("submodule %s doesn't exist", argv[0]);
 
 	printf("%s", ret);
 	strbuf_release(&sb);
diff --git a/git-submodule.sh b/git-submodule.sh
index 1b568e29b9..829b90ea97 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -431,6 +431,7 @@ fetch_in_submodule () (
 #
 cmd_update()
 {
+	remote_default_option=
 	# parse $args after "submodule ... update".
 	while test $# -ne 0
 	do
@@ -450,6 +451,12 @@ cmd_update()
 		--remote)
 			remote=1
 			;;
+		--remote-default-to-master)
+			remote_default_option=--default-master
+			;;
+		--remote-pinned)
+			remote_default_option=--default-pinned
+			;;
 		-N|--no-fetch)
 			nofetch=1
 			;;
@@ -555,17 +562,26 @@ cmd_update()
 
 		if test -n "$remote"
 		then
-			branch=$(git submodule--helper remote-branch "$sm_path")
-			if test -z "$nofetch"
+			if test -z $remote_default_option
 			then
-				# Fetch remote before determining tracking $sha1
-				fetch_in_submodule "$sm_path" $depth ||
-				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
+				say "--remote needs clarification: --remote-pinned or --remote-default-to-master?"
+				say "assuming --remote-default-to-master for now"
+				remote_default_option=--default-master
+			fi
+			branch=$(git submodule--helper remote-branch ${remote_default_option} -- "$sm_path")
+			if test -n "$branch"
+			then
+				if test -z "$nofetch"
+				then
+					# Fetch remote before determining tracking $sha1
+					fetch_in_submodule "$sm_path" $depth ||
+					die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
+				fi
+				remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
+				sha1=$(sanitize_submodule_env; cd "$sm_path" &&
+					git rev-parse --verify "${remote_name}/${branch}") ||
+				die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
 			fi
-			remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
-			sha1=$(sanitize_submodule_env; cd "$sm_path" &&
-				git rev-parse --verify "${remote_name}/${branch}") ||
-			die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
 		fi
 
 		if test "$subsha1" != "$sha1" || test -n "$force"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 10dc91620a..3019978211 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -260,6 +260,35 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
 	)
 '
 
+test_expect_success 'submodule update --remote should not fetch upstream when no branch is set' '
+	(
+		cd super &&
+		test_might_fail git config --unset -f .gitmodules submodule.submodule.branch &&
+		git add .gitmodules &&
+		git commit --allow-empty -m "submodules: pin in superproject branch" &&
+		git -C ../submodule checkout -f master
+	) &&
+	(
+		cd submodule &&
+		echo line4b >>file &&
+		git add file &&
+		test_tick &&
+		git commit -m "upstream line4b"
+	) &&
+	(
+		cd super &&
+
+		git submodule update --remote-pinned --remote --force submodule &&
+		git status --porcelain --untracked=no --ignore-submodules=none >actual &&
+		test_must_be_empty actual &&
+
+		git submodule update --remote-default-to-master --remote --force submodule &&
+		git -C submodule log -1 --oneline >actual &&
+		git -C ../submodule log -1 --oneline >expect &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
 	(cd submodule &&
 	 git checkout test-branch &&
-- 
2.19.0