git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
@ 2021-07-22 11:21 Atharva Raykar
  2021-07-22 11:41 ` Atharva Raykar
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Atharva Raykar @ 2021-07-22 11:21 UTC (permalink / raw)
  To: git
  Cc: Atharva Raykar, Emily Shaffer, Jonathan Nieder, Junio C Hamano,
	Christian Couder, Shourya Shukla, Kaartic Sivaraam,
	Eric Sunshine, Prathamesh Chavan,
	Đoàn Trần Công Danh, Rafael Silva

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",
because it is an invalid configuration. It would be helpful to let the
user know that the pathspec is unset, and the value of
'submodule.<name>.active' might be set to 'true' so that they can
rectify their configuration and prevent future surprises (especially
given that the latter variable has a higher priority than the former).

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.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <periperidip@gmail.com>
Based-on-patch-by: Shourya Shukla <periperidip@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---

This patch depends on changes introduced in 83913dd5b6 (t7400: test failure to
add submodule in tracked path, 2021-06-18), which belongs to the
ar/submodule-add (2021-07-12) series[1].

This changes in this patch are no different from the one in my first
submodule-add conversion series[2] which has already got some reviews before.

[1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/
[2] https://lore.kernel.org/git/20210615145745.33382-4-raykar.ath@gmail.com/

 builtin/submodule--helper.c | 125 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +-------
 2 files changed, 126 insertions(+), 27 deletions(-)

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);
+	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);
+
+	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);
+	}
+}
+
+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
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		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.
-	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"
 }
 
 #
-- 
2.32.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
  2021-07-22 11:21 [GSoC] [PATCH] submodule--helper: introduce add-config subcommand Atharva Raykar
@ 2021-07-22 11:41 ` Atharva Raykar
  2021-07-22 11:50 ` Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Atharva Raykar @ 2021-07-22 11:41 UTC (permalink / raw)
  To: Git List
  Cc: Emily Shaffer, Jonathan Nieder, Junio C Hamano, Christian Couder,
	Shourya Shukla, Kaartic Sivaraam, Eric Sunshine,
	Prathamesh Chavan, Đoàn Trần Công Danh,
	Rafael Silva

On 22-Jul-2021, at 16:51, 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",
> because it is an invalid configuration. It would be helpful to let the
> user know that the pathspec is unset, and the value of
> 'submodule.<name>.active' might be set to 'true' so that they can
> rectify their configuration and prevent future surprises (especially
> given that the latter variable has a higher priority than the former).
> 
> 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.
> 
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Shourya Shukla <periperidip@gmail.com>
> Based-on-patch-by: Shourya Shukla <periperidip@gmail.com>
> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> 
> This patch depends on changes introduced in 83913dd5b6 (t7400: test failure to
> add submodule in tracked path, 2021-06-18), which belongs to the
> ar/submodule-add (2021-07-12) series[1].
> 
> This changes in this patch are no different from the one in my first
> submodule-add conversion series[2] which has already got some reviews before.
> 
> [1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/
> [2] https://lore.kernel.org/git/20210615145745.33382-4-raykar.ath@gmail.com/

I forgot to mention, you can fetch this change through:
git fetch https://github.com/tfidfwastaken/git.git submodule-helper-add-config-1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
  2021-07-22 11:21 [GSoC] [PATCH] submodule--helper: introduce add-config subcommand Atharva Raykar
  2021-07-22 11:41 ` Atharva Raykar
@ 2021-07-22 11:50 ` Ævar Arnfjörð Bjarmason
  2021-07-22 13:28   ` Atharva Raykar
  2021-07-22 13:31 ` Atharva Raykar
  2021-07-23 20:36 ` Junio C Hamano
  3 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-22 11:50 UTC (permalink / raw)
  To: Atharva Raykar
  Cc: git, Emily Shaffer, Jonathan Nieder, Junio C Hamano,
	Christian Couder, Shourya Shukla, Kaartic Sivaraam,
	Eric Sunshine, Prathamesh Chavan,
	Đoàn Trần Công Danh, Rafael Silva


On Thu, Jul 22 2021, Atharva Raykar wrote:

> +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.

> +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.

> +
> +	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...)

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

> +		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...

> -	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...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
  2021-07-22 11:50 ` Ævar Arnfjörð Bjarmason
@ 2021-07-22 13:28   ` Atharva Raykar
  0 siblings, 0 replies; 7+ messages in thread
From: Atharva Raykar @ 2021-07-22 13:28 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Emily Shaffer, Jonathan Nieder, Junio C Hamano,
	Christian Couder, Shourya Shukla, Kaartic Sivaraam,
	Eric Sunshine, Prathamesh Chavan,
	Đoàn Trần Công Danh, Rafael Silva

On 22-Jul-2021, at 17:20, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> 
> On Thu, Jul 22 2021, Atharva Raykar wrote:
> 
>> +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.

Yes, you are right, all the callers prefix with "submodule.". Changing
the API to what you suggested, would require some special handling in
'submodule--helper:module_config()', which will be dead code after the
whole series is done, so probably a better time to change it would be
during the cleanup after the conversion.

>> +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.

Okay.

>> +
>> +	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...)
> 
>> +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)

Will fix.

>> +		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...
> 
>> -	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...

I followed the established pattern of not splitting the lines from the
previous conversions ('submodule--helper update-clone' being the
exception in that file).

In this case, I felt it should be fine because a follow-up series that
completes the full conversion will get rid of that line entirely.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
  2021-07-22 11:21 [GSoC] [PATCH] submodule--helper: introduce add-config subcommand Atharva Raykar
  2021-07-22 11:41 ` Atharva Raykar
  2021-07-22 11:50 ` Ævar Arnfjörð Bjarmason
@ 2021-07-22 13:31 ` Atharva Raykar
  2021-07-23 20:36 ` Junio C Hamano
  3 siblings, 0 replies; 7+ messages in thread
From: Atharva Raykar @ 2021-07-22 13:31 UTC (permalink / raw)
  To: Git List
  Cc: Emily Shaffer, Jonathan Nieder, Junio C Hamano, Christian Couder,
	Shourya Shukla, Kaartic Sivaraam, Eric Sunshine,
	Prathamesh Chavan, Đoàn Trần Công Danh,
	Rafael Silva

On 22-Jul-2021, at 16:51, Atharva Raykar <raykar.ath@gmail.com> wrote:
> [...]
> This patch depends on changes introduced in 83913dd5b6 (t7400: test failure to
> add submodule in tracked path, 2021-06-18), which belongs to the
> ar/submodule-add (2021-07-12) series[1].
> 
> This changes in this patch are no different from the one in my first
> submodule-add conversion series[2] which has already got some reviews before.
> 
> [1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/
> [2] https://lore.kernel.org/git/20210615145745.33382-4-raykar.ath@gmail.com/

A correction:

This patch depends on that same topic, but a different commit, which is
559e49fe5c (submodule: prefix die messages with 'fatal', 2021-07-08).

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
  2021-07-22 11:21 [GSoC] [PATCH] submodule--helper: introduce add-config subcommand Atharva Raykar
                   ` (2 preceding siblings ...)
  2021-07-22 13:31 ` Atharva Raykar
@ 2021-07-23 20:36 ` Junio C Hamano
  2021-07-24  9:59   ` Atharva Raykar
  3 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2021-07-23 20:36 UTC (permalink / raw)
  To: Atharva Raykar
  Cc: git, Emily Shaffer, Jonathan Nieder, Christian Couder,
	Shourya Shukla, Kaartic Sivaraam, Eric Sunshine,
	Prathamesh Chavan, Đoàn Trần Công Danh,
	Rafael Silva

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.

> 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?

> +	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.

Other than the "what happens when we see errors?" issue, the patch
looks quite straight-forward rewrite from the scripted version.

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand
  2021-07-23 20:36 ` Junio C Hamano
@ 2021-07-24  9:59   ` Atharva Raykar
  0 siblings, 0 replies; 7+ messages in thread
From: Atharva Raykar @ 2021-07-24  9:59 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Emily Shaffer, Jonathan Nieder, Christian Couder,
	Shourya Shukla, Kaartic Sivaraam, Eric Sunshine,
	Prathamesh Chavan, Đoàn Trần Công Danh,
	Rafael Silva

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.
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-07-24 10:00 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 11:21 [GSoC] [PATCH] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-07-22 11:41 ` Atharva Raykar
2021-07-22 11:50 ` Ævar Arnfjörð Bjarmason
2021-07-22 13:28   ` Atharva Raykar
2021-07-22 13:31 ` Atharva Raykar
2021-07-23 20:36 ` Junio C Hamano
2021-07-24  9:59   ` Atharva Raykar

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git