On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar <raykar.ath@xxxxxxxxx> 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!
