Re: [PATCH 1/2] submodule: prevent overwriting .gitmodules on path reuse

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2025, 20:53 UTC
Message-ID: <xmqqjz3xz4ww.fsf@gitster.g>
In-Reply-To: <20250724152418.45226-2-jayatheerthkulkarni2005@gmail.com>

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

Show 21 quoted lines

> Adding a submodule at a path that previously hosted
> another submodule (e.g., 'child') reuses the submodule
> name derived from the path. If the original submodule
> was only moved (e.g., to 'child_old') and not renamed,
> this silently overwrites its configuration in .gitmodules.
>
> This behavior loses user configuration and causes
> confusion when the original submodule is expected
> to remain intact. It assumes that the path-derived
> name is always safe to reuse, even though the name
> might still be in use elsewhere in the repository.
>
> Teach module_add() to check if the computed submodule
> name already exists in the repository's submodule config,
> and if so, refuse the operation unless the user explicitly
> renames the submodule or uses the --force option,
> which will automatically generate a unique name by
> appending a number (e.g., child1).
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---

Very well described.

Show 8 quoted lines

> +	existing = submodule_from_name(the_repository,
> +					null_oid(the_hash_algo),
> +					add_data.sm_name);
> +	
> +	if (existing && strcmp(existing->path, add_data.sm_path)) {
> +		if (!force) {
> +			die(_("submodule name '%s' already used for path '%s'"),
> +			add_data.sm_name, existing->path);

I'll locally fix this funny indentation; not a reason to require an update.

> +		}
> +		/* --force: build <name><n> until unique */
> +		for (i = 1; ; i++) {

I think you can narrow the scope of "i" to this loop alone. I'll locally do so (and if anything breaks, which I doubt); not a reason to require an update.

Show 5 quoted lines

> + ...
> +		# Now adding a *new* repo at the old name must fail
> +		git init ../child2-origin &&
> +		git -C ../child2-origin commit --allow-empty -m init &&
> +		test_must_fail git submodule add ../child2-origin child

Shouldn't we also check what this failed command tell the end-user?  E.g.

	test_must_fail git submodule add ../child2-origin child	2>err &&
	test_grep "alreayd used for" err