Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2025, 15:41 UTC
Message-ID: <xmqqwmacobf0.fsf@gitster.g>
In-Reply-To: <20250518075436.75139-1-jayatheerthkulkarni2005@gmail.com>

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

Show 8 quoted lines

> When a submodule is added, Git writes submodule.<name>.active = true
> to the repository configuration to mark it as active. This happens even
> when the submodule path already matches a pattern in submodule.active.
> This results in redundant configuration entries that are unnecessary
> and clutter the config, especially when pattern-based activation is used.
>
> Avoid writing the submodule.<name>.active entry if the path is already
> covered by a pattern in submodule.active.

This explains why the part of the change that deals the .active bit makes sense.

But now do we drop the other fix from the patch, namely "ouch, we are adding submodule at 'foo/' but the name 'foo' is taken by a different submodule that used to live there and moved elsewhere", or have you forgotten to describe that fix in the proposed log message?

Stepping back a bit, perhaps this patch addresses two independent issues, both of which can trigger with"submodule add"? If so, would it make sense to have it in two separate patches?

Show 22 quoted lines

> +test_expect_success 'submodule add fails when name is reused' '
> +	git init test-submodule &&
> +	(
> +		cd test-submodule &&
> +		git commit --allow-empty -m "initial commit" &&
> +
> +		git init ../child-origin &&
> +		git -C ../child-origin commit --allow-empty -m "initial commit" &&
> +
> +		git submodule add ../child-origin child &&
> +		git commit -m "Add submodule child" &&
> +
> +		git mv child child_old &&
> +		git commit -m "Move child to child_old" &&
> +
> +		# Create another submodule repo
> +		git init ../child2-origin &&
> +		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
> +
> +		test_must_fail git submodule add ../child2-origin child
> +	)
> +'

The test seems to be about "the other issue". Shouldn't we also have a test about "we no longer add redundant configuration entries"?

Thanks.