git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Rafael Silva <rafaeloliveira.cs@gmail.com>
To: Atharva Raykar <raykar.ath@gmail.com>
Cc: git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Shourya Shukla <shouryashukla.oo@gmail.com>,
	Prathamesh Chavan <pc44800@gmail.com>
Subject: Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand
Date: Mon, 14 Jun 2021 21:51:53 +0200	[thread overview]
Message-ID: <gohp6kh7i0xm08.fsf@cpm12071.fritz.box> (raw)
In-Reply-To: <20210614125157.99426-4-raykar.ath@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

  reply	other threads:[~2021-06-14 19:54 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-05 11:39 [GSoC] [PATCH 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-05 11:39 ` [GSoC] [PATCH 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-06  3:38   ` Bagas Sanjaya
2021-06-06  9:06     ` Christian Couder
2021-06-05 11:39 ` [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-07  9:24   ` Christian Couder
2021-06-07 11:24     ` Atharva Raykar
2021-06-08  9:56 ` [GSoC] [PATCH v2 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-08  9:56   ` [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-08 12:32     ` Đoàn Trần Công Danh
2021-06-09 10:31       ` Atharva Raykar
2021-06-09 13:06         ` Đoàn Trần Công Danh
2021-06-09 13:10           ` Atharva Raykar
2021-06-09  4:24     ` Junio C Hamano
2021-06-09 10:31       ` Atharva Raykar
2021-06-08  9:56   ` [GSoC] [PATCH v2 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-10  8:39   ` [GSoC] [PATCH v3 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-10  8:39     ` [PATCH v3 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-11  6:10       ` Junio C Hamano
2021-06-11  7:32         ` Atharva Raykar
2021-06-11  7:59           ` Junio C Hamano
2021-06-10  8:39     ` [PATCH v3 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-14 12:51     ` [GSoC] [PATCH v4 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-14 12:51       ` [PATCH v4 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15  3:51         ` Junio C Hamano
2021-06-15  9:03           ` Atharva Raykar
2021-06-14 12:51       ` [PATCH v4 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-14 12:51       ` [PATCH v4 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-14 19:51         ` Rafael Silva [this message]
2021-06-14 20:12           ` Eric Sunshine
2021-06-15  9:37             ` Rafael Silva
2021-06-15  7:09           ` Atharva Raykar
2021-06-15  9:38       ` [GSoC] [PATCH v5 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-15  9:38         ` [PATCH v5 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15  9:38         ` [PATCH v5 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-15  9:38         ` [PATCH v5 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-15 14:57         ` [GSoC] [PATCH v6 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-15 14:57           ` [PATCH v6 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15 14:57           ` [PATCH v6 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-15 14:57           ` [PATCH v6 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=gohp6kh7i0xm08.fsf@cpm12071.fritz.box \
    --to=rafaeloliveira.cs@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.com \
    --cc=raykar.ath@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --subject='Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).