On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
<jayatheerthkulkarni2005@xxxxxxxxx> wrote:
>
> 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]
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx>
> ---
> 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.
> - 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?
> + )
> +'
> +
> test_done
> --
> 2.49.GIT
>
>
--
D. Ben Knoble
