On 24/07/21 02:06, Junio C Hamano wrote:
> Atharva Raykar <raykar.ath@xxxxxxxxx> writes:
>
>> 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",
>
> ... meaning that submodule.active is *not* a boolean.
>
> In scripted porcelain, I think we let "submodule--helper is-active"
> to inspect its value(s), which may end up feeding a NULL as one of
> the pathspec elements when calling parse_pathspec(), so this may
> even be a bugfix. In any case, I think "submodule--helper
> is-active" is where such a fix should happen and in the longer term,
> the code that says "if submodule.active exists, ask is-active and
> set submodule.*.active accordingly, otherwise activate everything"
> we see in this patch should be simplified to always ask is-active
> and let is-active worry about cases like missing submodule.active
> and submodule.active being valueless true, so let's not worry too
> much about what happens in this patch, because it needs to be
> cleaned up anyway after the dust settles.
Okay, that makes sense. I'll remove the extra warning and special
handling and make it a bug-for-bug conversion for now, so that the
cleanup can be handled afterwards. It will probably be more fitting to
have this change 'is_submodule_active()' afterwards. I'll maybe add a
NEEDSWORK comment for now?
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 862053c9f2..9658804d24 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)
>> return 0;
>> }
>>
>> +static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)
>> +{
>> + char *key;
>> +
>> + if (!is_writing_gitmodules_ok())
>> + die(_("please make sure that the .gitmodules file is in the working tree"));
>> +
>> + key = xstrfmt("submodule.%s.%s", name, var);
>> + config_set_in_gitmodules_file_gently(key, value);
>
> This uses _gently() to avoid dying, but does it discard error return
> and hide it from our callers?
>
>> + free(key);
>> +}
>> +
>> +static void configure_added_submodule(struct add_data *add_data)
>> +{
>> + char *key, *submod_pathspec = NULL;
>> + struct child_process add_submod = CHILD_PROCESS_INIT;
>> + struct child_process add_gitmodules = CHILD_PROCESS_INIT;
>> + int pathspec_key_exists, activate = 0;
>> +
>> + key = xstrfmt("submodule.%s.url", add_data->sm_name);
>> + git_config_set_gently(key, add_data->realrepo);
>> + free(key);
>> +
>> + add_submod.git_cmd = 1;
>> + strvec_pushl(&add_submod.args, "add",
>> + "--no-warn-embedded-repo", NULL);
>> + if (add_data->force)
>> + strvec_push(&add_submod.args, "--force");
>> + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
>> +
>> + if (run_command(&add_submod))
>> + die(_("Failed to add submodule '%s'"), add_data->sm_path);
>> +
>> + config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path);
>> + config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo);
>> + if (add_data->branch)
>> + config_submodule_in_gitmodules(add_data->sm_name,
>> + "branch", add_data->branch);
>
> A failure in any of the above in the scripted version used to result
> in "failed to register submodule" error, but they are now ignored.
> Intended?
This was not intended. I think I did not notice those expressions were
chained in the scripted version. I'll fix this.
>> + add_gitmodules.git_cmd = 1;
>> + strvec_pushl(&add_gitmodules.args,
>> + "add", "--force", "--", ".gitmodules", NULL);
>> +
>> + if (run_command(&add_gitmodules))
>> + die(_("Failed to register submodule '%s'"), add_data->sm_path);
>> +
>> + /*
>> + * 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 the "
>> + "pathspec was unset. If the submodule is not already "
>> + "active, the value of submodule.%s.active will be "
>> + "be set to 'true'."), add_data->sm_name);
>> + activate = 1;
>> + }
>> +
>> + /*
>> + * If submodule.active does not exist, or if the pathspec was unset,
>> + * 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 || activate ||
>> + !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);
>> + }
>
> This is the part I discussed earlier. I think this "optimize so
> that we can avoid calling is_submodule_active()" should go away in
> the long run. In the current code, is_submodule_active() needs to
> find out the value of submodule.active itself anyway, so the
> short-circuit is not working as an optimization.
Agreed.
> Other than the "what happens when we see errors?" issue, the patch
> looks quite straight-forward rewrite from the scripted version.
>
> Thanks.
>
