threads / discuss / 42955

[PATCHv3 0/7] submodule update: allow '.' for branch value

Subject: [PATCHv3 0/7] submodule update: allow '.' for branch value

## tl;dr

12 messages between Jul 29, 2016 and Aug 3, 2016.

replies: 11people: 3as markdown or json

Stefan Beller· Jul 29, 2016, 00:44 UTC · lore
This got bigger than expected, but I am happier with the results.

The meat is found in the last patch. (At least what I am interested in; others may be more interested in the second patch which could be argued to be a real bug fix to be merged down to maint.)

Thanks Junio for the thourough review of the first patches, Stefan

Stefan Beller (7):
  t7406: future proof tests with hard coded depth
  submodule update: respect depth in subsequent fetches
  submodule update: narrow scope of local variable
  submodule--helper: fix usage string for relative-path
  submodule-config: keep configured branch around
  submodule--helper: add remote-branch helper
  submodule update: allow '.' for branch value
 builtin/submodule--helper.c | 48 ++++++++++++++++++++++++++++--
 git-submodule.sh            | 11 +++----
 submodule-config.c          | 11 ++++++-
 submodule-config.h          |  1 +
 t/t7406-submodule-update.sh | 71 +++++++++++++++++++++++++++++++++++++++------
 5 files changed, 125 insertions(+), 17 deletions(-)
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 1/7] t7406: future proof tests with hard coded depth

The prior hard coded depth was chosen to be exactly the length from the recorded gitlink to the tip of the remote, so if you add more commits to the remote before, this test will not test its intention any more.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7406-submodule-update.sh | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 88e9750..8fc3a25 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -841,16 +841,19 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
 '
 
 test_expect_success 'submodule update clone shallow submodule' '
+	test_when_finished "rm -rf super3" &&
+	first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+	second=$(git -C submodule rev-parse HEAD) &&
+	commit_count=$(git -C submodule rev-list $first^..$second | wc -l) &&
 	git clone cloned super3 &&
 	pwd=$(pwd) &&
-	(cd super3 &&
-	 sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
-	 mv -f .gitmodules.tmp .gitmodules &&
-	 git submodule update --init --depth=3
-	 (cd submodule &&
-	  test 1 = $(git log --oneline | wc -l)
-	 )
-)
+	(
+		cd super3 &&
+		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
+		mv -f .gitmodules.tmp .gitmodules &&
+		git submodule update --init --depth=$commit_count &&
+		test 1 = $(git -C submodule log --oneline | wc -l)
+	)
 '
 
 test_expect_success 'submodule update --recursive drops module name before recursing' '
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 2/7] submodule update: respect depth in subsequent fetches

When depth is given the user may have a reasonable expectation that any remote operation is using the given depth. Add a test to demonstrate we still get the desired sha1 even if the depth is too short to include the actual commit.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  9 +++++----
 t/t7406-submodule-update.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 4ec7546..174f4ea 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -481,7 +481,8 @@ fetch_in_submodule () (
 	'')
 		git fetch ;;
 	*)
-		git fetch $(get_default_remote) "$2" ;;
+		shift
+		git fetch $(get_default_remote) "$@" ;;
 	esac
 )
 
@@ -619,7 +620,7 @@ cmd_update()
 			if test -z "$nofetch"
 			then
 				# Fetch remote before determining tracking $sha1
-				fetch_in_submodule "$sm_path" ||
+				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)
@@ -642,13 +643,13 @@ cmd_update()
 				# Run fetch only if $sha1 isn't present or it
 				# is not reachable from a ref.
 				is_tip_reachable "$sm_path" "$sha1" ||
-				fetch_in_submodule "$sm_path" ||
+				fetch_in_submodule "$sm_path" $depth ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 
 				# Now we tried the usual fetch, but $sha1 may
 				# not be reachable from any of the refs
 				is_tip_reachable "$sm_path" "$sha1" ||
-				fetch_in_submodule "$sm_path" "$sha1" ||
+				fetch_in_submodule "$sm_path" $depth "$sha1" ||
 				die "$(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"
 			fi
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 8fc3a25..1bb1f43 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -856,6 +856,22 @@ test_expect_success 'submodule update clone shallow submodule' '
 	)
 '
 
+test_expect_success 'submodule update clone shallow submodule outside of depth' '
+	test_when_finished "rm -rf super3" &&
+	git clone cloned super3 &&
+	pwd=$(pwd) &&
+	(
+		cd super3 &&
+		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
+		mv -f .gitmodules.tmp .gitmodules &&
+		test_must_fail git submodule update --init --depth=1 2>actual &&
+		test_i18ngrep "Direct fetching of that commit failed." actual &&
+		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
+		git submodule update --init --depth=1 >actual &&
+		test 1 = $(git -C submodule log --oneline | wc -l)
+	)
+'
+
 test_expect_success 'submodule update --recursive drops module name before recursing' '
 	(cd super2 &&
 	 (cd deeper/submodule/subsubmodule &&
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 4/7] submodule--helper: fix usage string for relative-path

Internally we call the underscore version of relative_path, but externally we present an API with no underscores.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b22352b..fb90c64 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -892,7 +892,7 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix
 {
 	struct strbuf sb = STRBUF_INIT;
 	if (argc != 3)
-		die("submodule--helper relative_path takes exactly 2 arguments, got %d", argc);
+		die("submodule--helper relative-path takes exactly 2 arguments, got %d", argc);
 
 	printf("%s", relative_path(argv[1], argv[2], &sb));
 	strbuf_release(&sb);
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 5/7] submodule-config: keep configured branch around

The branch field will be used in a later patch by `submodule update`.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 11 ++++++++++-
 submodule-config.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/submodule-config.c b/submodule-config.c
index 077db40..ebee1e4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -59,6 +59,7 @@ static void free_one_config(struct submodule_entry *entry)
 {
 	free((void *) entry->config->path);
 	free((void *) entry->config->name);
+	free((void *) entry->config->branch);
 	free((void *) entry->config->update_strategy.command);
 	free(entry->config);
 }
@@ -199,6 +200,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	submodule->update_strategy.command = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
+	submodule->branch = NULL;
 	submodule->recommend_shallow = -1;
 
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
@@ -358,9 +360,16 @@ static int parse_config(const char *var, const char *value, void *data)
 		if (!me->overwrite && submodule->recommend_shallow != -1)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					     "shallow");
-		else {
+		else
 			submodule->recommend_shallow =
 				git_config_bool(var, value);
+	} else if (!strcmp(item.buf, "branch")) {
+		if (!me->overwrite && submodule->branch)
+			warn_multiple_config(me->commit_sha1, submodule->name,
+					     "branch");
+		else {
+			free((void *)submodule->branch);
+			submodule->branch = xstrdup(value);
 		}
 	}
 
diff --git a/submodule-config.h b/submodule-config.h
index b1fdcc0..d05c542 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -15,6 +15,7 @@ struct submodule {
 	const char *url;
 	int fetch_recurse;
 	const char *ignore;
+	const char *branch;
 	struct submodule_update_strategy update_strategy;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 3/7] submodule update: narrow scope of local variable

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 174f4ea..0d4021f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -591,7 +591,6 @@ cmd_update()
 
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
-		branch=$(get_submodule_config "$name" branch master)
 		if ! test -z "$update"
 		then
 			update_module=$update
@@ -617,6 +616,7 @@ cmd_update()
 
 		if test -n "$remote"
 		then
+			branch=$(get_submodule_config "$name" branch master)
 			if test -z "$nofetch"
 			then
 				# Fetch remote before determining tracking $sha1
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 7/7] submodule update: allow '.' for branch value

Gerrit has a "superproject subscription" feature[1], that triggers a commit in a superproject that is subscribed to its submodules. Conceptually this Gerrit feature can be done on the client side with Git via (except for raciness, error handling etc):

  while [ true ]; do
    git -C <superproject> submodule update --remote --force
    git -C <superproject> commit -a -m "Update submodules"
    git -C <superproject> push
  done

for each branch in the superproject. To ease the configuration in Gerrit a special value of "." has been introduced for the submodule.<name>.branch to mean the same branch as the superproject[2], such that you can create a new branch on both superproject and the submodule and this feature continues to work on that new branch.

Now we find projects in the wild with such a .gitmodules file. The .gitmodules used in these Gerrit projects do not conform to Gits understanding of how .gitmodules should look like. This teaches Git to deal gracefully with this syntax as well.

The redefinition of "." does no harm to existing projects unaware of this change, as "." is an invalid branch name in Git, so we do not expect such projects to exist.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 18 ++++++++++++++++++
 t/t7406-submodule-update.sh | 36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 710048f..ae88eff 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -909,6 +909,24 @@ static const char *remote_submodule_branch(const char *path)
 	if (!sub->branch)
 		return "master";
 
+	if (!strcmp(sub->branch, ".")) {
+		unsigned char sha1[20];
+		const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
+
+		if (!refname)
+			die(_("No such ref: %s"), "HEAD");
+
+		/* detached HEAD */
+		if (!strcmp(refname, "HEAD"))
+			die(_("Submodule (%s) branch configured to inherit "
+			      "branch from superproject, but the superproject "
+			      "is not on any branch"), sub->name);
+
+		if (!skip_prefix(refname, "refs/heads/", &refname))
+			die(_("Expecting a full ref name, got %s"), refname);
+		return refname;
+	}
+
 	return sub->branch;
 }
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1bb1f43..1c4c1f2 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -209,9 +209,43 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '
 	)
 '
 
+test_expect_success 'submodule update --remote should fetch upstream changes with .' '
+	(
+		cd super &&
+		git config -f .gitmodules submodule."submodule".branch "." &&
+		git add .gitmodules &&
+		git commit -m "submodules: update from the respective superproject branch"
+	) &&
+	(
+		cd submodule &&
+		echo line4a >> file &&
+		git add file &&
+		test_tick &&
+		git commit -m "upstream line4a" &&
+		git checkout -b test-branch &&
+		test_commit on-test-branch
+	) &&
+	(
+		cd super &&
+		git submodule update --remote --force submodule &&
+		git -C submodule log -1 --oneline >actual
+		git -C ../submodule log -1 --oneline master >expect
+		test_cmp expect actual &&
+		git checkout -b test-branch &&
+		git submodule update --remote --force submodule &&
+		git -C submodule log -1 --oneline >actual
+		git -C ../submodule log -1 --oneline test-branch >expect
+		test_cmp expect actual &&
+		git checkout master &&
+		git branch -d test-branch &&
+		#~ git -C ../submodule branch -d test-branch &&
+		git reset --hard HEAD^
+	)
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
 	(cd submodule &&
-	 git checkout -b test-branch &&
+	 git checkout test-branch &&
 	 echo line5 >> file &&
 	 git add file &&
 	 test_tick &&
-- 
2.9.2.472.g1ffb07c.dirty
Stefan Beller· Jul 29, 2016, 00:44 UTC · re: Stefan Beller · lore

[PATCHv3 6/7] submodule--helper: add remote-branch helper

In a later patch we want to enhance the logic for the branch selection. Rewrite the current logic to be in C, so we can directly use C when we enhance the logic.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 28 +++++++++++++++++++++++++++-
 git-submodule.sh            |  2 +-
 2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fb90c64..710048f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -899,6 +899,31 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix
 	return 0;
 }
 
+static const char *remote_submodule_branch(const char *path)
+{
+	const struct submodule *sub;
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub->branch)
+		return "master";
+
+	return sub->branch;
+}
+
+static int resolve_remote_submodule_branch(int argc, const char **argv,
+		const char *prefix)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (argc != 2)
+		die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
+
+	printf("%s", remote_submodule_branch(argv[1]));
+	strbuf_release(&sb);
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -912,7 +937,8 @@ static struct cmd_struct commands[] = {
 	{"relative-path", resolve_relative_path},
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
-	{"init", module_init}
+	{"init", module_init},
+	{"remote-branch", resolve_remote_submodule_branch}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 0d4021f..1e493a8 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -616,7 +616,7 @@ cmd_update()
 
 		if test -n "$remote"
 		then
-			branch=$(get_submodule_config "$name" branch master)
+			branch=$(git submodule--helper remote-branch "$sm_path")
 			if test -z "$nofetch"
 			then
 				# Fetch remote before determining tracking $sha1
-- 
2.9.2.472.g1ffb07c.dirty
Jeff King· Aug 3, 2016, 16:25 UTC · re: Stefan Beller · lore

Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper

On Thu, Jul 28, 2016 at 05:44:08PM -0700, Stefan Beller wrote:
Show 12 quoted lines
> +static const char *remote_submodule_branch(const char *path)
> +{
> +	const struct submodule *sub;
> +	gitmodules_config();
> +	git_config(submodule_config, NULL);
> +
> +	sub = submodule_from_path(null_sha1, path);
> +	if (!sub->branch)
> +		return "master";
> +
> +	return sub->branch;
> +}

Coverity complains about "sub" being NULL here, and indeed, it seems like an easy segfault:

  $ ./git submodule--helper remote-branch foo
  Segmentation fault
I guess this should return NULL in that case. But then this...
Show 11 quoted lines
> +static int resolve_remote_submodule_branch(int argc, const char **argv,
> +		const char *prefix)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	if (argc != 2)
> +		die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
> +
> +	printf("%s", remote_submodule_branch(argv[1]));
> +	strbuf_release(&sb);
> +	return 0;
> +}

would need to handle the NULL return, as well. So maybe "master" or the empty string would be better. I haven't followed the topic closely enough to have an opinion.

-Peff
Stefan Beller· Aug 3, 2016, 20:11 UTC · re: Jeff King · lore

Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper

On Wed, Aug 3, 2016 at 9:25 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Thu, Jul 28, 2016 at 05:44:08PM -0700, Stefan Beller wrote:
>
>> +static const char *remote_submodule_branch(const char *path)
>> +{
>> +     const struct submodule *sub;
>> +     gitmodules_config();
>> +     git_config(submodule_config, NULL);
>> +
>> +     sub = submodule_from_path(null_sha1, path);
>> +     if (!sub->branch)
>> +             return "master";
>> +
>> +     return sub->branch;
>> +}
>
> Coverity complains about "sub" being NULL here, and indeed, it seems
> like an easy segfault:
>
>   $ ./git submodule--helper remote-branch foo
>   Segmentation fault
>
> I guess this should return NULL in that case. But then this...

I thought about checking for (!sub && !sub->branch) to return "master"; However I disregarded that implementation as the submodule-config is reliable for any correct input. Though we need to handle wrong input as well. So how about:

  if (!sub)
    die(_("A submodule doesn't exist at path %s"), path);
  if (!sub->branch)
    return "master";
...
I think that explains best why we even check for !sub.
Show 16 quoted lines
>
>> +static int resolve_remote_submodule_branch(int argc, const char **argv,
>> +             const char *prefix)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     if (argc != 2)
>> +             die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
>> +
>> +     printf("%s", remote_submodule_branch(argv[1]));
>> +     strbuf_release(&sb);
>> +     return 0;
>> +}
>
> would need to handle the NULL return, as well. So maybe "master" or the
> empty string would be better. I haven't followed the topic closely
> enough to have an opinion.

Oh I see, this can turn into a discussion what the API for remote_submodule_branch is. I think handling the NULL here and dying makes more sense as then we can keep remote_submodule_branch pure to its intended use case. (If in the far future we have all the submodule stuff in C, we may want to call remote_submodule_branch when we already know that the path is valid, so no need to check it in there. Also not dying in there is more libified.)

Thanks for this review, Stefan

>
> -Peff
Jeff King· Aug 3, 2016, 20:15 UTC · re: Stefan Beller · lore

Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper

On Wed, Aug 03, 2016 at 01:11:51PM -0700, Stefan Beller wrote:
Show 17 quoted lines
> > Coverity complains about "sub" being NULL here, and indeed, it seems
> > like an easy segfault:
> >
> >   $ ./git submodule--helper remote-branch foo
> >   Segmentation fault
> >
> > I guess this should return NULL in that case. But then this...
> 
> I thought about checking for (!sub && !sub->branch) to return "master";
> However I disregarded that implementation as the submodule-config is
> reliable for any correct input. Though we need to handle wrong input as
> well. So how about:
> 
>   if (!sub)
>     die(_("A submodule doesn't exist at path %s"), path);
>   if (!sub->branch)
>     return "master";

OK by me, as long as the caller of submodule--helper handles the death reasonably. Since it's an internal helper program, we really only have to worry about what git-submodule.sh does with it, which is good.

Show 11 quoted lines
> > would need to handle the NULL return, as well. So maybe "master" or the
> > empty string would be better. I haven't followed the topic closely
> > enough to have an opinion.
> 
> Oh I see, this can turn into a discussion what the API for
> remote_submodule_branch is. I think handling the NULL here and dying makes
> more sense as then we can keep remote_submodule_branch pure to its
> intended use case. (If in the far future we have all the submodule stuff in C,
> we may want to call remote_submodule_branch when we already know
> that the path is valid, so no need to check it in there. Also not dying in there
> is more libified.)
Yep, that makes sense to me, too.
-Peff
Junio C Hamano· Aug 1, 2016, 21:48 UTC · re: Stefan Beller · lore

Re: [PATCHv3 0/7] submodule update: allow '.' for branch value

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> This got bigger than expected, but I am happier with the results.
>
> The meat is found in the last patch. (At least what I am interested in; others
> may be more interested in the second patch which could be argued to be a real
> bug fix to be merged down to maint.)
Indeed 2/7 and 7/7 are both interesting.
Will queue; thanks.

← back to recent threads