Derrick Stolee <stolee@xxxxxxxxx> writes:
> On 12/15/2020 12:53 PM, Elijah Newren via GitGitGadget wrote:
>> From: Elijah Newren <newren@xxxxxxxxx>
>>
>> In a subsequent commit, we will implement the traditional recursiveness
>> that gave merge-recursive its name, namely merging non-unique
>> merge-bases to come up with a single virtual merge base. Copy a few
>> helper functions from merge-recursive.c that we will use in the
>> implementation.
>
> I'm sure these are copies, but...
>
>> +static struct commit_list *reverse_commit_list(struct commit_list *list)
>> +{
>> + struct commit_list *next = NULL, *current, *backup;
>> + for (current = list; current; current = backup) {
>> + backup = current->next;
>> + current->next = next;
>> + next = current;
>> + }
>
> The naming of 'next' seems backwards to me, since it is really
> the "previous" node. Using something like 'previous' makes it
> clear that you are reversing when you say
>
> current->next = previous;
Hmph. I took "next" commit_list as "list is the original one, and
next is the reversed list, the next generation of what we received".
Calling it "previous" feels even more backwards when you view among
three "struct commit_list *" pointers, one (the one that holds the
eventual return value) is primarily used to denote the resulting
list itself, and the other two are used to point individual elements
on the original list.
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?
I dunno if it matters too much, though.
