git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 22, 2021, 11:50 UTC
Message-ID
<87sg06tvab.fsf@evledraar.gmail.com>
In-Reply-To
<20210722112143.97944-1-raykar.ath@gmail.com>
On Thu, Jul 22 2021, Atharva Raykar wrote:
Show 11 quoted lines
> +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);
> +	free(key);
> +}

Just a small point not per-se to do with this patch, but aren't all callers of config_set_in_gitmodules_file_gently() wanting to prefix thigs with "submodule."? Looks like its API could be simplified a bit with that xstrfmt() and free() inside that function.

Show 6 quoted lines
> +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;

Usual style is to have different variables on different lines, unless they're closely related (like "int i, j"), so "char *key;\n char *submod[...]" in this case.

Show 27 quoted lines
> +
> +	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);
> +
> +	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);
Looks good at a glance.
> +	/*
> +	 * NEEDSWORK: In a multi-working-tree world this needs to be
> +	 * set in the per-worktree config.
> +	 */

So should we have a failing test for that scenario, or...? (Update: but read ahead...)

Show 36 quoted lines
> +static int add_config(int argc, const char **argv, const char *prefix)
> +{
> +	int force = 0;
> +	struct add_data add_data = ADD_DATA_INIT;
> +
> +	struct option options[] = {
> +		OPT_STRING('b', "branch", &add_data.branch,
> +			   N_("branch"),
> +			   N_("branch of repository to store in "
> +			      "the submodule configuration")),
> +		OPT_STRING(0, "url", &add_data.repo,
> +			   N_("string"),
> +			   N_("url to clone submodule from")),
> +		OPT_STRING(0, "resolved-url", &add_data.realrepo,
> +			   N_("string"),
> +			   N_("url to clone the submodule from, after it has "
> +			      "been dereferenced relative to parent's url, "
> +			      "in the case where <url> is a relative url")),
> +		OPT_STRING(0, "path", &add_data.sm_path,
> +			   N_("path"),
> +			   N_("where the new submodule will be cloned to")),
> +		OPT_STRING(0, "name", &add_data.sm_name,
> +			   N_("string"),
> +			   N_("name of the new submodule")),
> +		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
> +			   PARSE_OPT_NOCOMPLETE),
> +		OPT_END()
> +	};
> +
> +	const char *const usage[] = {
> +		N_("git submodule--helper add-config "
> +		   "[--force|-f] [--branch|-b <branch>] "
> +		   "--url <url> --resolved-url <resolved-url> "
> +		   "--path <path> --name <name>"),
> +		NULL
> +	};

I'd say consider adding this as a "static" earlier in the file, but it's an established pattern in this file, so let's keep it.

> +	argc = parse_options(argc, argv, prefix, options, usage, 0);

It's fine to omit it for a helper, but we're being non-pedantic about checking mandatory options here. Would do it in a "real" built-in, but for internal use it's fine.

> +	if (argc != 0)
Style: if (!argc)
Show 43 quoted lines
> +		usage_with_options(usage, options);
> +
> +	add_data.force = !!force;
> +	configure_added_submodule(&add_data);
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2949,6 +3073,7 @@ static struct cmd_struct commands[] = {
>  	{"name", module_name, 0},
>  	{"clone", module_clone, 0},
>  	{"add-clone", add_clone, 0},
> +	{"add-config", add_config, 0},
>  	{"update-module-mode", module_update_module_mode, 0},
>  	{"update-clone", update_clone, 0},
>  	{"ensure-core-worktree", ensure_core_worktree, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 053daf3724..f713cb113c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -242,33 +242,7 @@ cmd_add()
>  	fi
>  
>  	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> -	git config submodule."$sm_name".url "$realrepo"
> -
> -	git add --no-warn-embedded-repo $force "$sm_path" ||
> -	die "fatal: $(eval_gettext "Failed to add submodule '\$sm_path'")"
> -
> -	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
> -	git submodule--helper config submodule."$sm_name".url "$repo" &&
> -	if test -n "$branch"
> -	then
> -		git submodule--helper config submodule."$sm_name".branch "$branch"
> -	fi &&
> -	git add --force .gitmodules ||
> -	die "fatal: $(eval_gettext "Failed to register submodule '\$sm_path'")"
> -
> -	# NEEDSWORK: In a multi-working-tree world, this needs to be
> -	# set in the per-worktree config.
Ah, this is the NEEDSWORK comment, just copied to the C code...
Show 14 quoted lines
> -	if git config --get submodule.active >/dev/null
> -	then
> -		# If the submodule being adding isn't already covered by the
> -		# current configured pathspec, set the submodule's active flag
> -		if ! git submodule--helper is-active "$sm_path"
> -		then
> -			git config submodule."$sm_name".active "true"
> -		fi
> -	else
> -		git config submodule."$sm_name".active "true"
> -	fi
> +	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
>  }
>  
Very nice to have this simplified.

Would be good to split this very long line across multiple lines though...

Previous: Atharva RaykarNext: Atharva Raykar
Message 3 of 13 in “submodule--helper: introduce add-config subcommand”
  1. Atharva RaykarJul 22, 2021
  2. Atharva RaykarJul 22, 2021
  3. Ævar Arnfjörð BjarmasonJul 22, 2021
  4. Atharva RaykarJul 22, 2021
  5. Atharva RaykarJul 22, 2021
  6. Junio C HamanoJul 23, 2021
  7. Atharva RaykarJul 24, 2021
  8. [GSoC] [PATCH v2] submodule--helper: introduce add-config subcommandAtharva Raykar, Jul 28, 2021
  9. Kaartic SivaraamJul 28, 2021
  10. [GSoC] [PATCH v3] submodule--helper: introduce add-config subcommandAtharva Raykar, Aug 1, 2021
  11. Kaartic SivaraamAug 5, 2021
  12. Kaartic SivaraamJul 29, 2021
  13. Atharva RaykarJul 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.