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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Oct 04 2021, Elijah Newren wrote:

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

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.