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

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