Merge branch 'ar/submodule-gitdir-tweak' into seen

Avoid local submodule repository directory paths overlapping with
each other by encoding submodule names before using them as path
components.

* ar/submodule-gitdir-tweak:
  submodule: fix case-folding gitdir filesystem colisions
  submodule: add extension to encode gitdir paths
  builtin/credential-store: move is_rfc3986_unreserved to url.[ch]
  submodule--helper: use submodule_name_to_gitdir in add_submodule

Author: gitster

Documentation/config/extensions.adoc

@@ -73,6 +73,12 @@ relativeWorktrees:::
 	repaired with either the `--relative-paths` option or with the
 	`worktree.useRelativePaths` config set to `true`.
 
+submoduleEncoding:::
+	If enabled, submodule gitdir paths are encoded to avoid filesystem
+	conflicts due to nested gitdirs, case insensitivity or other issues
+	When enabled, the submodule.<name>.gitdir config is always set for
+	all submodulesand is the single point of authority for gitdir paths.
+
 worktreeConfig:::
 	If enabled, then worktrees will load config settings from the
 	`$GIT_DIR/config.worktree` file in addition to the

Documentation/config/submodule.adoc

@@ -52,6 +52,11 @@ submodule.<name>.active::
 	submodule.active config option. See linkgit:gitsubmodules[7] for
 	details.
 
+submodule.<name>.gitdir::
+	This option sets the gitdir path for submodule <name>, allowing users to
+	override the default path. Only works when `extensions.submoduleEncoding`
+	is enabled, otherwise does nothing. See linkgit:git-config[1] for details.
+
 submodule.active::
 	A repeated field which contains a pathspec used to match against a
 	submodule's path to determine if the submodule is of interest to git

builtin/credential-store.c

@@ -7,6 +7,7 @@
 #include "path.h"
 #include "string-list.h"
 #include "parse-options.h"
+#include "url.h"
 #include "write-or-die.h"
 
 static struct lock_file credential_lock;
@@ -76,12 +77,6 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to write credential store");
 }
 
-static int is_rfc3986_unreserved(char ch)
-{
-	return isalnum(ch) ||
-		ch == '-' || ch == '_' || ch == '.' || ch == '~';
-}
-
 static int is_rfc3986_reserved_or_unreserved(char ch)
 {
 	if (is_rfc3986_unreserved(ch))

builtin/submodule--helper.c

@@ -1204,6 +1204,22 @@ static int module_summary(int argc, const char **argv, const char *prefix,
 	return ret;
 }
 
+static int module_gitdir(int argc, const char **argv, const char *prefix UNUSED,
+			 struct repository *repo)
+{
+	struct strbuf gitdir = STRBUF_INIT;
+
+	if (argc != 2)
+		usage(_("git submodule--helper gitdir <name>"));
+
+	submodule_name_to_gitdir(&gitdir, repo, argv[1]);
+
+	printf("%s\n", gitdir.buf);
+
+	strbuf_release(&gitdir);
+	return 0;
+}
+
 struct sync_cb {
 	const char *prefix;
 	const char *super_prefix;
@@ -3183,13 +3199,13 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
 
 static int add_submodule(const struct add_data *add_data)
 {
-	char *submod_gitdir_path;
 	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
 	struct string_list reference = STRING_LIST_INIT_NODUP;
 	int ret = -1;
 
 	/* perhaps the path already exists and is already a git repo, else clone it */
 	if (is_directory(add_data->sm_path)) {
+		char *submod_gitdir_path;
 		struct strbuf sm_path = STRBUF_INIT;
 		strbuf_addstr(&sm_path, add_data->sm_path);
 		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
@@ -3203,10 +3219,11 @@ static int add_submodule(const struct add_data *add_data)
 		free(submod_gitdir_path);
 	} else {
 		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf submod_gitdir = STRBUF_INIT;
 
-		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+		submodule_name_to_gitdir(&submod_gitdir, the_repository, add_data->sm_name);
 
-		if (is_directory(submod_gitdir_path)) {
+		if (is_directory(submod_gitdir.buf)) {
 			if (!add_data->force) {
 				struct strbuf msg = STRBUF_INIT;
 				char *die_msg;
@@ -3215,8 +3232,8 @@ static int add_submodule(const struct add_data *add_data)
 						    "locally with remote(s):\n"),
 					    add_data->sm_name);
 
-				append_fetch_remotes(&msg, submod_gitdir_path);
-				free(submod_gitdir_path);
+				append_fetch_remotes(&msg, submod_gitdir.buf);
+				strbuf_release(&submod_gitdir);
 
 				strbuf_addf(&msg, _("If you want to reuse this local git "
 						    "directory instead of cloning again from\n"
@@ -3234,7 +3251,7 @@ static int add_submodule(const struct add_data *add_data)
 					 "submodule '%s'\n"), add_data->sm_name);
 			}
 		}
-		free(submod_gitdir_path);
+		strbuf_release(&submod_gitdir);
 
 		clone_data.prefix = add_data->prefix;
 		clone_data.path = add_data->sm_path;
@@ -3586,6 +3603,7 @@ int cmd_submodule__helper(int argc,
 		NULL
 	};
 	struct option options[] = {
+		OPT_SUBCOMMAND("gitdir", &fn, module_gitdir),
 		OPT_SUBCOMMAND("clone", &fn, module_clone),
 		OPT_SUBCOMMAND("add", &fn, module_add),
 		OPT_SUBCOMMAND("update", &fn, module_update),

repository.c

@@ -288,6 +288,7 @@ int repo_init(struct repository *repo,
 	repo->repository_format_worktree_config = format.worktree_config;
 	repo->repository_format_relative_worktrees = format.relative_worktrees;
 	repo->repository_format_precious_objects = format.precious_objects;
+	repo->repository_format_submodule_encoding = format.submodule_encoding;
 
 	/* take ownership of format.partial_clone */
 	repo->repository_format_partial_clone = format.partial_clone;

repository.h

@@ -158,6 +158,7 @@ struct repository {
 	int repository_format_worktree_config;
 	int repository_format_relative_worktrees;
 	int repository_format_precious_objects;
+	int repository_format_submodule_encoding;
 
 	/* Indicate if a repository has a different 'commondir' from 'gitdir' */
 	unsigned different_commondir:1;

setup.c

@@ -687,6 +687,9 @@ static enum extension_result handle_extension(const char *var,
 	} else if (!strcmp(ext, "relativeworktrees")) {
 		data->relative_worktrees = git_config_bool(var, value);
 		return EXTENSION_OK;
+	} else if (!strcmp(ext, "submoduleencoding")) {
+		data->submodule_encoding = git_config_bool(var, value);
+		return EXTENSION_OK;
 	}
 	return EXTENSION_UNKNOWN;
 }
@@ -1865,6 +1868,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				repo_fmt.worktree_config;
 			the_repository->repository_format_relative_worktrees =
 				repo_fmt.relative_worktrees;
+			the_repository->repository_format_submodule_encoding =
+				repo_fmt.submodule_encoding;
 			/* take ownership of repo_fmt.partial_clone */
 			the_repository->repository_format_partial_clone =
 				repo_fmt.partial_clone;
@@ -1963,6 +1968,8 @@ void check_repository_format(struct repository_format *fmt)
 				    fmt->ref_storage_format);
 	the_repository->repository_format_worktree_config =
 		fmt->worktree_config;
+	the_repository->repository_format_submodule_encoding =
+		fmt->submodule_encoding;
 	the_repository->repository_format_relative_worktrees =
 		fmt->relative_worktrees;
 	the_repository->repository_format_partial_clone =

setup.h

@@ -130,6 +130,7 @@ struct repository_format {
 	char *partial_clone; /* value of extensions.partialclone */
 	int worktree_config;
 	int relative_worktrees;
+	int submodule_encoding;
 	int is_bare;
 	int hash_algo;
 	int compat_hash_algo;

submodule.c

@@ -31,6 +31,7 @@
 #include "commit-reach.h"
 #include "read-cache-ll.h"
 #include "setup.h"
+#include "url.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
 static int initialized_fetch_ref_tips;
@@ -2250,16 +2251,30 @@ int submodule_move_head(const char *path, const char *super_prefix,
 	return ret;
 }
 
+/*
+ * Find the last submodule name in the gitdir path (modules can be nested).
+ * Returns a pointer into `path` to the beginning of the name or NULL if not found.
+ */
+static char *find_last_submodule_name(char *git_dir_path)
+{
+	const char *modules_marker = "/modules/";
+	char *p = git_dir_path;
+	char *last = NULL;
+
+	while ((p = strstr(p, modules_marker))) {
+		last = p + strlen(modules_marker);
+		p++;
+	}
+
+	return last;
+}
+
 int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
 {
 	size_t len = strlen(git_dir), suffix_len = strlen(submodule_name);
-	char *p;
-	int ret = 0;
-
-	if (len <= suffix_len || (p = git_dir + len - suffix_len)[-1] != '/' ||
-	    strcmp(p, submodule_name))
-		BUG("submodule name '%s' not a suffix of git dir '%s'",
-		    submodule_name, git_dir);
+	char *p = git_dir + len - suffix_len;
+	bool suffixes_match = !strcmp(p, submodule_name);
+	int ret = 0, config_ignorecase = 0;
 
 	/*
 	 * We prevent the contents of sibling submodules' git directories to
@@ -2271,7 +2286,7 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
 	 * but the latter directory is already designated to contain the hooks
 	 * of the former.
 	 */
-	for (; *p; p++) {
+	for (; *p && suffixes_match; p++) {
 		if (is_dir_sep(*p)) {
 			char c = *p;
 
@@ -2288,6 +2303,51 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
 		}
 	}
 
+	/* tests after this check are only for encoded names, when the extension is enabled */
+	if (!the_repository->repository_format_submodule_encoding)
+		return 0;
+
+	/* Prevent the use of '/' in names */
+	p = find_last_submodule_name(git_dir);
+	if (p && strchr(p, '/') != NULL)
+		return error("submodule gitdir name '%s' contains unexpected '/'", p);
+
+	/* Prevent conflicts on case-folding filesystems */
+	repo_config_get_bool(the_repository, "core.ignorecase", &config_ignorecase);
+	if (ignore_case || config_ignorecase) {
+		char *lower_gitdir = xstrdup(git_dir);
+		char *module_name = find_last_submodule_name(lower_gitdir);
+
+		if (module_name) {
+			for (p = module_name; *p; p++)
+				*p = tolower(*p);
+
+			/*
+			 * If lower path is different and already exists, check for collision.
+			 * Intentionally double-check to eliminate false-positives.
+			 */
+			if (strcmp(lower_gitdir, git_dir) && is_git_directory(lower_gitdir)) {
+				char *canonical = real_pathdup(git_dir, 0);
+				if (canonical) {
+					struct strbuf norm_git_dir = STRBUF_INIT;
+					strbuf_addstr(&norm_git_dir, git_dir);
+					strbuf_normalize_path(&norm_git_dir);
+
+					if (strcmp(canonical, norm_git_dir.buf))
+						ret = error(_("submodule git dir '%s' "
+							      "collides with '%s'"),
+							    canonical, norm_git_dir.buf);
+
+					strbuf_release(&norm_git_dir);
+					FREE_AND_NULL(canonical);
+				}
+			}
+		}
+
+		FREE_AND_NULL(lower_gitdir);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -2575,29 +2635,70 @@ int submodule_to_gitdir(struct repository *repo,
 	return ret;
 }
 
+static int validate_and_set_submodule_gitdir(struct strbuf *gitdir_path,
+					     const char *submodule_name)
+{
+	char *key;
+
+	if (validate_submodule_git_dir(gitdir_path->buf, submodule_name))
+		return -1;
+
+	key = xstrfmt("submodule.%s.gitdir", submodule_name);
+	repo_config_set_gently(the_repository, key, gitdir_path->buf);
+	FREE_AND_NULL(key);
+
+	return 0;
+
+}
+
 void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,
 			      const char *submodule_name)
 {
+	const char *gitdir;
+	char *key;
+
+	repo_git_path_append(r, buf, "modules/");
+	strbuf_addstr(buf, submodule_name);
+
+	/* If extensions.submoduleEncoding is disabled, use the plain path set above */
+	if (!r->repository_format_submodule_encoding)
+		return;
+
+	/* Extension is enabled: use the gitdir config if it exists */
+	key = xstrfmt("submodule.%s.gitdir", submodule_name);
+	if (!repo_config_get_string_tmp(r, key, &gitdir)) {
+		strbuf_reset(buf);
+		strbuf_addstr(buf, gitdir);
+		FREE_AND_NULL(key);
+		return;
+	}
+	FREE_AND_NULL(key);
+
 	/*
-	 * NEEDSWORK: The current way of mapping a submodule's name to
-	 * its location in .git/modules/ has problems with some naming
-	 * schemes. For example, if a submodule is named "foo" and
-	 * another is named "foo/bar" (whether present in the same
-	 * superproject commit or not - the problem will arise if both
-	 * superproject commits have been checked out at any point in
-	 * time), or if two submodule names only have different cases in
-	 * a case-insensitive filesystem.
-	 *
-	 * There are several solutions, including encoding the path in
-	 * some way, introducing a submodule.<name>.gitdir config in
-	 * .git/config (not .gitmodules) that allows overriding what the
-	 * gitdir of a submodule would be (and teach Git, upon noticing
-	 * a clash, to automatically determine a non-clashing name and
-	 * to write such a config), or introducing a
-	 * submodule.<name>.gitdir config in .gitmodules that repo
-	 * administrators can explicitly set. Nothing has been decided,
-	 * so for now, just append the name at the end of the path.
+	 * The gitdir config does not exist, even though the extension is enabled.
+	 * Therefore we are in one of the following cases:
 	 */
+
+	/* Case 1: legacy migration of valid plain submodule names */
+	if (!validate_and_set_submodule_gitdir(buf, submodule_name))
+		return;
+
+	/* Case 2.1: Try URI-safe (RFC3986) encoding first, this fixes nested gitdirs */
+	strbuf_reset(buf);
 	repo_git_path_append(r, buf, "modules/");
-	strbuf_addstr(buf, submodule_name);
+	strbuf_addstr_urlencode(buf, submodule_name, is_rfc3986_unreserved);
+	if (!validate_and_set_submodule_gitdir(buf, submodule_name))
+		return;
+
+	/* Case 2.2: Try extended uppercase URI (RFC3986) encoding, to fix case-folding */
+	strbuf_reset(buf);
+	repo_git_path_append(r, buf, "modules/");
+	strbuf_addstr_urlencode(buf, submodule_name, is_casefolding_rfc3986_unreserved);
+	if (!validate_and_set_submodule_gitdir(buf, submodule_name))
+		return;
+
+	/* Case 3: error out */
+	die(_("Cannot construct a valid gitdir path for submodule '%s': "
+	      "please set a unique git config for 'submodule.%s.gitdir'."),
+	    submodule_name, submodule_name);
 }

t/lib-verify-submodule-gitdir-path.sh

@@ -0,0 +1,24 @@
+# Helper to verify if repo $1 contains a submodule named $2 with gitdir path $3
+
+# This does not check filesystem existence. That is done in submodule.c via the
+# submodule_name_to_gitdir() API which this helper ends up calling. The gitdirs
+# might or might not exist (e.g. when adding a new submodule), so this only
+# checks the expected configuration path, which might be overridden by the user.
+
+verify_submodule_gitdir_path() {
+	repo="$1" &&
+	name="$2" &&
+	path="$3" &&
+	(
+		cd "$repo" &&
+		# Compute expected absolute path
+		expected="$(git rev-parse --git-common-dir)/$path" &&
+		expected="$(test-tool path-utils real_path "$expected")" &&
+		# Compute actual absolute path
+		actual="$(git submodule--helper gitdir "$name")" &&
+		actual="$(test-tool path-utils real_path "$actual")" &&
+		echo "$expected" >expect &&
+		echo "$actual" >actual &&
+		test_cmp expect actual
+	)
+}