Re: [PATCH v7 04/20] submodule--helper: run update using child process struct

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Feb 10 2022, Glen Choo wrote:

> From: Atharva Raykar <raykar.ath@gmail.com>
>
> We switch to using the run-command API function that takes a
> 'struct child process', since we are using a lot of the options. This
> will also make it simple to switch over to using 'capture_command()'
> when we start handling the output of the command completely in C.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Shourya Shukla <periperidip@gmail.com>
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/submodule--helper.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 09cda67c1e..db71e6f4ec 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2344,47 +2344,45 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
>  
>  static int run_update_command(struct update_data *ud, int subforce)
>  {
> -	struct strvec args = STRVEC_INIT;
> -	struct strvec child_env = STRVEC_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
>  	char *oid = oid_to_hex(&ud->oid);
>  	int must_die_on_failure = 0;
> -	int git_cmd;
>  
>  	switch (ud->update_strategy.type) {
>  	case SM_UPDATE_CHECKOUT:
> -		git_cmd = 1;
> -		strvec_pushl(&args, "checkout", "-q", NULL);
> +		cp.git_cmd = 1;
> +		strvec_pushl(&cp.args, "checkout", "-q", NULL);
>  		if (subforce)
> -			strvec_push(&args, "-f");
> +			strvec_push(&cp.args, "-f");
>  		break;
>  	case SM_UPDATE_REBASE:
> -		git_cmd = 1;
> -		strvec_push(&args, "rebase");
> +		cp.git_cmd = 1;
> +		strvec_push(&cp.args, "rebase");
>  		if (ud->quiet)
> -			strvec_push(&args, "--quiet");
> +			strvec_push(&cp.args, "--quiet");
>  		must_die_on_failure = 1;
>  		break;
>  	case SM_UPDATE_MERGE:
> -		git_cmd = 1;
> -		strvec_push(&args, "merge");
> +		cp.git_cmd = 1;
> +		strvec_push(&cp.args, "merge");
>  		if (ud->quiet)
> -			strvec_push(&args, "--quiet");
> +			strvec_push(&cp.args, "--quiet");
>  		must_die_on_failure = 1;
>  		break;
>  	case SM_UPDATE_COMMAND:
> -		git_cmd = 0;
> -		strvec_push(&args, ud->update_strategy.command);
> +		cp.use_shell = 1;
> +		strvec_push(&cp.args, ud->update_strategy.command);
>  		must_die_on_failure = 1;
>  		break;
>  	default:
>  		BUG("unexpected update strategy type: %s",
>  		    submodule_strategy_to_string(&ud->update_strategy));
>  	}
> -	strvec_push(&args, oid);
> +	strvec_push(&cp.args, oid);
>  
> -	prepare_submodule_repo_env(&child_env);
> -	if (run_command_v_opt_cd_env(args.v, git_cmd ? RUN_GIT_CMD : RUN_USING_SHELL,
> -				     ud->sm_path, child_env.v)) {
> +	cp.dir = xstrdup(ud->sm_path);
> +	prepare_submodule_repo_env(&cp.env_array);
> +	if (run_command(&cp)) {
>  		switch (ud->update_strategy.type) {
>  		case SM_UPDATE_CHECKOUT:
>  			printf(_("Unable to checkout '%s' in submodule path '%s'"),

If this series is re-arranged so that this comes first, this compiles
just fine & passes all tests.

So I wonder if we can peel this and perhaps other such easy to review
prep changes off into its own series, since this one has grown to 20
patches.

We could then hopefully fast-track those easy-to-review prep changes,
which would make the "real" series to follow smaller and easier to
review/grok.