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

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 9, 2022, 19:08 UTC
Message-ID: <3a89df25-8bfe-ba17-415c-3e2f14e17d61@github.com>
In-Reply-To: <patch-08.24-0f9d313b338-20220309T123321Z-avarab@gmail.com>

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.

Show 10 quoted lines

>  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...

Show 5 quoted lines

> @@ -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