[PATCH v4 0/3] implement fetching of moved submodules

[PATCH v4 0/3] implement fetching of moved submodules

[Heiko Voigt]

The previous RFC iteration can be found here:

https://public-inbox.org/git/20171006222544.GA26642@sandbox/

This should now be in a state ready for review for inclusion.

The main difference from last iteration is that we now also support
unconfigured gitlinks for push and fetch for backwards compatibility.

To implement this compatibility we construct a default name for gitlinks
if there is a repository found at their location in the worktree.

Cheers Heiko

Heiko Voigt (3):
  fetch: add test to make sure we stay backwards compatible
  implement fetching of moved submodules
  submodule: simplify decision tree whether to or not to fetch

 submodule-config.h          |   3 +
 submodule.c                 | 200 +++++++++++++++++++++++++++++---------------
 t/t5526-fetch-submodules.sh |  77 ++++++++++++++++-
 3 files changed, 210 insertions(+), 70 deletions(-)

-- 
2.14.1.145.gb3622a4

[PATCH v4 1/3] fetch: add test to make sure we stay backwards compatible

[Heiko Voigt]

The current implementation of submodules supports on-demand fetch if
there is no .gitmodules entry for a submodule. Let's add a test to
document this behavior.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 t/t5526-fetch-submodules.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 42251f7f3a..43a22f680f 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -478,7 +478,47 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
 		git fetch >../actual.out 2>../actual.err
 	) &&
 	! test -s actual.out &&
-	test_i18ncmp expect.err actual.err
+	test_i18ncmp expect.err actual.err &&
+	(
+		cd submodule &&
+		git checkout -q master
+	)
+'
+
+test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .gitmodule entry" '
+	(
+		cd downstream &&
+		git fetch --recurse-submodules
+	) &&
+	add_upstream_commit &&
+	head1=$(git rev-parse --short HEAD) &&
+	git add submodule &&
+	git rm .gitmodules &&
+	git commit -m "new submodule without .gitmodules" &&
+	printf "" >expect.out &&
+	head2=$(git rev-parse --short HEAD) &&
+	echo "From $pwd/." >expect.err.2 &&
+	echo "   $head1..$head2  master     -> origin/master" >>expect.err.2 &&
+	head -3 expect.err >>expect.err.2 &&
+	(
+		cd downstream &&
+		rm .gitmodules &&
+		git config fetch.recurseSubmodules on-demand &&
+		# fake submodule configuration to avoid skipping submodule handling
+		git config -f .gitmodules submodule.fake.path fake &&
+		git config -f .gitmodules submodule.fake.url fakeurl &&
+		git add .gitmodules &&
+		git config --unset submodule.submodule.url &&
+		git fetch >../actual.out 2>../actual.err &&
+		# cleanup
+		git config --unset fetch.recurseSubmodules &&
+		git reset --hard
+	) &&
+	test_i18ncmp expect.out actual.out &&
+	test_i18ncmp expect.err.2 actual.err &&
+	git checkout HEAD^ -- .gitmodules &&
+	git add .gitmodules &&
+	git commit -m "new submodule restored .gitmodules"
 '
 
 test_expect_success 'fetching submodules respects parallel settings' '
-- 
2.14.1.145.gb3622a4

[PATCH v4 2/3] implement fetching of moved submodules

[Heiko Voigt]

We store the changed submodules paths to calculate which submodule needs
fetching. This does not work for moved submodules since their paths do
not stay the same in case of a moved submodules. In case of new
submodules we do not have a path in the current checkout, since they
just appeared in this fetch.

It is more general to collect the submodule names for changes instead of
their paths to include the above cases. If we do not have a
configuration for a gitlink we rely on constructing a default name from
the path if a git repository can be found at its path. We skip
non-configured gitlinks whose default name collides with a configured
one.

With the change described above we implement 'on-demand' fetching of
changes in moved submodules.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule-config.h          |   3 +
 submodule.c                 | 138 ++++++++++++++++++++++++++++++++------------
 t/t5526-fetch-submodules.sh |  35 +++++++++++
 3 files changed, 138 insertions(+), 38 deletions(-)

diff --git a/submodule-config.h b/submodule-config.h
index e3845831f6..a5503a5d17 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -22,6 +22,9 @@ struct submodule {
 	int recommend_shallow;
 };
 
+#define SUBMODULE_INIT { NULL, NULL, NULL, RECURSE_SUBMODULES_NONE, \
+	NULL, NULL, SUBMODULE_UPDATE_STRATEGY_INIT, {0}, -1 };
+
 struct submodule_cache;
 struct repository;
 
diff --git a/submodule.c b/submodule.c
index 63e7094e16..71d1773e2e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -21,7 +21,7 @@
 #include "parse-options.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
+static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -674,11 +674,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)
 }
 
 static struct oid_array *submodule_commits(struct string_list *submodules,
-					   const char *path)
+					   const char *name)
 {
 	struct string_list_item *item;
 
-	item = string_list_insert(submodules, path);
+	item = string_list_insert(submodules, name);
 	if (item->util)
 		return (struct oid_array *) item->util;
 
@@ -687,39 +687,67 @@ static struct oid_array *submodule_commits(struct string_list *submodules,
 	return (struct oid_array *) item->util;
 }
 
+struct collect_changed_submodules_cb_data {
+	struct string_list *changed;
+	const struct object_id *commit_oid;
+};
+
+/*
+ * this would normally be two functions: default_name_from_path() and
+ * path_from_default_name(). Since the default name is the same as
+ * the submodule path we can get away with just one function which only
+ * checks whether there is a submodule in the working directory at that
+ * location.
+ */
+static const char *default_name_or_path(const char *path_or_name)
+{
+	int error_code;
+
+	if (!is_submodule_populated_gently(path_or_name, &error_code))
+		return NULL;
+
+	return path_or_name;
+}
+
 static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 					  struct diff_options *options,
 					  void *data)
 {
+	struct collect_changed_submodules_cb_data *me = data;
+	struct string_list *changed = me->changed;
+	const struct object_id *commit_oid = me->commit_oid;
 	int i;
-	struct string_list *changed = data;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		struct oid_array *commits;
+		const struct submodule *submodule;
+		const char *name;
+
 		if (!S_ISGITLINK(p->two->mode))
 			continue;
 
-		if (S_ISGITLINK(p->one->mode)) {
-			/*
-			 * NEEDSWORK: We should honor the name configured in
-			 * the .gitmodules file of the commit we are examining
-			 * here to be able to correctly follow submodules
-			 * being moved around.
-			 */
-			commits = submodule_commits(changed, p->two->path);
-			oid_array_append(commits, &p->two->oid);
-		} else {
-			/* Submodule is new or was moved here */
-			/*
-			 * NEEDSWORK: When the .git directories of submodules
-			 * live inside the superprojects .git directory some
-			 * day we should fetch new submodules directly into
-			 * that location too when config or options request
-			 * that so they can be checked out from there.
-			 */
-			continue;
+		submodule = submodule_from_path(commit_oid, p->two->path);
+		if (submodule)
+			name = submodule->name;
+		else {
+			name = default_name_or_path(p->two->path);
+			/* make sure name does not collide with existing one */
+			submodule = submodule_from_name(commit_oid, name);
+			if (submodule) {
+				warning("Submodule in commit %s at path: "
+					"'%s' collides with a submodule named "
+					"the same. Skipping it.",
+					oid_to_hex(commit_oid), name);
+				name = NULL;
+			}
 		}
+
+		if (!name)
+			continue;
+
+		commits = submodule_commits(changed, name);
+		oid_array_append(commits, &p->two->oid);
 	}
 }
 
@@ -742,11 +770,14 @@ static void collect_changed_submodules(struct string_list *changed,
 
 	while ((commit = get_revision(&rev))) {
 		struct rev_info diff_rev;
+		struct collect_changed_submodules_cb_data data;
+		data.changed = changed;
+		data.commit_oid = &commit->object.oid;
 
 		init_revisions(&diff_rev, NULL);
 		diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 		diff_rev.diffopt.format_callback = collect_changed_submodules_cb;
-		diff_rev.diffopt.format_callback_data = changed;
+		diff_rev.diffopt.format_callback_data = &data;
 		diff_tree_combined_merge(commit, 1, &diff_rev);
 	}
 
@@ -894,7 +925,7 @@ int find_unpushed_submodules(struct oid_array *commits,
 		const char *remotes_name, struct string_list *needs_pushing)
 {
 	struct string_list submodules = STRING_LIST_INIT_DUP;
-	struct string_list_item *submodule;
+	struct string_list_item *name;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 
 	/* argv.argv[0] will be ignored by setup_revisions */
@@ -905,9 +936,19 @@ int find_unpushed_submodules(struct oid_array *commits,
 
 	collect_changed_submodules(&submodules, &argv);
 
-	for_each_string_list_item(submodule, &submodules) {
-		struct oid_array *commits = submodule->util;
-		const char *path = submodule->string;
+	for_each_string_list_item(name, &submodules) {
+		struct oid_array *commits = name->util;
+		const struct submodule *submodule;
+		const char *path = NULL;
+
+		submodule = submodule_from_name(&null_oid, name->string);
+		if (submodule)
+			path = submodule->path;
+		else
+			path = default_name_or_path(name->string);
+
+		if (!path)
+			continue;
 
 		if (submodule_needs_pushing(path, commits))
 			string_list_insert(needs_pushing, path);
@@ -1065,7 +1106,7 @@ static void calculate_changed_submodule_paths(void)
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
 	struct string_list changed_submodules = STRING_LIST_INIT_DUP;
-	const struct string_list_item *item;
+	const struct string_list_item *name;
 
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
@@ -1080,16 +1121,26 @@ static void calculate_changed_submodule_paths(void)
 
 	/*
 	 * Collect all submodules (whether checked out or not) for which new
-	 * commits have been recorded upstream in "changed_submodule_paths".
+	 * commits have been recorded upstream in "changed_submodule_names".
 	 */
 	collect_changed_submodules(&changed_submodules, &argv);
 
-	for_each_string_list_item(item, &changed_submodules) {
-		struct oid_array *commits = item->util;
-		const char *path = item->string;
+	for_each_string_list_item(name, &changed_submodules) {
+		struct oid_array *commits = name->util;
+		const struct submodule *submodule;
+		const char *path = NULL;
+
+		submodule = submodule_from_name(&null_oid, name->string);
+		if (submodule)
+			path = submodule->path;
+		else
+			path = default_name_or_path(name->string);
+
+		if (!path)
+			continue;
 
 		if (!submodule_has_commits(path, commits))
-			string_list_append(&changed_submodule_paths, path);
+			string_list_append(&changed_submodule_names, name->string);
 	}
 
 	free_submodules_oids(&changed_submodules);
@@ -1149,11 +1200,19 @@ static int get_next_submodule(struct child_process *cp,
 		const struct cache_entry *ce = active_cache[spf->count];
 		const char *git_dir, *default_argv;
 		const struct submodule *submodule;
+		struct submodule default_submodule = SUBMODULE_INIT;
 
 		if (!S_ISGITLINK(ce->ce_mode))
 			continue;
 
 		submodule = submodule_from_path(&null_oid, ce->name);
+		if (!submodule) {
+			const char *name = default_name_or_path(ce->name);
+			if (name) {
+				default_submodule.path = default_submodule.name = name;
+				submodule = &default_submodule;
+			}
+		}
 
 		default_argv = "yes";
 		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
@@ -1175,7 +1234,8 @@ static int get_next_submodule(struct child_process *cp,
 				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
 					continue;
 				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					if (!unsorted_string_list_lookup(&changed_submodule_names,
+									 submodule->name))
 						continue;
 					default_argv = "on-demand";
 				}
@@ -1183,13 +1243,15 @@ static int get_next_submodule(struct child_process *cp,
 				if (spf->default_option == RECURSE_SUBMODULES_OFF)
 					continue;
 				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					if (!unsorted_string_list_lookup(&changed_submodule_names,
+									  submodule->name))
 						continue;
 					default_argv = "on-demand";
 				}
 			}
 		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
-			if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+			if (!unsorted_string_list_lookup(&changed_submodule_names,
+							 submodule->name))
 				continue;
 			default_argv = "on-demand";
 		}
@@ -1282,7 +1344,7 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 	argv_array_clear(&spf.args);
 out:
-	string_list_clear(&changed_submodule_paths, 1);
+	string_list_clear(&changed_submodule_names, 1);
 	return spf.result;
 }
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 43a22f680f..a552ad4ead 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -570,4 +570,39 @@ test_expect_success 'fetching submodule into a broken repository' '
 	test_must_fail git -C dst fetch --recurse-submodules
 '
 
+test_expect_success "fetch new commits when submodule got renamed" '
+	git clone . downstream_rename &&
+	(
+		cd downstream_rename &&
+		git submodule update --init &&
+# NEEDSWORK: we omitted --recursive for the submodule update here since
+# that does not work. See test 7001 for mv "moving nested submodules"
+# for details. Once that is fixed we should add the --recursive option
+# here.
+		git checkout -b rename &&
+		git mv submodule submodule_renamed &&
+		(
+			cd submodule_renamed &&
+			git checkout -b rename_sub &&
+			echo a >a &&
+			git add a &&
+			git commit -ma &&
+			git push origin rename_sub &&
+			git rev-parse HEAD >../../expect
+		) &&
+		git add submodule_renamed &&
+		git commit -m "update renamed submodule" &&
+		git push origin rename
+	) &&
+	(
+		cd downstream &&
+		git fetch --recurse-submodules=on-demand &&
+		(
+			cd submodule &&
+			git rev-parse origin/rename_sub >../../actual
+		)
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.14.1.145.gb3622a4

[PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Heiko Voigt]

To make extending this logic later easier.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/submodule.c b/submodule.c
index 71d1773e2e..82d206eb65 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
 
+static int get_fetch_recurse_config(const struct submodule *submodule,
+				    struct submodule_parallel_fetch *spf)
+{
+	if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
+		return spf->command_line_option;
+
+	if (submodule) {
+		char *key;
+		const char *value;
+
+		int fetch_recurse = submodule->fetch_recurse;
+		key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
+		if (!repo_config_get_string_const(the_repository, key, &value)) {
+			fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
+		}
+		free(key);
+
+		if (fetch_recurse != RECURSE_SUBMODULES_NONE)
+			/* local config overrules everything except commandline */
+			return fetch_recurse;
+	}
+
+	return spf->default_option;
+}
+
 static int get_next_submodule(struct child_process *cp,
 			      struct strbuf *err, void *data, void **task_cb)
 {
@@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,
 			}
 		}
 
-		default_argv = "yes";
-		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-			int fetch_recurse = RECURSE_SUBMODULES_NONE;
-
-			if (submodule) {
-				char *key;
-				const char *value;
-
-				fetch_recurse = submodule->fetch_recurse;
-				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
-				if (!repo_config_get_string_const(the_repository, key, &value)) {
-					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
-				}
-				free(key);
-			}
-
-			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
-				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
-					continue;
-				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_names,
-									 submodule->name))
-						continue;
-					default_argv = "on-demand";
-				}
-			} else {
-				if (spf->default_option == RECURSE_SUBMODULES_OFF)
-					continue;
-				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_names,
-									  submodule->name))
-						continue;
-					default_argv = "on-demand";
-				}
-			}
-		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
-			if (!unsorted_string_list_lookup(&changed_submodule_names,
+		switch (get_fetch_recurse_config(submodule, spf))
+		{
+		default:
+		case RECURSE_SUBMODULES_DEFAULT:
+		case RECURSE_SUBMODULES_ON_DEMAND:
+			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
 							 submodule->name))
 				continue;
 			default_argv = "on-demand";
+			break;
+		case RECURSE_SUBMODULES_ON:
+			default_argv = "yes";
+			break;
+		case RECURSE_SUBMODULES_OFF:
+			continue;
 		}
 
 		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
-- 
2.14.1.145.gb3622a4

Re: [PATCH v4 0/3] implement fetching of moved submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> The previous RFC iteration can be found here:
>
> https://public-inbox.org/git/20171006222544.GA26642@sandbox/
>
> This should now be in a state ready for review for inclusion.
>
> The main difference from last iteration is that we now also support
> unconfigured gitlinks for push and fetch for backwards compatibility.
>
> To implement this compatibility we construct a default name for gitlinks
> if there is a repository found at their location in the worktree.

I do not remember the details of the patch in the previous round
that corresponds to PATCH 2/3 here, so I cannot comment on the
incremental improvement between the two, but the fallback in 2/3
looks like a sensible thing to do.

Let's see what others, especially those who are interested in the
"--recurse-submodules" area, say.

Thanks.

Re: [PATCH v4 2/3] implement fetching of moved submodules

[Stefan Beller]

On Mon, Oct 16, 2017 at 6:58 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> We store the changed submodules paths to calculate which submodule needs
> fetching. This does not work for moved submodules since their paths do
> not stay the same in case of a moved submodules. In case of new
> submodules we do not have a path in the current checkout, since they
> just appeared in this fetch.
>
> It is more general to collect the submodule names for changes instead of
> their paths to include the above cases. If we do not have a
> configuration for a gitlink we rely on constructing a default name from
> the path if a git repository can be found at its path. We skip
> non-configured gitlinks whose default name collides with a configured
> one.

Thanks for working on this!

As detailed below, I wonder if it is easier (in maintenance, explaining
correctness, reviewing) if we'd rather keep two lists around. One for
based on names, and if we cannot lookup a name for a submodule, we
rather use the second path based list as a fallback. That would avoid
potential namespace collisions between names and paths, as well as
not having the confusion of mapping back and forth.

Most functions would then need to operate on path, as the name->path
mapping can be looked up for the first list, but the path->name mapping
cannot be looked up for the second list.

Cheers,
Stefan

> With the change described above we implement 'on-demand' fetching of
> changes in moved submodules.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

> ---
>  submodule-config.h          |   3 +
>  submodule.c                 | 138 ++++++++++++++++++++++++++++++++------------
>  t/t5526-fetch-submodules.sh |  35 +++++++++++
>  3 files changed, 138 insertions(+), 38 deletions(-)
>
> diff --git a/submodule-config.h b/submodule-config.h
> index e3845831f6..a5503a5d17 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -22,6 +22,9 @@ struct submodule {
>         int recommend_shallow;
>  };
>
> +#define SUBMODULE_INIT { NULL, NULL, NULL, RECURSE_SUBMODULES_NONE, \
> +       NULL, NULL, SUBMODULE_UPDATE_STRATEGY_INIT, {0}, -1 };
> +
>  struct submodule_cache;
>  struct repository;
>
> diff --git a/submodule.c b/submodule.c
> index 63e7094e16..71d1773e2e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -21,7 +21,7 @@
>  #include "parse-options.h"
>
>  static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
> -static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
> +static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
>  static int initialized_fetch_ref_tips;
>  static struct oid_array ref_tips_before_fetch;
>  static struct oid_array ref_tips_after_fetch;
> @@ -674,11 +674,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)
>  }
>
>  static struct oid_array *submodule_commits(struct string_list *submodules,
> -                                          const char *path)
> +                                          const char *name)
>  {
>         struct string_list_item *item;
>
> -       item = string_list_insert(submodules, path);
> +       item = string_list_insert(submodules, name);
>         if (item->util)
>                 return (struct oid_array *) item->util;
>
> @@ -687,39 +687,67 @@ static struct oid_array *submodule_commits(struct string_list *submodules,
>         return (struct oid_array *) item->util;
>  }
>
> +struct collect_changed_submodules_cb_data {
> +       struct string_list *changed;
> +       const struct object_id *commit_oid;
> +};
> +
> +/*
> + * this would normally be two functions: default_name_from_path() and

Please start the comment capitalised. (minor nit)

> + * path_from_default_name(). Since the default name is the same as
> + * the submodule path we can get away with just one function which only
> + * checks whether there is a submodule in the working directory at that
> + * location.

This is an interesting comment, as it hints that we ought to keep it that way.
Earlier I was wondering if we want to make the name distinctively different
than its path, as that will confuse users *less* IMHO. (I just remember
someone asking on the mailing list why their "rename" did not work, as
they just renamed everything in the .gitmodules that looked like the path)

As the path/name is confusing, I'd wish we'd be super concise, such that
errors are harder to sneak into. For example, the arguments name should
be "path" as that is the only thing we can look up using is_sub_pop_gently,
if a "name" is given, than it just works because the chosen default name
was its path.

> +static const char *default_name_or_path(const char *path_or_name)
> +{
> +       int error_code;
> +
> +       if (!is_submodule_populated_gently(path_or_name, &error_code))
> +               return NULL;
> +
> +       return path_or_name;
> +}
> +


> +               if (submodule)
> +                       name = submodule->name;
> +               else {
> +                       name = default_name_or_path(p->two->path);

Here we use the path, as expected. So ideally we'd use
"default_name_for_path".


> +                       /* make sure name does not collide with existing one */
> +                       submodule = submodule_from_name(commit_oid, name);
> +                       if (submodule) {
> +                               warning("Submodule in commit %s at path: "
> +                                       "'%s' collides with a submodule named "
> +                                       "the same. Skipping it.",
> +                                       oid_to_hex(commit_oid), name);
> +                               name = NULL;
> +                       }

This is the ugly part of using one string list and storing names or
path in it. I wonder if we could omit this warning if we had 2 string lists?
One for names (which will then be used for renamed and new submodules)
and the "fall back" path based list. In such a world we would not need
to map back and forth between names and path.

> +               submodule = submodule_from_name(&null_oid, name->string);
> +               if (submodule)
> +                       path = submodule->path;
> +               else
> +                       path = default_name_or_path(name->string);
> +
> +               if (!path)
> +                       continue;


> +               submodule = submodule_from_name(&null_oid, name->string);
> +               if (submodule)
> +                       path = submodule->path;
> +               else
> +                       path = default_name_or_path(name->string);
> +
> +               if (!path)
> +                       continue;


>                 submodule = submodule_from_path(&null_oid, ce->name);
> +               if (!submodule) {
> +                       const char *name = default_name_or_path(ce->name);
> +                       if (name) {
> +                               default_submodule.path = default_submodule.name = name;
> +                               submodule = &default_submodule;
> +                       }
> +               }
>

>
> +test_expect_success "fetch new commits when submodule got renamed" '
> +       git clone . downstream_rename &&
> +       (
> +               cd downstream_rename &&
> +               git submodule update --init &&
> +# NEEDSWORK: we omitted --recursive for the submodule update here since
> +# that does not work. See test 7001 for mv "moving nested submodules"
> +# for details. Once that is fixed we should add the --recursive option
> +# here.
> +               git checkout -b rename &&
> +               git mv submodule submodule_renamed &&
> +               (
> +                       cd submodule_renamed &&
> +                       git checkout -b rename_sub &&
> +                       echo a >a &&
> +                       git add a &&
> +                       git commit -ma &&
> +                       git push origin rename_sub &&
> +                       git rev-parse HEAD >../../expect
> +               ) &&
> +               git add submodule_renamed &&
> +               git commit -m "update renamed submodule" &&
> +               git push origin rename
> +       ) &&
> +       (
> +               cd downstream &&
> +               git fetch --recurse-submodules=on-demand &&
> +               (
> +                       cd submodule &&
> +                       git rev-parse origin/rename_sub >../../actual
> +               )
> +       ) &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.14.1.145.gb3622a4
>

Re: [PATCH v4 1/3] fetch: add test to make sure we stay backwards compatible

[Stefan Beller]

On Mon, Oct 16, 2017 at 6:57 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> The current implementation of submodules supports on-demand fetch if
> there is no .gitmodules entry for a submodule. Let's add a test to
> document this behavior.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  t/t5526-fetch-submodules.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 42251f7f3a..43a22f680f 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -478,7 +478,47 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
>                 git fetch >../actual.out 2>../actual.err
>         ) &&
>         ! test -s actual.out &&
> -       test_i18ncmp expect.err actual.err
> +       test_i18ncmp expect.err actual.err &&
> +       (
> +               cd submodule &&
> +               git checkout -q master
> +       )

For few instructions inside another repo, I tend to use the
-C option:

  git -C submodule checkout -q master

That saves a shell, which is noticeable cost on Windows I was told.
(also fewer lines to type).

Oh, I see, that is consistent with the rest of the file. Oh well.
(Otherwise I would have lobbied to even move it further up and
put it inside a test_when_finished "<cmd>"


> +'
> +
> +test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .gitmodule entry" '
> +       (
> +               cd downstream &&
> +               git fetch --recurse-submodules
> +       ) &&

This is consistent with the rest of the file as well, so I shall
refrain from complaining. ;)

> +       add_upstream_commit &&
> +       head1=$(git rev-parse --short HEAD) &&
> +       git add submodule &&
> +       git rm .gitmodules &&
> +       git commit -m "new submodule without .gitmodules" &&
> +       printf "" >expect.out &&

This could be just

    : >expect.out

no need to invoke a function to print nothing.

> +       head2=$(git rev-parse --short HEAD) &&
> +       echo "From $pwd/." >expect.err.2 &&
> +       echo "   $head1..$head2  master     -> origin/master" >>expect.err.2 &&
> +       head -3 expect.err >>expect.err.2 &&
> +       (
> +               cd downstream &&
> +               rm .gitmodules &&
> +               git config fetch.recurseSubmodules on-demand &&
> +               # fake submodule configuration to avoid skipping submodule handling
> +               git config -f .gitmodules submodule.fake.path fake &&
> +               git config -f .gitmodules submodule.fake.url fakeurl &&
> +               git add .gitmodules &&
> +               git config --unset submodule.submodule.url &&
> +               git fetch >../actual.out 2>../actual.err &&
> +               # cleanup
> +               git config --unset fetch.recurseSubmodules &&
> +               git reset --hard
> +       ) &&
> +       test_i18ncmp expect.out actual.out &&
> +       test_i18ncmp expect.err.2 actual.err &&
> +       git checkout HEAD^ -- .gitmodules &&
> +       git add .gitmodules &&
> +       git commit -m "new submodule restored .gitmodules"
>  '

Thanks for writing this test.
With or without the nits addressed, this is

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

Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Stefan Beller]

On Mon, Oct 16, 2017 at 6:59 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> To make extending this logic later easier.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

Thanks for this readability fix!

Stefan

Re: [PATCH v4 2/3] implement fetching of moved submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> +                       /* make sure name does not collide with existing one */
>> +                       submodule = submodule_from_name(commit_oid, name);
>> +                       if (submodule) {
>> +                               warning("Submodule in commit %s at path: "
>> +                                       "'%s' collides with a submodule named "
>> +                                       "the same. Skipping it.",
>> +                                       oid_to_hex(commit_oid), name);
>> +                               name = NULL;
>> +                       }
>
> This is the ugly part of using one string list and storing names or
> path in it. I wonder if we could omit this warning if we had 2 string lists?

We are keying off of 'name', because that is what will give a module
its identity.  If we have a gitlink whose path is not in .gitmodules
in the same tree, then we are seeing an unregistered submodule.  If
we were to "git add" it, then we'd use its path as the default name,
but if we already have a submodule with that name (the most likely
explanation for its existence is because it started its life there
and then later moved), and the submodule is bound to a different
path, then that is a different submodule.  Skipping and warning both
are sensible thing to do.

I do not know what you see as ugly here, and more importantly, I am
not sure how having two lists would help.

Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Oct 16, 2017 at 6:59 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> To make extending this logic later easier.
>>
>> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
>
> Thanks for this readability fix!
>
> Stefan

Thanks, both.

Re: [PATCH v4 2/3] implement fetching of moved submodules

[Stefan Beller]

On Tue, Oct 17, 2017 at 5:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> +                       /* make sure name does not collide with existing one */
>>> +                       submodule = submodule_from_name(commit_oid, name);
>>> +                       if (submodule) {
>>> +                               warning("Submodule in commit %s at path: "
>>> +                                       "'%s' collides with a submodule named "
>>> +                                       "the same. Skipping it.",
>>> +                                       oid_to_hex(commit_oid), name);
>>> +                               name = NULL;
>>> +                       }
>>
>> This is the ugly part of using one string list and storing names or
>> path in it. I wonder if we could omit this warning if we had 2 string lists?
>
> We are keying off of 'name', because that is what will give a module
> its identity.  If we have a gitlink whose path is not in .gitmodules
> in the same tree, then we are seeing an unregistered submodule.

Right, so it has no submodule specific identity and we chose to "fake it"
by pretending its path is its name. However this requires checking as
there might be overlap in the name-namespace and the path-namespace.


>  If
> we were to "git add" it, then we'd use its path as the default name,

I presume "git submodule add"

> but if we already have a submodule with that name (the most likely
> explanation for its existence is because it started its life there
> and then later moved), and the submodule is bound to a different
> path, then that is a different submodule.  Skipping and warning both
> are sensible thing to do.

Skipping and warning is sensible once we decide to go this way.

I propose to take a step back and not throw away the information
whether the given string is a name or path, as then we do not have
to warn&skip, but we can treat both correctly.

As we only need to store an additional boolean (is it path or name?),
I had suggested to just use two lists, one for key-by-name and one
key-by-path, where we intend to use the key-by-name for submodules
and the by-path only for those with no name (i.e. lone gitlinks), hence
making this a "fallback list"

>
> I do not know what you see as ugly here,

the necessity of warn&skip instead of having a solution that
works in corner cases just fine.

> and more importantly, I am
> not sure how having two lists would help.

The current situation is that we use the path of the submodules only,
which makes it work without warn&skip, but it has other disadvantages
(i.e. new & moved submodules are not detected), which we want to fix.

We can add this functionality without caving in to skip the corner case
by storing an additional bit of information. The renaming is detected by
having a constant name before and after, just the path changed.
So we could continue to use by-path logic and only have the name
for rename detection. However that seems to be ugly, too. So we
seem to think that the by-name is better (as it is more in line with what
we think should happen, it is easier to explain, review and maintain(?)).

So we could have by-name keys, with the extra information of whether the
key is genuine or a "fake" key, which is t be resolved to a path instead.
And as that is just one bit, I proposed two lists for that.

Do I miss an essential part here?

Thanks,
Stefan

Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Brandon Williams]

On 10/16, Heiko Voigt wrote:
> To make extending this logic later easier.

This makes things so much clearer, thanks!

> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 71d1773e2e..82d206eb65 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {
>  };
>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
>  
> +static int get_fetch_recurse_config(const struct submodule *submodule,
> +				    struct submodule_parallel_fetch *spf)
> +{
> +	if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
> +		return spf->command_line_option;
> +
> +	if (submodule) {
> +		char *key;
> +		const char *value;
> +
> +		int fetch_recurse = submodule->fetch_recurse;
> +		key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> +		if (!repo_config_get_string_const(the_repository, key, &value)) {
> +			fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> +		}
> +		free(key);
> +
> +		if (fetch_recurse != RECURSE_SUBMODULES_NONE)
> +			/* local config overrules everything except commandline */
> +			return fetch_recurse;
> +	}
> +
> +	return spf->default_option;
> +}
> +
>  static int get_next_submodule(struct child_process *cp,
>  			      struct strbuf *err, void *data, void **task_cb)
>  {
> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,
>  			}
>  		}
>  
> -		default_argv = "yes";
> -		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> -			int fetch_recurse = RECURSE_SUBMODULES_NONE;
> -
> -			if (submodule) {
> -				char *key;
> -				const char *value;
> -
> -				fetch_recurse = submodule->fetch_recurse;
> -				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> -				if (!repo_config_get_string_const(the_repository, key, &value)) {
> -					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> -				}
> -				free(key);
> -			}
> -
> -			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
> -				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
> -					continue;
> -				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
> -									 submodule->name))
> -						continue;
> -					default_argv = "on-demand";
> -				}
> -			} else {
> -				if (spf->default_option == RECURSE_SUBMODULES_OFF)
> -					continue;
> -				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
> -									  submodule->name))
> -						continue;
> -					default_argv = "on-demand";
> -				}
> -			}
> -		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
> -			if (!unsorted_string_list_lookup(&changed_submodule_names,
> +		switch (get_fetch_recurse_config(submodule, spf))
> +		{
> +		default:
> +		case RECURSE_SUBMODULES_DEFAULT:
> +		case RECURSE_SUBMODULES_ON_DEMAND:
> +			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
>  							 submodule->name))
>  				continue;
>  			default_argv = "on-demand";
> +			break;
> +		case RECURSE_SUBMODULES_ON:
> +			default_argv = "yes";
> +			break;
> +		case RECURSE_SUBMODULES_OFF:
> +			continue;
>  		}
>  
>  		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
> -- 
> 2.14.1.145.gb3622a4
> 

-- 
Brandon Williams

Re: [PATCH v4 2/3] implement fetching of moved submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> but if we already have a submodule with that name (the most likely
>> explanation for its existence is because it started its life there
>> and then later moved), and the submodule is bound to a different
>> path, then that is a different submodule.  Skipping and warning both
>> are sensible thing to do.
>
> Skipping and warning is sensible once we decide to go this way.
>
> I propose to take a step back and not throw away the information
> whether the given string is a name or path, as then we do not have
> to warn&skip, but we can treat both correctly.

Now either one of us is utterly confused, and I suspect it is me, as
I do not see how "treat both correctly" could possibly work in the
case this code warns and skips.

At this point in the flow, we already know that it is not name,
because we asked and got a "Nah, there is no submodule registered in
.gitmodules at that path" from submodule_from_path().  Then we ask
submodule_from_name() if there is any submodule registered under the
name it would have got if it were added there, and we indeed find
one.  And that is definitely *not* a submodule we are looking for,
because if it were, its .path would have pointed at the path we were
using to ask in the first place.  The one we originally found at
path and are interested in finding out the details is not known to
.gitmodules, and the one under that name is not the one that we are
intereted in, so fetching from the repository the other one that
happens to have the same name but is different from the submodule we
are interested in would simply be wrong.

If we only have path without any .gitmodules entry (hence there is
not even URL), how would we proceed from that point on?

Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> On 10/16, Heiko Voigt wrote:
>> To make extending this logic later easier.
>
> This makes things so much clearer, thanks!

I agree that it is clear to see what the code after the patch does,
but the code before the patch is so convoluted to follow that it is
a bit hard to see if the code before and after are doing the same
thing, though ;-)

>
>> 
>> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
>> ---
>>  submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------
>>  1 file changed, 37 insertions(+), 37 deletions(-)
>> 
>> diff --git a/submodule.c b/submodule.c
>> index 71d1773e2e..82d206eb65 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {
>>  };
>>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
>>  
>> +static int get_fetch_recurse_config(const struct submodule *submodule,
>> +				    struct submodule_parallel_fetch *spf)
>> +{
>> +	if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
>> +		return spf->command_line_option;
>> +
>> +	if (submodule) {
>> +		char *key;
>> +		const char *value;
>> +
>> +		int fetch_recurse = submodule->fetch_recurse;
>> +		key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
>> +		if (!repo_config_get_string_const(the_repository, key, &value)) {
>> +			fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
>> +		}
>> +		free(key);
>> +
>> +		if (fetch_recurse != RECURSE_SUBMODULES_NONE)
>> +			/* local config overrules everything except commandline */
>> +			return fetch_recurse;
>> +	}
>> +
>> +	return spf->default_option;
>> +}
>> +
>>  static int get_next_submodule(struct child_process *cp,
>>  			      struct strbuf *err, void *data, void **task_cb)
>>  {
>> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,
>>  			}
>>  		}
>>  
>> -		default_argv = "yes";
>> -		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
>> -			int fetch_recurse = RECURSE_SUBMODULES_NONE;
>> -
>> -			if (submodule) {
>> -				char *key;
>> -				const char *value;
>> -
>> -				fetch_recurse = submodule->fetch_recurse;
>> -				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
>> -				if (!repo_config_get_string_const(the_repository, key, &value)) {
>> -					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
>> -				}
>> -				free(key);
>> -			}
>> -
>> -			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
>> -				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
>> -					continue;
>> -				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
>> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
>> -									 submodule->name))
>> -						continue;
>> -					default_argv = "on-demand";
>> -				}
>> -			} else {
>> -				if (spf->default_option == RECURSE_SUBMODULES_OFF)
>> -					continue;
>> -				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
>> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
>> -									  submodule->name))
>> -						continue;
>> -					default_argv = "on-demand";
>> -				}
>> -			}
>> -		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
>> -			if (!unsorted_string_list_lookup(&changed_submodule_names,
>> +		switch (get_fetch_recurse_config(submodule, spf))
>> +		{
>> +		default:
>> +		case RECURSE_SUBMODULES_DEFAULT:
>> +		case RECURSE_SUBMODULES_ON_DEMAND:
>> +			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
>>  							 submodule->name))
>>  				continue;
>>  			default_argv = "on-demand";
>> +			break;
>> +		case RECURSE_SUBMODULES_ON:
>> +			default_argv = "yes";
>> +			break;
>> +		case RECURSE_SUBMODULES_OFF:
>> +			continue;
>>  		}
>>  
>>  		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
>> -- 
>> 2.14.1.145.gb3622a4
>> 

Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Heiko Voigt]

On Thu, Oct 19, 2017 at 09:36:47AM +0900, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > On 10/16, Heiko Voigt wrote:
> >> To make extending this logic later easier.
> >
> > This makes things so much clearer, thanks!
> 
> I agree that it is clear to see what the code after the patch does,
> but the code before the patch is so convoluted to follow that it is
> a bit hard to see if the code before and after are doing the same
> thing, though ;-)

That is why I would appreciate some extra pairs of eyes on this :) I
tried to be as careful as possible when refactoring this, but since it
is quite convoluted something might have slipped through. The testsuite
does not show anything, but there might be corner cases that are not
tested I guess.

Will hopefully have time to look into the comments to the main patch of
this series tomorrow. Did not get around to properly do that yet.

Cheers Heiko

> >> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> >> ---
> >>  submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------
> >>  1 file changed, 37 insertions(+), 37 deletions(-)
> >> 
> >> diff --git a/submodule.c b/submodule.c
> >> index 71d1773e2e..82d206eb65 100644
> >> --- a/submodule.c
> >> +++ b/submodule.c
> >> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {
> >>  };
> >>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
> >>  
> >> +static int get_fetch_recurse_config(const struct submodule *submodule,
> >> +				    struct submodule_parallel_fetch *spf)
> >> +{
> >> +	if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
> >> +		return spf->command_line_option;
> >> +
> >> +	if (submodule) {
> >> +		char *key;
> >> +		const char *value;
> >> +
> >> +		int fetch_recurse = submodule->fetch_recurse;
> >> +		key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> >> +		if (!repo_config_get_string_const(the_repository, key, &value)) {
> >> +			fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> >> +		}
> >> +		free(key);
> >> +
> >> +		if (fetch_recurse != RECURSE_SUBMODULES_NONE)
> >> +			/* local config overrules everything except commandline */
> >> +			return fetch_recurse;
> >> +	}
> >> +
> >> +	return spf->default_option;
> >> +}
> >> +
> >>  static int get_next_submodule(struct child_process *cp,
> >>  			      struct strbuf *err, void *data, void **task_cb)
> >>  {
> >> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,
> >>  			}
> >>  		}
> >>  
> >> -		default_argv = "yes";
> >> -		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> >> -			int fetch_recurse = RECURSE_SUBMODULES_NONE;
> >> -
> >> -			if (submodule) {
> >> -				char *key;
> >> -				const char *value;
> >> -
> >> -				fetch_recurse = submodule->fetch_recurse;
> >> -				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> >> -				if (!repo_config_get_string_const(the_repository, key, &value)) {
> >> -					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> >> -				}
> >> -				free(key);
> >> -			}
> >> -
> >> -			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
> >> -				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
> >> -					continue;
> >> -				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
> >> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
> >> -									 submodule->name))
> >> -						continue;
> >> -					default_argv = "on-demand";
> >> -				}
> >> -			} else {
> >> -				if (spf->default_option == RECURSE_SUBMODULES_OFF)
> >> -					continue;
> >> -				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
> >> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
> >> -									  submodule->name))
> >> -						continue;
> >> -					default_argv = "on-demand";
> >> -				}
> >> -			}
> >> -		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
> >> -			if (!unsorted_string_list_lookup(&changed_submodule_names,
> >> +		switch (get_fetch_recurse_config(submodule, spf))
> >> +		{
> >> +		default:
> >> +		case RECURSE_SUBMODULES_DEFAULT:
> >> +		case RECURSE_SUBMODULES_ON_DEMAND:
> >> +			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
> >>  							 submodule->name))
> >>  				continue;
> >>  			default_argv = "on-demand";
> >> +			break;
> >> +		case RECURSE_SUBMODULES_ON:
> >> +			default_argv = "yes";
> >> +			break;
> >> +		case RECURSE_SUBMODULES_OFF:
> >> +			continue;
> >>  		}
> >>  
> >>  		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
> >> -- 
> >> 2.14.1.145.gb3622a4
> >> 

[PATCH 1/2] t5526: check for name/path collision in submodule fetch

[Stefan Beller]

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

This is just to test the corner case we're discussing.
Applies on top of origin/hv/fetch-moved-submodules-on-demand.


 t/t5526-fetch-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index a552ad4ead..c82d519e06 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -571,6 +571,7 @@ test_expect_success 'fetching submodule into a broken repository' '
 '
 
 test_expect_success "fetch new commits when submodule got renamed" '
+	test_when_finished "rm -rf downstream_rename" &&
 	git clone . downstream_rename &&
 	(
 		cd downstream_rename &&
@@ -605,4 +606,45 @@ test_expect_success "fetch new commits when submodule got renamed" '
 	test_cmp expect actual
 '
 
+test_expect_success "warn on submodule name/path clash, but new commits fetched in renamed" '
+	test_when_finished "rm -rf downstream_rename" &&
+	git clone . downstream_rename &&
+	(
+		cd downstream_rename &&
+		git submodule update --init &&
+# NEEDSWORK: we omitted --recursive for the submodule update here since
+# that does not work. See test 7001 for mv "moving nested submodules"
+# for details. Once that is fixed we should add the --recursive option
+# here.
+		git checkout -b rename &&
+		git mv submodule submodule_renamed &&
+		(
+			cd submodule_renamed &&
+			git checkout -b rename_sub &&
+			echo a >a &&
+			git add a &&
+			git commit -ma &&
+			git push origin rename_sub &&
+			git rev-parse HEAD >../../expect
+		) &&
+		git add submodule_renamed &&
+		git commit -m "update renamed submodule" &&
+		# produce collision, note that we use no submodule command
+		git clone ../submodule submodule &&
+		git add submodule &&
+		git commit -m "have new submodule at old path " &&
+		git push origin rename
+	) &&
+	(
+		cd downstream &&
+		git fetch --recurse-submodules=on-demand 2>err &&
+		grep "collides with a submodule named" err &&
+		(
+			cd submodule &&
+			git rev-parse origin/rename_sub >../../actual
+		)
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.14.0.rc0.3.g6c2e499285

[PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks

[Stefan Beller]

Currently when fetching we collect the names of submodules to be fetched
in a list. As we also want to support fetching 'gitlinks, that happen to
have a repo checked out at the right place', we'll just pretend that these
are submodules. We do that by assuming their path is their name. This in
turn can yield collisions between the name-namespace and the
path-namespace. (See the previous test for a demonstration.)

This patch rewrites the code such that we treat the 'real submodule' case
differently from the 'gitlink, but ok' case. This introduces a bit
of code duplication, but gets rid of the confusing mapping between names
and paths.

The test is incomplete as the long term vision is not achieved yet.
(which would be fetching both the renamed submodule as well as
the gitlink thing, putting them in place via e.g. git-pull)

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

 Heiko,
 Junio,

 I assumed the code would ease up a lot more, but now I am undecided if
 I want to keep arguing as the code is not stopping to be ugly. :)
 
 The idea is to treat submodule and gitlinks separately, with submodules
 supporting renames, and gitlinks as a historic artefact.
 
 Sorry for the noise about code ugliness.
 
 Thanks,
 Stefan
 

 submodule.c                 | 168 +++++++++++++++++++++-----------------------
 t/t5526-fetch-submodules.sh |   1 -
 2 files changed, 81 insertions(+), 88 deletions(-)

diff --git a/submodule.c b/submodule.c
index 82d206eb65..115df82f32 100644
--- a/submodule.c
+++ b/submodule.c
@@ -22,6 +22,7 @@
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
 static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
+static struct string_list changed_gitlink_paths = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -674,11 +675,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)
 }
 
 static struct oid_array *submodule_commits(struct string_list *submodules,
-					   const char *name)
+					   const char *key)
 {
 	struct string_list_item *item;
 
-	item = string_list_insert(submodules, name);
+	item = string_list_insert(submodules, key);
 	if (item->util)
 		return (struct oid_array *) item->util;
 
@@ -688,33 +689,20 @@ static struct oid_array *submodule_commits(struct string_list *submodules,
 }
 
 struct collect_changed_submodules_cb_data {
-	struct string_list *changed;
-	const struct object_id *commit_oid;
-};
+	/* used for submodules, supports renames: */
+	struct string_list *changed_by_name;
 
-/*
- * this would normally be two functions: default_name_from_path() and
- * path_from_default_name(). Since the default name is the same as
- * the submodule path we can get away with just one function which only
- * checks whether there is a submodule in the working directory at that
- * location.
- */
-static const char *default_name_or_path(const char *path_or_name)
-{
-	int error_code;
+	/* support old 'gitlink' with repo in-place, no rename support*/
+	struct string_list *changed_by_path;
 
-	if (!is_submodule_populated_gently(path_or_name, &error_code))
-		return NULL;
-
-	return path_or_name;
-}
+	const struct object_id *commit_oid;
+};
 
 static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 					  struct diff_options *options,
 					  void *data)
 {
 	struct collect_changed_submodules_cb_data *me = data;
-	struct string_list *changed = me->changed;
 	const struct object_id *commit_oid = me->commit_oid;
 	int i;
 
@@ -722,42 +710,35 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 		struct diff_filepair *p = q->queue[i];
 		struct oid_array *commits;
 		const struct submodule *submodule;
-		const char *name;
 
 		if (!S_ISGITLINK(p->two->mode))
 			continue;
 
 		submodule = submodule_from_path(commit_oid, p->two->path);
-		if (submodule)
-			name = submodule->name;
-		else {
-			name = default_name_or_path(p->two->path);
-			/* make sure name does not collide with existing one */
-			submodule = submodule_from_name(commit_oid, name);
-			if (submodule) {
-				warning("Submodule in commit %s at path: "
-					"'%s' collides with a submodule named "
-					"the same. Skipping it.",
-					oid_to_hex(commit_oid), name);
-				name = NULL;
-			}
+		if (submodule) {
+			commits = submodule_commits(me->changed_by_name, submodule->name);
+			oid_array_append(commits, &p->two->oid);
+		} else {
+			commits = submodule_commits(me->changed_by_path, p->two->path);
+			oid_array_append(commits, &p->two->oid);
 		}
-
-		if (!name)
-			continue;
-
-		commits = submodule_commits(changed, name);
-		oid_array_append(commits, &p->two->oid);
 	}
 }
 
 /*
- * Collect the paths of submodules in 'changed' which have changed based on
- * the revisions as specified in 'argv'.  Each entry in 'changed' will also
- * have a corresponding 'struct oid_array' (in the 'util' field) which lists
- * what the submodule pointers were updated to during the change.
+ * Collect the paths of submodules in 'changed_by_{name, path}' which have
+ * changed based on the revisions as specified in 'argv'.
+ *
+ * Each gitlink/submodule will occur in only one of the list. We'll prefer
+ * to give it by_name as that allows rename detection. We'll fall back to
+ * by_path to support gitlinks with no entry in '.gitmodules'.
+ *
+ * Each entry in 'changed_*' will also have a corresponding 'struct oid_array'
+ * (in the 'util' field) which lists what the submodule pointers were updated
+ * to during the change.
  */
-static void collect_changed_submodules(struct string_list *changed,
+static void collect_changed_submodules(struct string_list *changed_by_name,
+				       struct string_list *changed_by_path,
 				       struct argv_array *argv)
 {
 	struct rev_info rev;
@@ -771,7 +752,8 @@ static void collect_changed_submodules(struct string_list *changed,
 	while ((commit = get_revision(&rev))) {
 		struct rev_info diff_rev;
 		struct collect_changed_submodules_cb_data data;
-		data.changed = changed;
+		data.changed_by_name = changed_by_name;
+		data.changed_by_path = changed_by_path;
 		data.commit_oid = &commit->object.oid;
 
 		init_revisions(&diff_rev, NULL);
@@ -924,8 +906,9 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)
 int find_unpushed_submodules(struct oid_array *commits,
 		const char *remotes_name, struct string_list *needs_pushing)
 {
-	struct string_list submodules = STRING_LIST_INIT_DUP;
-	struct string_list_item *name;
+	struct string_list submodules_by_name = STRING_LIST_INIT_DUP;
+	struct string_list gitlinks_by_path = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 
 	/* argv.argv[0] will be ignored by setup_revisions */
@@ -934,27 +917,33 @@ int find_unpushed_submodules(struct oid_array *commits,
 	argv_array_push(&argv, "--not");
 	argv_array_pushf(&argv, "--remotes=%s", remotes_name);
 
-	collect_changed_submodules(&submodules, &argv);
+	collect_changed_submodules(&submodules_by_name, &gitlinks_by_path, &argv);
 
-	for_each_string_list_item(name, &submodules) {
-		struct oid_array *commits = name->util;
+	for_each_string_list_item(item, &submodules_by_name) {
+		struct oid_array *commits = item->util;
+		const char *name = item->string;
 		const struct submodule *submodule;
-		const char *path = NULL;
+		const char *path;
 
-		submodule = submodule_from_name(&null_oid, name->string);
-		if (submodule)
-			path = submodule->path;
-		else
-			path = default_name_or_path(name->string);
+		submodule = submodule_from_name(&null_oid, name);
+		if (!submodule)
+			BUG("submodule name/path mapping corrupt");
+		path = submodule->path;
 
-		if (!path)
-			continue;
+		if (submodule_needs_pushing(path, commits))
+			string_list_insert(needs_pushing, path);
+	}
+
+	for_each_string_list_item(item, &gitlinks_by_path) {
+		struct oid_array *commits = item->util;
+		const char *path = item->string;
 
 		if (submodule_needs_pushing(path, commits))
 			string_list_insert(needs_pushing, path);
 	}
 
-	free_submodules_oids(&submodules);
+	free_submodules_oids(&submodules_by_name);
+	free_submodules_oids(&gitlinks_by_path);
 	argv_array_clear(&argv);
 
 	return needs_pushing->nr;
@@ -1106,7 +1095,8 @@ static void calculate_changed_submodule_paths(void)
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
 	struct string_list changed_submodules = STRING_LIST_INIT_DUP;
-	const struct string_list_item *name;
+	struct string_list changed_gitlinks = STRING_LIST_INIT_DUP;
+	const struct string_list_item *item;
 
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
@@ -1123,27 +1113,32 @@ static void calculate_changed_submodule_paths(void)
 	 * Collect all submodules (whether checked out or not) for which new
 	 * commits have been recorded upstream in "changed_submodule_names".
 	 */
-	collect_changed_submodules(&changed_submodules, &argv);
+	collect_changed_submodules(&changed_submodules, &changed_gitlinks, &argv);
 
-	for_each_string_list_item(name, &changed_submodules) {
-		struct oid_array *commits = name->util;
-		const struct submodule *submodule;
-		const char *path = NULL;
+	for_each_string_list_item(item, &changed_submodules) {
+		struct oid_array *commits = item->util;
+		const char *name = item->string;
+		const struct submodule *sub =
+			submodule_from_name(&null_oid, name);
 
-		submodule = submodule_from_name(&null_oid, name->string);
-		if (submodule)
-			path = submodule->path;
-		else
-			path = default_name_or_path(name->string);
+		if (!sub)
+			BUG("cannot lookup submodule, but we could before?");
 
-		if (!path)
-			continue;
+		if (!submodule_has_commits(sub->path, commits))
+			string_list_append(&changed_submodule_names, name);
+	}
+
+	/* the same for gitnlinks, stored in 'changed_gitlink_paths' */
+	for_each_string_list_item(item, &changed_gitlinks) {
+		const char *path = item->string;
+		struct oid_array *commits = item->util;
 
 		if (!submodule_has_commits(path, commits))
-			string_list_append(&changed_submodule_names, name->string);
+			string_list_append(&changed_gitlink_paths, path);
 	}
 
 	free_submodules_oids(&changed_submodules);
+	free_submodules_oids(&changed_gitlinks);
 	argv_array_clear(&argv);
 	oid_array_clear(&ref_tips_before_fetch);
 	oid_array_clear(&ref_tips_after_fetch);
@@ -1154,6 +1149,7 @@ int submodule_touches_in_range(struct object_id *excl_oid,
 			       struct object_id *incl_oid)
 {
 	struct string_list subs = STRING_LIST_INIT_DUP;
+	struct string_list gitlinks = STRING_LIST_INIT_DUP;
 	struct argv_array args = ARGV_ARRAY_INIT;
 	int ret;
 
@@ -1166,8 +1162,8 @@ int submodule_touches_in_range(struct object_id *excl_oid,
 	argv_array_push(&args, "--not");
 	argv_array_push(&args, oid_to_hex(excl_oid));
 
-	collect_changed_submodules(&subs, &args);
-	ret = subs.nr;
+	collect_changed_submodules(&subs, &gitlinks, &args);
+	ret = subs.nr + gitlinks.nr;
 
 	argv_array_clear(&args);
 
@@ -1225,27 +1221,25 @@ static int get_next_submodule(struct child_process *cp,
 		const struct cache_entry *ce = active_cache[spf->count];
 		const char *git_dir, *default_argv;
 		const struct submodule *submodule;
-		struct submodule default_submodule = SUBMODULE_INIT;
+		int found = 0;
 
 		if (!S_ISGITLINK(ce->ce_mode))
 			continue;
 
 		submodule = submodule_from_path(&null_oid, ce->name);
-		if (!submodule) {
-			const char *name = default_name_or_path(ce->name);
-			if (name) {
-				default_submodule.path = default_submodule.name = name;
-				submodule = &default_submodule;
-			}
-		}
 
 		switch (get_fetch_recurse_config(submodule, spf))
 		{
 		default:
 		case RECURSE_SUBMODULES_DEFAULT:
 		case RECURSE_SUBMODULES_ON_DEMAND:
-			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
-							 submodule->name))
+
+			if (submodule)
+				found |= !!unsorted_string_list_lookup(&changed_submodule_names, submodule->name);
+
+			found |= !!unsorted_string_list_lookup(&changed_gitlink_paths, ce->name);
+
+			if (!found)
 				continue;
 			default_argv = "on-demand";
 			break;
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index c82d519e06..d6a6d6a4e1 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -638,7 +638,6 @@ test_expect_success "warn on submodule name/path clash, but new commits fetched
 	(
 		cd downstream &&
 		git fetch --recurse-submodules=on-demand 2>err &&
-		grep "collides with a submodule named" err &&
 		(
 			cd submodule &&
 			git rev-parse origin/rename_sub >../../actual
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch

[Brandon Williams]

On 10/19, Heiko Voigt wrote:
> On Thu, Oct 19, 2017 at 09:36:47AM +0900, Junio C Hamano wrote:
> > Brandon Williams <bmwill@google.com> writes:
> > 
> > > On 10/16, Heiko Voigt wrote:
> > >> To make extending this logic later easier.
> > >
> > > This makes things so much clearer, thanks!
> > 
> > I agree that it is clear to see what the code after the patch does,
> > but the code before the patch is so convoluted to follow that it is
> > a bit hard to see if the code before and after are doing the same
> > thing, though ;-)
> 
> That is why I would appreciate some extra pairs of eyes on this :) I
> tried to be as careful as possible when refactoring this, but since it
> is quite convoluted something might have slipped through. The testsuite
> does not show anything, but there might be corner cases that are not
> tested I guess.
> 
> Will hopefully have time to look into the comments to the main patch of
> this series tomorrow. Did not get around to properly do that yet.
> 
> Cheers Heiko
> 
> > >> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> > >> ---
> > >>  submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------
> > >>  1 file changed, 37 insertions(+), 37 deletions(-)
> > >> 
> > >> diff --git a/submodule.c b/submodule.c
> > >> index 71d1773e2e..82d206eb65 100644
> > >> --- a/submodule.c
> > >> +++ b/submodule.c
> > >> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {
> > >>  };
> > >>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
> > >>  
> > >> +static int get_fetch_recurse_config(const struct submodule *submodule,
> > >> +				    struct submodule_parallel_fetch *spf)
> > >> +{
> > >> +	if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
> > >> +		return spf->command_line_option;
> > >> +
> > >> +	if (submodule) {
> > >> +		char *key;
> > >> +		const char *value;
> > >> +
> > >> +		int fetch_recurse = submodule->fetch_recurse;
> > >> +		key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> > >> +		if (!repo_config_get_string_const(the_repository, key, &value)) {
> > >> +			fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> > >> +		}
> > >> +		free(key);
> > >> +
> > >> +		if (fetch_recurse != RECURSE_SUBMODULES_NONE)
> > >> +			/* local config overrules everything except commandline */
> > >> +			return fetch_recurse;
> > >> +	}
> > >> +
> > >> +	return spf->default_option;
> > >> +}
> > >> +
> > >>  static int get_next_submodule(struct child_process *cp,
> > >>  			      struct strbuf *err, void *data, void **task_cb)
> > >>  {
> > >> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,
> > >>  			}
> > >>  		}
> > >>  
> > >> -		default_argv = "yes";
> > >> -		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> > >> -			int fetch_recurse = RECURSE_SUBMODULES_NONE;
> > >> -
> > >> -			if (submodule) {
> > >> -				char *key;
> > >> -				const char *value;
> > >> -
> > >> -				fetch_recurse = submodule->fetch_recurse;
> > >> -				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> > >> -				if (!repo_config_get_string_const(the_repository, key, &value)) {
> > >> -					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> > >> -				}
> > >> -				free(key);
> > >> -			}
> > >> -
> > >> -			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
> > >> -				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
> > >> -					continue;
> > >> -				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
> > >> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
> > >> -									 submodule->name))
> > >> -						continue;
> > >> -					default_argv = "on-demand";
> > >> -				}
> > >> -			} else {
> > >> -				if (spf->default_option == RECURSE_SUBMODULES_OFF)
> > >> -					continue;
> > >> -				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
> > >> -					if (!unsorted_string_list_lookup(&changed_submodule_names,
> > >> -									  submodule->name))
> > >> -						continue;
> > >> -					default_argv = "on-demand";
> > >> -				}
> > >> -			}
> > >> -		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
> > >> -			if (!unsorted_string_list_lookup(&changed_submodule_names,
> > >> +		switch (get_fetch_recurse_config(submodule, spf))
> > >> +		{

I looked through this one more time and I was able to convince myself
again that it's doing the same thing.  Instead of repeating the same
logic over and over again (via copy and paste of code) in deeply nested
if's, you are first determining what the value of fetch_recurse is and
then based on that doing a set of specific things.

Only nit would be to move this brace onto the previous line :)

> > >> +		default:
> > >> +		case RECURSE_SUBMODULES_DEFAULT:
> > >> +		case RECURSE_SUBMODULES_ON_DEMAND:
> > >> +			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
> > >>  							 submodule->name))
> > >>  				continue;
> > >>  			default_argv = "on-demand";
> > >> +			break;
> > >> +		case RECURSE_SUBMODULES_ON:
> > >> +			default_argv = "yes";
> > >> +			break;
> > >> +		case RECURSE_SUBMODULES_OFF:
> > >> +			continue;
> > >>  		}
> > >>  
> > >>  		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
> > >> -- 
> > >> 2.14.1.145.gb3622a4
> > >> 

-- 
Brandon Williams

Re: [PATCH v4 2/3] implement fetching of moved submodules

[Stefan Beller]

On Wed, Oct 18, 2017 at 5:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> but if we already have a submodule with that name (the most likely
>>> explanation for its existence is because it started its life there
>>> and then later moved), and the submodule is bound to a different
>>> path, then that is a different submodule.  Skipping and warning both
>>> are sensible thing to do.
>>
>> Skipping and warning is sensible once we decide to go this way.
>>
>> I propose to take a step back and not throw away the information
>> whether the given string is a name or path, as then we do not have
>> to warn&skip, but we can treat both correctly.
>
> Now either one of us is utterly confused, and I suspect it is me, as
> I do not see how "treat both correctly" could possibly work in the
> case this code warns and skips.
>
> At this point in the flow, we already know that it is not name,
> because we asked and got a "Nah, there is no submodule registered in
> .gitmodules at that path" from submodule_from_path().  Then we ask
> submodule_from_name() if there is any submodule registered under the
> name it would have got if it were added there, and we indeed find
> one.  And that is definitely *not* a submodule we are looking for,
> because if it were, its .path would have pointed at the path we were
> using to ask in the first place.  The one we originally found at
> path and are interested in finding out the details is not known to
> .gitmodules, and the one under that name is not the one that we are
> intereted in, so fetching from the repository the other one that
> happens to have the same name but is different from the submodule we
> are interested in would simply be wrong.

Eventually we'd want to also init new submodules on fetch
(if you use submodule.active to specify the interesting submodules),
and in that case I would imagine to fetch both submodules.

As I wrote the code to further improve this series,
I realized that this is maybe "good enough" for now,
so assume that I have reviewed this series and found it good.

> If we only have path without any .gitmodules entry (hence there is
> not even URL), how would we proceed from that point on?

Oh well, right. We can only offer to keep the existing behavior
which means supporting existing repos in place at that path.

Stefan

>

Re: [PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks

[Heiko Voigt]

On Thu, Oct 19, 2017 at 11:11:09AM -0700, Stefan Beller wrote:
> Currently when fetching we collect the names of submodules to be fetched
> in a list. As we also want to support fetching 'gitlinks, that happen to
> have a repo checked out at the right place', we'll just pretend that these
> are submodules. We do that by assuming their path is their name. This in
> turn can yield collisions between the name-namespace and the
> path-namespace. (See the previous test for a demonstration.)
> 
> This patch rewrites the code such that we treat the 'real submodule' case
> differently from the 'gitlink, but ok' case. This introduces a bit
> of code duplication, but gets rid of the confusing mapping between names
> and paths.
> 
> The test is incomplete as the long term vision is not achieved yet.
> (which would be fetching both the renamed submodule as well as
> the gitlink thing, putting them in place via e.g. git-pull)
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>  Heiko,
>  Junio,
> 
>  I assumed the code would ease up a lot more, but now I am undecided if
>  I want to keep arguing as the code is not stopping to be ugly. :)

So we are basically coming to the same conclusion? :) My previous
fallback approach basically did the same but with the old architecture
(without parallel fetch, ...) and was already ugly.

With the fallback on submodule default names approach we can keep most
of the old functionality and keep the code that handles that minimal.

Since there is only a small (IMO quite unlikely) cornercase that could
break peoples expectations I would like to have a look whether anyone
even notices the behavioral change on next or master. If there are
complaints we can still extend and add the two lists.

>  The idea is to treat submodule and gitlinks separately, with submodules
>  supporting renames, and gitlinks as a historic artefact.
>  
>  Sorry for the noise about code ugliness.

Why sorry? For me it is actually interesting to see you basically coming
to the same conclusions.

Cheers Heiko

Re: [PATCH 1/2] t5526: check for name/path collision in submodule fetch

[Heiko Voigt]

On Thu, Oct 19, 2017 at 11:11:08AM -0700, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> This is just to test the corner case we're discussing.
> Applies on top of origin/hv/fetch-moved-submodules-on-demand.
> 
> 
>  t/t5526-fetch-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index a552ad4ead..c82d519e06 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -571,6 +571,7 @@ test_expect_success 'fetching submodule into a broken repository' '
>  '
>  
>  test_expect_success "fetch new commits when submodule got renamed" '
> +	test_when_finished "rm -rf downstream_rename" &&
>  	git clone . downstream_rename &&
>  	(
>  		cd downstream_rename &&
> @@ -605,4 +606,45 @@ test_expect_success "fetch new commits when submodule got renamed" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success "warn on submodule name/path clash, but new commits fetched in renamed" '
> +	test_when_finished "rm -rf downstream_rename" &&
> +	git clone . downstream_rename &&
> +	(
> +		cd downstream_rename &&
> +		git submodule update --init &&
> +# NEEDSWORK: we omitted --recursive for the submodule update here since
> +# that does not work. See test 7001 for mv "moving nested submodules"
> +# for details. Once that is fixed we should add the --recursive option
> +# here.
> +		git checkout -b rename &&
> +		git mv submodule submodule_renamed &&
> +		(
> +			cd submodule_renamed &&
> +			git checkout -b rename_sub &&
> +			echo a >a &&
> +			git add a &&
> +			git commit -ma &&
> +			git push origin rename_sub &&
> +			git rev-parse HEAD >../../expect
> +		) &&
> +		git add submodule_renamed &&
> +		git commit -m "update renamed submodule" &&
> +		# produce collision, note that we use no submodule command
> +		git clone ../submodule submodule &&
> +		git add submodule &&

A small note even though this is not meant for inclusion: This would
break when I start working on teaching 'git add' to set default values
in .gitmodules when available.

But I guess I will discover a few other places, when starting that, that
will break in the tests anyway.

Cheers Heiko

Re: [PATCH 1/2] t5526: check for name/path collision in submodule fetch

[Stefan Beller]

>> +             git add submodule &&
>
> A small note even though this is not meant for inclusion: This would
> break when I start working on teaching 'git add' to set default values
> in .gitmodules when available.

Yes. I should have used something like:

    git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub

as that is the real plumbing command to change a gitlink.

>
> But I guess I will discover a few other places, when starting that, that
> will break in the tests anyway.

Yes, I have the same suspicion. Thanks for reminding
me to use more plumbing!

Stefan

Re: [PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks

[Stefan Beller]

On Mon, Oct 23, 2017 at 7:12 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Thu, Oct 19, 2017 at 11:11:09AM -0700, Stefan Beller wrote:
>> Currently when fetching we collect the names of submodules to be fetched
>> in a list. As we also want to support fetching 'gitlinks, that happen to
>> have a repo checked out at the right place', we'll just pretend that these
>> are submodules. We do that by assuming their path is their name. This in
>> turn can yield collisions between the name-namespace and the
>> path-namespace. (See the previous test for a demonstration.)
>>
>> This patch rewrites the code such that we treat the 'real submodule' case
>> differently from the 'gitlink, but ok' case. This introduces a bit
>> of code duplication, but gets rid of the confusing mapping between names
>> and paths.
>>
>> The test is incomplete as the long term vision is not achieved yet.
>> (which would be fetching both the renamed submodule as well as
>> the gitlink thing, putting them in place via e.g. git-pull)
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>>  Heiko,
>>  Junio,
>>
>>  I assumed the code would ease up a lot more, but now I am undecided if
>>  I want to keep arguing as the code is not stopping to be ugly. :)
>
> So we are basically coming to the same conclusion? :) My previous
> fallback approach basically did the same but with the old architecture
> (without parallel fetch, ...) and was already ugly.

It depends on the conclusion you drew. ;)
Here is my conclusion:
* It would really be nice to have this fallback separated out.
* However for the current state the ugliness of such code trumps the
  more maintainable, long term oriented thing with path/names not
  clashing. I could not spend more time polishing these patches,
  so I could not ask you to do it either
-> I think your patches are fine as is for inclusion
-> We may have #leftoverbits here to clear up the confusion around
  path/names, as well as making the code more pleasant to read.

> With the fallback on submodule default names approach we can keep most
> of the old functionality and keep the code that handles that minimal.
>
> Since there is only a small (IMO quite unlikely) cornercase that could
> break peoples expectations I would like to have a look whether anyone
> even notices the behavioral change on next or master. If there are
> complaints we can still extend and add the two lists.

That sounds good to me.

>
>>  The idea is to treat submodule and gitlinks separately, with submodules
>>  supporting renames, and gitlinks as a historic artefact.
>>
>>  Sorry for the noise about code ugliness.
>
> Why sorry? For me it is actually interesting to see you basically coming
> to the same conclusions.

I thought I might come off awkwardly criticizing code for ugliness without
having a better alternative to show.

Thanks for working on this,
Stefan

>
> Cheers Heiko

Re: [PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> Why sorry? For me it is actually interesting to see you basically coming
> to the same conclusions.

I find it also assuring to see that two people not constantly
working together closely come to the same conclusion.  Thanks.