Re: [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 7, 2021, 09:24 UTC
Message-ID: <CAP8UFD2kwSB60kF_cjs4JYSYeXzvjAtz0OuFOfTqEU7F+ijR-w@mail.gmail.com>
In-Reply-To: <20210605113913.29005-3-raykar.ath@gmail.com>

On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar <raykar.ath@gmail.com> wrote:

Show 13 quoted lines

>
> 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'?

Show 8 quoted lines

> 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.

[...]

Show 10 quoted lines

> +       /*
> +        * 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)) {
...

Show 15 quoted lines

> +       /*
> +        * 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!