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

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

On 24/07/21 02:06, Junio C Hamano wrote:
> Atharva Raykar <raykar.ath@gmail.com> 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.
>