Re: [PATCH 1/3] merge-ort: copy a few small helper functions from merge-recursive.c
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 16, 2020, 20:41 UTC
- Message-ID
- <xmqqlfdx32ln.fsf@gitster.c.googlers.com>
- In-Reply-To
- <CABPp-BG+_4Dv_+HNgQxW8KNe1q04NwuhRpExeOuc=wLeunsj_w@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 15 quoted lines
> I wonder if a slightly different codeflow may be easier to follow
>>
>> struct commit_list *result = NULL;
>> while (list) {
>> struct commit_list *next = list->next;
>> list->next = result;
>> result = list;
>> list = next;
>> }
>> return result;
>>
>> if we were to try improving this for readability?
>
> Looks like Felipe also came up with the same version you did (modulo
> the temporary variable name).Funny if it was sent as a response to the message that already had the same answer...
Show 12 quoted lines
>> I dunno if it matters too much, though. > > Yeah, reverse_commit_list() has been unchanged in merge-recursive.c > since its introduction in August of 2006. The function's purpose > seems obvious from its name, so no one ever needs to look at or modify > the implementation. I'm certain I'll never touch it again. So, I > personally don't care what the particular implementation is, and I'm > happy to take whatever reviewers prefer here. > > Since we have three people weighing in with opinions though -- and on > a point that's irrelevant to me -- do you want to make the call here > Junio?
If I were pressed to give a suggestion, I have two ;-)
I would prefer to see us first find out if all other callers of get_merge_bases() _care_ about a particular order of the resulting list. If they do not care [*1*] and if it seems feasible to teach get_merge_bases() build its return value reversed already without too much extra effort, then the commit list reverser can disappear and get_merge_bases() can be fixed to return its commit list in older-to-newer order.
If the above does not happen, then I'd prefer to see a single commit list reverser in commit.c and have it *shared* between the two merge backends, instead of adding another one in merge-ort.c while leaving the one in merge-recursive.c behind. And the single implementation can be either "copied from merge-recursive as that may be unintuitive or harder to follow but at least we know it is battle tested", or the one we see above. If we were to take the latter, we really need to avoid making stupid mistakes while attempting to replace an existing awkward one with "simplified" version, though.
Sorry for listing even more work, but since you asked ... ;-)
[Footnote]
*1* if those who care actually would benefit if we used older-to-newer order, it would be even better.