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

From: Stefan Beller <[email-masked]>
Date: Thu, 19 Oct 2017 11:11:09 -0700

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 <[email-masked]>
---

 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