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

[Atharva Raykar <raykar.ath@gmail.com>]

On 07-Jun-2021, at 14:54, Christian Couder <christian.couder@gmail.com> wrote:
> 
> On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar <raykar.ath@gmail.com> 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'?

Will do.

For now I'll leave an explanation here as well, so that those
who might see this thread can know the motivation behind it.

(I'll make it more concise in my cover letter of v2)

Junio in his review of Shourya's patch[1] said:
> When a user has "[submodule] active" in his or her
> configuration file, it is a configuration error.  When Git reads
> "submodule.active" configuration variable to make a decision (like
> the above code) and finds that the user has such an error, the user
> would appreciate if the error is pointed out, so that it can be
> corrected, rather than silently ignored.

Git might set 'submodule.<name>.active = true' in the above case, and
since it has a higher priority[2] than the 'submodule.active' switch,
it would be useful to let the user know of this change, and the incorrect
configuration that led to this behaviour, so that they may be able to
remedy it, and avoid surprises later on.

[1] https://lore.kernel.org/git/xmqqo8isxefz.fsf@gitster.c.googlers.com/
[2] https://git-scm.com/docs/gitsubmodules#_active_submodules

>> 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()

Hmm, I see the issue.

Would it be more accurate to say this:

"The submodule.active configuration exists, but no pathspec
was specified. If the module is not already active, the value
of 'submodule.<name>.active' will be set to 'true'."

> , 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)) {

Got it. Thanks for suggesting this improvement!

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

:^)