Re: [PATCH 19/20] submodule--helper: don't exit() on failure, return

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Change code downstream of module_update() to short-circuit and return
> to the top-level on failure, rather than calling exit().
>
> To do so we need to diligently check whether we "must_die_on_failure",
> which is a pattern started in c51f8f94e5b (submodule--helper: run
> update procedures from C, 2021-08-24), but which hadn't been completed
> to the point where we could avoid calling exit() here.
>
> This introduces no functional changes, but makes it easier to both
> call these routines as a library in the future, and to eventually
> avoid leaking memory.
>
> This and similar control flow in submodule--helper.c could be made
> simpler by properly "libifying" it, i.e. to have it consistently
> return -1 on failures, and to early return on any non-success.
>
> But let's leave that larger project for now, and (mostly) emulate what
> were doing with the "exit(128)" before this change.

Thanks! This is a lot clearer. I'm still kind of iffy on this change
though. On the one hand, we know for sure that we're doing the right
thing because we have tests for "git submodule update --recursive". But
on the other, we return 128 from so deep inside the call chain that I
don't think it's easy for a casual reader to convince themselves that
this is indeed the same.

If the "libification" isn't too hard, I'd strongly prefer to have that
now, but since we already have tests in place, I don't feel strongly
enough to block this patch.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 68aa10a26cd..d3f22f03766 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2121,7 +2121,8 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
>  	return run_command(&cp);
>  }
>  
> -static int run_update_command(struct update_data *ud, int subforce)
> +static int run_update_command(struct update_data *ud, int subforce,
> +			      int *must_die_on_failurep)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	char *oid = oid_to_hex(&ud->oid);
> @@ -2183,8 +2184,10 @@ static int run_update_command(struct update_data *ud, int subforce)
>  			BUG("unexpected update strategy type: %s",
>  			    submodule_strategy_to_string(&ud->update_strategy));
>  		}
> -		if (must_die_on_failure)
> -			exit(128);
> +		if (must_die_on_failure) {
> +			*must_die_on_failurep = 1;
> +			return 128;
> +		}
>  
>  		/* the command failed, but update must continue */
>  		return 1;
> @@ -2218,7 +2221,8 @@ static int run_update_command(struct update_data *ud, int subforce)
>  	return 0;
>  }
>  
> -static int run_update_procedure(struct update_data *ud)
> +static int run_update_procedure(struct update_data *ud,
> +				int *must_die_on_failure)
>  {
>  	int subforce = is_null_oid(&ud->suboid) || ud->force;
>  
> @@ -2245,7 +2249,7 @@ static int run_update_procedure(struct update_data *ud)
>  			    ud->displaypath, oid_to_hex(&ud->oid));
>  	}
>  
> -	return run_update_command(ud, subforce);
> +	return run_update_command(ud, subforce, must_die_on_failure);
>  }
>  
>  static const char *remote_submodule_branch(const char *path)
> @@ -2381,7 +2385,8 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
>  				    "--no-single-branch");
>  }
>  
> -static int update_submodule(struct update_data *update_data)
> +static int update_submodule(struct update_data *update_data,
> +			    int *must_die_on_failure)
>  {
>  	int ret = 1;
>  
> @@ -2420,8 +2425,13 @@ static int update_submodule(struct update_data *update_data)
>  	}
>  
>  	if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) {
> -		if (run_update_procedure(update_data))
> +		ret = run_update_procedure(update_data, must_die_on_failure);
> +		if (ret && *must_die_on_failure) {
> +			goto cleanup;
> +		} else if (ret) {
> +			ret = 1;
>  			goto cleanup;
> +		}
>  	}
>  
>  	if (update_data->recursive) {
> @@ -2444,7 +2454,8 @@ static int update_submodule(struct update_data *update_data)
>  		die_message(_("Failed to recurse into submodule path '%s'"),
>  			    update_data->displaypath);
>  		if (ret == 128) {
> -			exit(ret);
> +			*must_die_on_failure = 1;
> +			goto cleanup;
>  		} else if (ret) {
>  			ret = 1;
>  			goto cleanup;
> @@ -2482,13 +2493,18 @@ static int update_submodules(struct update_data *update_data)
>  
>  	for (i = 0; i < suc.update_clone_nr; i++) {
>  		struct update_clone_data ucd = suc.update_clone[i];
> +		int code;
> +		int must_die_on_failure = 0;
>  
>  		oidcpy(&update_data->oid, &ucd.oid);
>  		update_data->just_cloned = ucd.just_cloned;
>  		update_data->sm_path = ucd.sub->path;
>  
> -		if (update_submodule(update_data))
> -			res = 1;
> +		code = update_submodule(update_data, &must_die_on_failure);
> +		if (code)
> +			res = code;
> +		if (must_die_on_failure)
> +			goto cleanup;
>  	}
>  
>  cleanup:
> -- 
> 2.37.1.1167.g38fda70d8c4