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

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

> @@ -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);