Re: [PATCH 1/3] merge-ort: copy a few small helper functions from merge-recursive.c

[Elijah Newren <newren@gmail.com>]

On Wed, Dec 16, 2020 at 9:43 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Derrick Stolee <stolee@gmail.com> writes:
>
> > 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?

Looks like Felipe also came up with the same version you did (modulo
the temporary variable name).

> 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?

Otherwise I'll just leave it as-is and not update further.