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, 17:43 UTC
Message-ID: <xmqqim914pfs.fsf@gitster.c.googlers.com>
In-Reply-To: <4a4d9d29-ccba-8d39-a7cd-34b1ba785e22@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 25 quoted lines

> On 12/15/2020 12:53 PM, Elijah Newren via GitGitGadget wrote:
>> From: Elijah Newren <newren@gmail.com>
>> 
>> 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.