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

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2025, 22:47 UTC
Message-ID: <xmqqplga6c80.fsf@gitster.g>
In-Reply-To: <20250514020108.24396-1-jayatheerthkulkarni2005@gmail.com>

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> Add helper `submodule_active_matches_path()` so we can
> re-implement the old “is this path already covered by
> submodule.active?” logic without re-reading the config twice.

Having duplicated code to implement what is supposed to be the same thing is a bug waiting to happen by them diverging from each other.

Isn't the fact that our configuration reading code reads things just once and the caches the result good enough for the purpose of this code path? Do we have a measurement that tells us that the extra complexity is worth the maintenance headache?

Show 9 quoted lines

> @@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,
>  	int force = 0, quiet = 0, progress = 0, dissociate = 0;
>  	struct add_data add_data = ADD_DATA_INIT;
>  	const char *ref_storage_format = NULL;
> +	const struct submodule *existing;
>  	char *to_free = NULL;
> +	struct strbuf buf = STRBUF_INIT;
> +	int i;
> +	int allocated_sm_name = 0;

A separate flag is not wrong per-se, but the idiom used in this project more often is to have an extra pointer variable that points at an allocated piece of memory (or NULL), and free the piece of memory unconditionally.

"git grep -e to_free" to see the idiom in action. Even better yet, this codepath already uses the idiom.

By doing so

> +	if (allocated_sm_name)
> +		free((char *)add_data.sm_name);

becomes

	free(sm_name_to_free);

and we can keep the "add_data.sm_name is pointing at a borrowed piece of memory, and we will _never_ free anything through that pointer" memory ownership rule. We were borrowing from a separate variable sm_name_to_free, and we may free it when add_data is getting destroyed, or we may be borrowing from the .sm_path string, which we would free it when add_data is getting destroyed.

Show 35 quoted lines

>  	free(add_data.sm_path);
>  	free(to_free);
>  	strbuf_release(&sb);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index d6a501d453..5c3f471338 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '
>  	)
>  '
>  
> +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
> +	)
> +'
> +
>  test_done