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

[Junio C Hamano <gitster@pobox.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?

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

>  	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