On Mon, Oct 04 2021, Elijah Newren wrote:
> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
