Subject: Re: [PATCH 08/24] revisions API users: add "goto cleanup" for release_revisions()

From: Derrick Stolee <[email-masked]>
Date: Wed, 9 Mar 2022 14:08:54 -0500

On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:
> Add a release_revisions() to various users of "struct rev_list" which
> requires a minor refactoring to a "goto cleanup" pattern to use that
> function.

A straight-forward refactor. I have a couple nits on this one.

>  static int do_store_stash(const struct object_id *w_commit, const char *stash_msg,
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 09db2620829..19393da4e31 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1222,6 +1222,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
>  	struct strvec diff_args = STRVEC_INIT;
>  	struct rev_info rev;
>  	struct module_cb_list list = MODULE_CB_LIST_INIT;
> +	int ret = 0;

Since we initialize ret = 0 here...

> @@ -1259,9 +1262,11 @@ static int compute_summary_module_list(struct object_id *head_oid,
>  	else
>  		run_diff_files(&rev, 0);
>  	prepare_submodule_summary(info, &list);
> +	ret = 0;

... this is extraneous.

>  		die(_("unable to write %s"), get_index_file());
> -
> +	ret = (result.clean == 0);
> +cleanup:

Here (and other places) I feel the loss of this empty line
above the goto label. It just seems helpful to help see
these labels a little more clearly if they have an empty
line first, instead of immediately following a statement.

Thanks,
-Stolee