Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand

From: Rafael Silva <rafaeloliveira.cs@gmail.com>
Date: Jun 14, 2021, 19:51 UTC
Message-ID: <gohp6kh7i0xm08.fsf@cpm12071.fritz.box>
In-Reply-To: <20210614125157.99426-4-raykar.ath@gmail.com>

Atharva Raykar <raykar.ath@gmail.com> writes:

Show 5 quoted lines

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

I do not have enough expertise to judge the entire content of this patch. I would like, however, to propose a slight code change for the sake of readability.

Show 32 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6dffaeb6cb..c4b2aa6537 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +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);
> +
> +	key = xstrfmt("submodule.%s.path", add_data->sm_name);
> +	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
> +	free(key);

This above three lines of code is very similar to the two operations that follows (including the one inside the `if (add_data->branch)` condition. So [ ... ]

Show 9 quoted lines

> +	key = xstrfmt("submodule.%s.url", add_data->sm_name);
> +	config_set_in_gitmodules_file_gently(key, add_data->repo);
> +	free(key);
> +	if (add_data->branch) {
> +		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
> +		config_set_in_gitmodules_file_gently(key, add_data->branch);
> +		free(key);
> +	}
> +

[ ... ] it might be worth to write a small wrapper that will perform: (1) `xstrfmt()` on the specified config section, (2) set the configuration in the file and (3) free()'ing the variable inside the wrapper. Thus, most of these code will become one liners that is easier to read (given the function is properly named :) ).

After abstracting the code on the wrapper, this code will become something like:

    function_properly_named("submodule.%s.path", add_data->sm_name, add_data->sm_path);
    function_properly_named("submodule.%s.url", add_data->sm_name, add_data->repo);
    if (add_data->branch)
         function_properly_named("submodule.%s.branch", add_data->sm_path, add_data->branch);

Just as an example, here's a diff to demonstrate the argument:

-- >8 --

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 65f79fbd53..48ea909f51 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+void add_config_in_submodules_file(const char *keyfmt, const char *submodule,
+			  const char *value)
+{
+	char *key = xstrfmt(keyfmt, submodule);
+	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;
@@ -2955,17 +2963,10 @@ static void configure_added_submodule(struct add_data *add_data)
 	if (run_command(&add_submod))
 		die(_("Failed to add submodule '%s'"), add_data->sm_path);
 
-	key = xstrfmt("submodule.%s.path", add_data->sm_name);
-	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
-	free(key);
-	key = xstrfmt("submodule.%s.url", add_data->sm_name);
-	config_set_in_gitmodules_file_gently(key, add_data->repo);
-	free(key);
-	if (add_data->branch) {
-		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
-		config_set_in_gitmodules_file_gently(key, add_data->branch);
-		free(key);
-	}
+	add_config_in_submodules_file("submodule.%s.path", add_data->sm_name, add_data->sm_path);
+	add_config_in_submodules_file("submodule.%s.url", add_data->sm_name, add_data->repo);
+	if (add_data->branch)
+		add_config_in_submodules_file("submodule.%s.branch", add_data->sm_path, add_data->branch);
 
 	add_gitmodules.git_cmd = 1;
 	strvec_pushl(&add_gitmodules.args,

-- >8 --

A proper name than "add_config_in_submodules_file" should be considered - I'm
not very good in naming things.

These change does (should) not change the behavior of code, even though
I believe it make the code simpler to read, I do not have strong
opinions about it. So, take this proposal as you wish.

-- 
Thanks
Rafael