On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar [off-list ref] wrote:

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n".

Maybe explaining a bit how this warning is useful could help reviewers
here. Especially what could happen if no value is specified in
'submodule.active'?

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

[...]

+       /*
+        * NEEDSWORK: In a multi-working-tree world this needs to be
+        * set in the per-worktree config.
+        */
+       pathspec_key_exists = !git_config_get_string("submodule.active",
+                                                    &submod_pathspec);
+       if (pathspec_key_exists && !submod_pathspec)
+               warning(_("The submodule.active configuration exists, but "
+                         "no pathspec was specified. Setting the value of "
+                         "submodule.%s.active to 'true'."), add_data->sm_name);

It's not very clear that we will actually set
'submodule.<name>.active' to true below as it depends on the result of
calling is_submodule_active(), and anyway is_submodule_active() will
check again if "submodule.active" is set, which is wasteful.

Maybe we could set a variable, called for example "activate" here,
with something like:

       if (pathspec_key_exists && !submod_pathspec) {
               warning(...);
               activate = 1;
      }

and below use a check like:

       if (!pathspec_key_exists || activate ||
           !is_submodule_active(the_repository, add_data->sm_path)) {
...

+       /*
+        * If submodule.active does not exist, we will activate this
+        * module unconditionally.
+        *
+        * Otherwise, we ask is_submodule_active(), which iterates
+        * through all the values of 'submodule.active' to determine
+        * if this module is already active.
+        */
+       if (!pathspec_key_exists ||
+           !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);
+       }
+}

The rest looks good to me! Thanks!