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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

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

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

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

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