Re: [PATCH v4 1/3] submodule--helper: refactor module_clone()

From: Junio C Hamano <gitster@pobox.com>
To: Atharva Raykar <raykar.ath@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4 1/3] submodule--helper: refactor module_clone()
Date: Tue, 15 Jun 2021 12:51:28 +0900	[thread overview]
Message-ID: <xmqqzgvrwzsv.fsf@gitster.g> (raw)
In-Reply-To: <20210614125157.99426-2-raykar.ath@gmail.com> (Atharva Raykar's message of "Mon, 14 Jun 2021 18:21:55 +0530")

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

> Separate out the core logic of module_clone() from the flag
> parsing---this way we can call the equivalent of the `submodule--helper
> clone` subcommand directly within C, without needing to push arguments
> in a strvec.
> ---

Missing sign-off?

>  builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
>  1 file changed, 128 insertions(+), 113 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..321b623d81 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  

> +struct module_clone_data {
> +	const char* prefix;

Asterisk sticks to identifier not type in this codebase, i.e.

	const char *prefix;

Other than that, the split makes sense, especially for the purpose
of more easily reusing this helper without having to unparse and
reparse the option strings.