Re: [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code

[Elijah Newren <newren@gmail.com>]

On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Refactor code added in 1c41d2805e4 (unpack_trees_options: free
> messages when done, 2018-05-21) to use a ret/goto pattern, rather than
> duplicating the end cleanup in the function.
>
> This control flow will be matched in subsequent commits by various
> other similar code, which often needs to do more than just call
> unpack_trees_options_release(). Let's change this to consistently use
> the same pattern everywhere.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  merge.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/merge.c b/merge.c
> index 2f618425aff..2e3714ccaa0 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,
>         struct tree_desc t[MAX_UNPACK_TREES];
>         int i, nr_trees = 0;
>         struct lock_file lock_file = LOCK_INIT;
> +       int ret = 0;
>
>         refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
>
> @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,
>
>         if (unpack_trees(nr_trees, t, &opts)) {
>                 rollback_lock_file(&lock_file);
> -               unpack_trees_options_release(&opts);
> -               return -1;
> +               ret = -1;
> +               goto cleanup;
>         }
> -       unpack_trees_options_release(&opts);
>
>         if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> -               return error(_("unable to write new index file"));
> -       return 0;
> +               ret = error(_("unable to write new index file"));
> +
> +cleanup:
> +       unpack_trees_options_release(&opts);
> +       return ret;
>  }
> --
> 2.33.0.1404.g83021034c5d

I would say 'meh'...but the commit message is downright confusing.  It
suggests that subsequent commits, plural, will be modifying this code
further.  There is only one more commit in this series, and looking
ahead, it doesn't even touch this file.  So, there actually isn't any
reason for these changes beyond what we see in this file, at least not
for this series.  And as far as this series is concerned, I think it's
actually less readable.  If there's a subsequent series that will use
this and make further changes I could be convinced, but then I'd say
this commit belongs as part of the later series, not this one.