On Tue, May 27, 2025 at 8:12 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx> 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.
>
I've actually done something like this, but I've wrapped the core logic within
the if else after checking [1]
> > @@ -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);
I've done the exact thing in a bit different way
Should I add this in the second patch, because this passes the tests too
including the new one created.
1- https://lore.kernel.org/git/20250518075436.75139-1-jayatheerthkulkarni2005@xxxxxxxxx/
