Elijah Newren <newren@xxxxxxxxx> writes:
> 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...
>> 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.
