Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path

From: Junio C Hamano <gitster@pobox.com>
Date: May 27, 2025, 14:42 UTC
Message-ID: <xmqqcyburu6z.fsf@gitster.g>
In-Reply-To: <20250524073628.58944-3-jayatheerthkulkarni2005@gmail.com>

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> configure_added_submodule always writes an explicit submodule.<name>.active
> entry, even when the new path is already matched by submodule.active
> patterns. This leads to unnecessary and cluttered configuration.

It might be less cluttered but is it a good thing?

Earlier, if submodule.active were b* (as documented in the "git help submodules") and then you added submodule "baz", your "baz" would be kept active even after you reconfigured submodule.active to another pattern. With this change, that is no longer true, which to existing users is a change in behaviour, and to some it may appear a regression.

According to that documentation, presense of submodule.<name>.url is also a signal that the submodule is activated. If we are going to omit setting .active because its path matches submodule.active, shouldn't we also be checking if .url exists and omit setting .active as well?

> Introduce a single helper to centralize wildmatch-based pattern lookup.
> In configure_added_submodule, wrap the active-entry write in a conditional
> that only fires when that helper reports no existing pattern covers the
> submodule’s path.

The new helper is a maintenance burden to keep in sync with submodule.c:is_tree_submodule_active(); if we really want to go this route, the patch should extract that "ah, submodule.active is set so let's turn it into pathspec and see if the path matches" part of the logic to make sure the logic is shared. But I am wondering if we can do this without any new helper.

Show 16 quoted lines

> @@ -3370,17 +3390,7 @@ static void configure_added_submodule(struct add_data *add_data)
>  	 * is_submodule_active(), since that function needs to find
>  	 * out the value of "submodule.active" again anyway.
>  	 */
> -	if (!git_config_get("submodule.active")) {
> -		/*
> -		 * If the submodule being added isn't already covered by the
> -		 * current configured pathspec, set the submodule's active flag
> -		 */
> -		if (!is_submodule_active(the_repository, add_data->sm_path)) {
> -			key = xstrfmt("submodule.%s.active", add_data->sm_name);
> -			git_config_set_gently(key, "true");
> -			free(key);
> -		}
> -	} else {
> +	if (!submodule_active_matches_path(add_data->sm_path)) {

I.e.  Can we replace this if() condition with something like this?

	/*
	 * Explicitly set 'submodule.<name>.active' only if it is not
	 * 'active' due to other reasons.
	 */
	if (!is_submodule_active(the_repository, add_data->sm_path)) {

That is, we ask if the submodule is already active (we are before adding submodule.<name>.active for this thing---if may be active due to submodule.active or submodule.<name>.url) and enter the block only when it is not yet.

That way, this codepath does not have to worry about the exact logic that determines if a submodule is 'active' even when its .active configuration variable is not set.

>  		key = xstrfmt("submodule.%s.active", add_data->sm_name);
>  		git_config_set_gently(key, "true");
>  		free(key);