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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Jul 9, 2025, 02:50 UTC
Message-ID: <CALnO6CAXermmya0UjTHU2gPoEAb1m32fOt7Uzf6CL2mSJ=RkTg@mail.gmail.com>
In-Reply-To: <20250608032705.11990-3-jayatheerthkulkarni2005@gmail.com>

On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth <jayatheerthkulkarni2005@gmail.com> wrote:

Show 9 quoted lines

>
> 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.
>
> change the logic 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.

I use submodules, but am not too familiar with their internals, so I find it hard to follow the details here. Perhaps some examples showing when the configuration becomes cluttered and what about it is cluttered would help?

When I check a repo of mine that has ~50 submodules:

    git config get submodule.active
    .
    git config get --show-names --regexp 'submodule.*active'
    submodule.active .

So I'm not seeing this in practice, though I didn't try with the test case in the patch.

[Nit: s/change/Change]

Show 39 quoted lines

>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  builtin/submodule--helper.c    | 32 +++++++++++++++++++++-----------
>  t/t7413-submodule-is-active.sh | 15 +++++++++++++++
>  2 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9f6df833f0..514abe480e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -32,6 +32,8 @@
>  #include "advice.h"
>  #include "branch.h"
>  #include "list-objects-filter-options.h"
> +#include "wildmatch.h"
> +#include "strbuf.h"
>
>  #define OPT_QUIET (1 << 0)
>  #define OPT_CACHED (1 << 1)
> @@ -3328,6 +3330,9 @@ static void configure_added_submodule(struct add_data *add_data)
>         char *key;
>         struct child_process add_submod = CHILD_PROCESS_INIT;
>         struct child_process add_gitmodules = CHILD_PROCESS_INIT;
> +       const struct string_list *values;
> +       size_t i;
> +       int matched = 0;
>
>         key = xstrfmt("submodule.%s.url", add_data->sm_name);
>         git_config_set_gently(key, add_data->realrepo);
> @@ -3370,20 +3375,25 @@ 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
> -                */

Do we want to lose this comment? The replacement below ("… -> force-enable") is a bit terse for me and rather loses some information.

Show 48 quoted lines

> -               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 (git_config_get("submodule.active") || /* key absent */
> +           git_config_get_string_multi("submodule.active", &values)) {
> +               /* submodule.active is missing -> force-enable */
>                 key = xstrfmt("submodule.%s.active", add_data->sm_name);
>                 git_config_set_gently(key, "true");
>                 free(key);
> +       } else {
> +               for (i = 0; i < values->nr; i++) {
> +                       const char *pat = values->items[i].string;
> +                       if (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */
> +                               matched = 1;
> +                               break;
> +                       }
> +               }
> +               if (!matched) { /* no pattern matched -> force-enable */
> +                       key = xstrfmt("submodule.%s.active", add_data->sm_name);
> +                       git_config_set_gently(key, "true");
> +                       free(key);
> +               }
>         }
>  }
>
> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
> index 9509dc18fd..a42060cac9 100755
> --- a/t/t7413-submodule-is-active.sh
> +++ b/t/t7413-submodule-is-active.sh
> @@ -124,4 +124,19 @@ test_expect_success 'is-active, submodule.active and submodule add' '
>         git -C super2 config --get submodule.mod.active
>  '
>
> +test_expect_success 'submodule add skips redundant active entry' '
> +       git init repo &&
> +       (
> +               cd repo &&
> +               git config submodule.active "lib/*" &&
> +               git commit --allow-empty -m init &&
> +
> +               git init ../lib-origin &&
> +               git -C ../lib-origin commit --allow-empty -m init &&
> +
> +               git submodule add ../lib-origin lib/foo &&
> +               ! git config --get submodule.lib/foo.active

(Not my area of expertise) Should this be test_must_fail?

Show 8 quoted lines

> +       )
> +'
> +
>  test_done
> --
> 2.49.GIT
>
>

-- 
D. Ben Knoble