From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 06 Jan 2026 07:25:54 GMT
Subject: Re: [PATCH v7 06/11] submodule--helper: add gitdir migration command
Message-ID: <aVy5Ap7wX4aW-UL4@pks.im>
In-Reply-To: <20251220101528.1227487-7-adrian.ratiu@collabora.com>

On Sat, Dec 20, 2025 at 12:15:23PM +0200, Adrian Ratiu wrote:
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f8cae345a5..5a6436f18f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1266,6 +1266,63 @@ static int module_gitdir(int argc, const char **argv, const char *prefix UNUSED,
>  	return 0;
>  }
>  
> +static int module_migrate(int argc UNUSED, const char **argv UNUSED,
> +			  const char *prefix UNUSED, struct repository *repo)
> +{
> +	struct strbuf module_dir = STRBUF_INIT;
> +	DIR *dir;
> +	struct dirent *de;
> +
> +	repo_git_path_append(repo, &module_dir, "modules/");
> +
> +	dir = opendir(module_dir.buf);
> +	if (!dir)
> +		die(_("could not open '%s'"), module_dir.buf);
> +
> +	while ((de = readdir(dir))) {
> +		struct strbuf gitdir_path = STRBUF_INIT;
> +		char *key;
> +		const char *value;
> +
> +		if (is_dot_or_dotdot(de->d_name))
> +			continue;
> +
> +		strbuf_addf(&gitdir_path, "%s/%s", module_dir.buf, de->d_name);
> +		if (!is_git_directory(gitdir_path.buf)) {
> +			strbuf_release(&gitdir_path);
> +			continue;
> +		}
> +		strbuf_release(&gitdir_path);
> +
> +		key = xstrfmt("submodule.%s.gitdir", de->d_name);
> +		if (!repo_config_get_string_tmp(repo, key, &value)) {
> +			/* Already has a gitdir config, nothing to do. */
> +			free(key);
> +			continue;
> +		}
> +		free(key);
> +
> +		create_default_gitdir_config(de->d_name);
> +	}
> +
> +	closedir(dir);
> +	strbuf_release(&module_dir);
> +
> +	if (repo_config_set_gently(repo, "core.repositoryformatversion", "1"))
> +		die(_("could not set core.repositoryformatversion to 1. "
> +		      "Please enable it for migration to work, for example: "
> +		      "git config core.repositoryformatversion 1"));

We should probably be careful here to not override the repository format
version in case it's already greater than 0. We don't have version 2
yet, but if we ever do this would otherwise need to be changed.

> diff --git a/t/t7425-submodule-gitdir-path-extension.sh b/t/t7425-submodule-gitdir-path-extension.sh
> index 06ee1ff86b..6ca9f13a59 100755
> --- a/t/t7425-submodule-gitdir-path-extension.sh
> +++ b/t/t7425-submodule-gitdir-path-extension.sh
> @@ -260,4 +260,71 @@ test_expect_success '`git clone --recurse-submodules` respects init.autoSetupSub
>  	git config --global --unset init.autoSetupSubmodulePathConfig
>  '
>  
> +test_expect_success 'submodule--helper migrates legacy modules' '
> +	(
> +		cd upstream &&
> +
> +		# previous submodules exist and were not migrated yet
> +		test_must_fail git config submodule.sub1.gitdir &&
> +		test_must_fail git config submodule.sub2.gitdir &&
> +		test_path_is_dir .git/modules/sub1 &&
> +		test_path_is_dir .git/modules/sub2 &&
> +
> +		# run migration
> +		git submodule--helper migrate-gitdir-configs &&
> +
> +		# test that migration worked
> +		git config submodule.sub1.gitdir >actual &&
> +		echo ".git/modules/sub1" >expect &&
> +		test_cmp expect actual &&
> +		git config submodule.sub2.gitdir >actual &&
> +		echo ".git/modules/sub2" >expect &&
> +		test_cmp expect actual &&
> +
> +		# repository extension is enabled after migration
> +		git config extensions.submodulePathConfig > actual &&
> +		echo "true" > expect &&

Style nit: redirection operator strikes again :) Probably makes sense to
scan through all commits for this style issue.

Patrick