From: Derrick Stolee Date: Wed, 09 Mar 2022 19:08:54 GMT Subject: Re: [PATCH 08/24] revisions API users: add "goto cleanup" for release_revisions() Message-ID: <3a89df25-8bfe-ba17-415c-3e2f14e17d61@github.com> In-Reply-To: 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