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

On Mon, Oct 04 2021, Elijah Newren wrote:

On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

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

Will fix, initially had these built-in fixes split into one commit per
built-in, but decided it was too verbose and squashed them all, and
didn't copyedit the commit message properly.