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

gitster · gitster · commit c799166942fd · 2025-10-08T12:40:24.000-07:00
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: error out if gitdir name is too long
  submodule: encode gitdir paths to avoid conflicts
  strbuf: bring back is_rfc3986_unreserved
  submodule: add gitdir path config override
  submodule--helper: use submodule_name_to_gitdir in add_submodule
diff --git a/Documentation/config/extensions.adoc b/Documentation/config/extensions.adoc
@@ -73,6 +73,15 @@ 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 or case insensitivity. For now, only
+	url-encoding (rfc3986) is available, with a small addition to encode
+	uppercase to lowercase letters (`A  -> _a`, `B -> _b` and so on).
+	Other encoding or hashing methods may be added in the future.
+	Any preexisting non-encoded submodule gitdirs are used as-is, to
+	ease migration and reduce risk of gitdirs not being recognized.
+
 worktreeConfig:::
 	If enabled, then worktrees will load config settings from the
 	`$GIT_DIR/config.worktree` file in addition to the
diff --git a/Documentation/config/submodule.adoc b/Documentation/config/submodule.adoc
@@ -52,6 +52,13 @@ 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 or change the default path name encoding.
+	Submodule gitdir encoding is enabled via `extensions.submoduleEncoding`
+	(see linkgit:git-config[1]). This config works both with the extension
+	enabled or disabled.
+
 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
diff --git a/Makefile b/Makefile
@@ -2268,6 +2268,11 @@ ifndef HAVE_PLATFORM_PROCINFO
 	COMPAT_OBJS += compat/stub/procinfo.o
 endif
 
+ifdef NO_PATHCONF
+	COMPAT_CFLAGS += -DNO_PATHCONF
+	COMPAT_OBJS += compat/pathconf.o
+endif
+
 ifdef RUNTIME_PREFIX
 
         ifdef HAVE_BSD_KERN_PROC_SYSCTL
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
@@ -76,12 +76,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))
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
@@ -1208,6 +1208,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;
@@ -3187,13 +3203,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);
@@ -3207,10 +3223,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;
@@ -3219,8 +3236,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"
@@ -3238,7 +3255,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;
@@ -3590,6 +3607,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),
diff --git a/compat/pathconf.c b/compat/pathconf.c
@@ -0,0 +1,10 @@
+#include "git-compat-util.h"
+
+/*
+ * Minimal stub for platforms without pathconf() (e.g. Windows),
+ * to fall back to NAME_MAX from limits.h or compat/posix.h.
+ */
+long git_pathconf(const char *path UNUSED, int name UNUSED)
+{
+	return -1;
+}
diff --git a/compat/posix.h b/compat/posix.h
@@ -250,6 +250,14 @@ char *gitdirname(char *);
 #define NAME_MAX 255
 #endif
 
+#ifdef NO_PATHCONF
+#ifndef _PC_NAME_MAX
+#define _PC_NAME_MAX 1 /* dummy value, only used for git_pathconf */
+#endif
+#define pathconf(a,b) git_pathconf(a,b)
+long git_pathconf(const char *path, int name);
+#endif
+
 typedef uintmax_t timestamp_t;
 #define PRItime PRIuMAX
 #define parse_timestamp strtoumax
diff --git a/config.mak.uname b/config.mak.uname
@@ -473,6 +473,7 @@ ifeq ($(uname_S),Windows)
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
+	NO_PATHCONF = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
 	NO_SETENV = YesPlease
@@ -688,6 +689,7 @@ ifeq ($(uname_S),MINGW)
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
+	NO_PATHCONF = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NO_SETENV = YesPlease
 	NO_STRCASESTR = YesPlease
diff --git a/meson.build b/meson.build
@@ -1396,6 +1396,7 @@ checkfuncs = {
   'initgroups' : [],
   'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
   'pread' : ['pread.c'],
+  'pathconf' : ['pathconf.c'],
 }
 
 if host_machine.system() == 'windows'
diff --git a/repository.h b/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;
diff --git a/setup.c b/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 =
diff --git a/setup.h b/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;
diff --git a/strbuf.c b/strbuf.c
@@ -817,6 +817,12 @@ void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)
 	}
 }
 
+int is_rfc3986_unreserved(char ch)
+{
+	return isalnum(ch) ||
+		ch == '-' || ch == '_' || ch == '.' || ch == '~';
+}
+
 static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
 				 char_predicate allow_unencoded_fn)
 {
diff --git a/strbuf.h b/strbuf.h
@@ -640,6 +640,8 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 
 typedef int (*char_predicate)(char ch);
 
+int is_rfc3986_unreserved(char ch);
+
 void strbuf_addstr_urlencode(struct strbuf *sb, const char *name,
 			     char_predicate allow_unencoded_fn);
 
diff --git a/submodule.c b/submodule.c
@@ -2262,6 +2262,13 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
 	char *p;
 	int ret = 0;
 
+	/*
+	 * Skip these checks when extensions.submoduleEncoding is enabled because
+	 * it fixes the nesting issues and the suffixes will not match by design.
+	 */
+	if (the_repository->repository_format_submodule_encoding)
+		return 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'",
@@ -2581,29 +2588,66 @@ int submodule_to_gitdir(struct repository *repo,
 	return ret;
 }
 
+static void strbuf_addstr_case_encode(struct strbuf *dst, const char *src)
+{
+	for (; *src; src++) {
+		unsigned char c = *src;
+		if (c >= 'A' && c <= 'Z') {
+			strbuf_addch(dst, '_');
+			strbuf_addch(dst, c - 'A' + 'a');
+		} else {
+			strbuf_addch(dst, c);
+		}
+	}
+}
+
 void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,
 			      const char *submodule_name)
 {
-	/*
-	 * 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.
-	 */
+	char *gitdir_path, *key;
+
+	/* Allow config override. */
+	key = xstrfmt("submodule.%s.gitdirpath", submodule_name);
+	if (!repo_config_get_string(r, key, &gitdir_path)) {
+		strbuf_addstr(buf, gitdir_path);
+		free(key);
+		free(gitdir_path);
+		return;
+	}
+	free(key);
+
 	repo_git_path_append(r, buf, "modules/");
 	strbuf_addstr(buf, submodule_name);
+
+	/* Existing legacy non-encoded names are used as-is */
+	if (is_git_directory(buf->buf))
+		return;
+
+	if (the_repository->repository_format_submodule_encoding) {
+		struct strbuf tmp = STRBUF_INIT;
+		size_t base_len;
+		long name_max;
+
+		strbuf_reset(buf);
+		repo_git_path_append(r, buf, "modules/");
+		base_len = buf->len;
+
+		strbuf_addstr_urlencode(&tmp, submodule_name, is_rfc3986_unreserved);
+		strbuf_addstr_case_encode(buf, tmp.buf);
+
+		/* Ensure final path length is below NAME_MAX after encoding */
+		name_max = pathconf(buf->buf, _PC_NAME_MAX);
+		if (name_max == -1)
+			name_max = NAME_MAX;
+
+		if (buf->len - base_len > name_max)
+			/*
+			 * TODO: make this smarter; instead of erroring out, maybe we could trim or
+			 * shard the gitdir names to make them fit under NAME_MAX.
+			 */
+			die(_("submodule name %s is too long (%"PRIuMAX" bytes, limit %"PRIuMAX")"),
+			    buf->buf, (uintmax_t)buf->len - base_len, (uintmax_t)name_max);
+
+		strbuf_release(&tmp);
+	}
 }
diff --git a/t/lib-verify-submodule-gitdir-path.sh b/t/lib-verify-submodule-gitdir-path.sh
@@ -0,0 +1,20 @@
+# 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" &&
+		cat >expect <<-EOF &&
+			$(git rev-parse --git-common-dir)/$path
+		EOF
+		git submodule--helper gitdir "$name" >actual &&
+		test_cmp expect actual
+	)
+}
diff --git a/t/meson.build b/t/meson.build
@@ -887,6 +887,7 @@ integration_tests = [
   't7422-submodule-output.sh',
   't7423-submodule-symlinks.sh',
   't7424-submodule-mixed-ref-formats.sh',
+  't7425-submodule-encoding.sh',
   't7450-bad-git-dotfiles.sh',
   't7500-commit-template-squash-signoff.sh',
   't7501-commit-basic-functionality.sh',
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
diff --git a/t/t7425-submodule-encoding.sh b/t/t7425-submodule-encoding.sh
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh

Original file line number	Diff line number	Diff line change
`@@ -76,12 +76,6 @@ static void rewrite_credential_file(const char fn, struct credential c,`
`76`	`76`	`die_errno("unable to write credential store");`
`77`	`77`	`}`
`78`	`78`
`79`		`-static int is_rfc3986_unreserved(char ch)`
`80`		`-{`
`81`		`- return isalnum(ch) \|\|`
`82`		`- ch == '-' \|\| ch == '_' \|\| ch == '.' \|\| ch == '~';`
`83`		`-}`
`84`		`-`
`85`	`79`	`static int is_rfc3986_reserved_or_unreserved(char ch)`
`86`	`80`	`{`
`87`	`81`	`if (is_rfc3986_unreserved(ch))`
Original file line number	Diff line number	Diff line change
`@@ -1396,6 +1396,7 @@ checkfuncs = {`
`1396`	`1396`	`'initgroups' : [],`
`1397`	`1397`	`'strtoumax' : ['strtoumax.c', 'strtoimax.c'],`
`1398`	`1398`	`'pread' : ['pread.c'],`
	`1399`	`+ 'pathconf' : ['pathconf.c'],`
`1399`	`1400`	`}`
`1400`	`1401`
`1401`	`1402`	`if host_machine.system() == 'windows'`