From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Wed, 07 Jan 2026 16:42:17 GMT
Subject: Re: [PATCH v7 06/11] submodule--helper: add gitdir migration command
Message-ID: <871pk1idcm.fsf@collabora.com>
In-Reply-To: <aVy5Ap7wX4aW-UL4@pks.im>

On Tue, 06 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:
> 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.

Ack, it's best to future proof this. Will do in v8.

>> 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.

Ack, will do.