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 12:42 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> 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.

The ones in sequencer.c and builtin/merge.c care about the order, but
they manually reverse it (because they are going to call the merge
machinery).  So, these two would benefit from it being reversed.
However...

notes-merge.c and submodule.c both have subtle dependencies on the
order (in ways that might be buggy).  They could perhaps be taught to
depend on the reversed order, but I'm leery of touching something that
looks possibly buggy (one even has a TODO highlighting it) and
becoming responsible.

get_octopus_merge_bases() depends on the order from get_merge_bases(),
and builtin/merge-base.c depends on that function.  So, the output
order of a command depends on it, which might thus affect user
scripts.

builtin/pull.c also depends on the order returned by get_octopus_merge_bases().

That's enough dependencies that I'm inclined to just leave this side
of things as they are.


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

commit.c seems like a natural location.  I'll insert a patch at the
beginning of the series to move the function there, and just use
Johannes' original version from 2006 as-is -- that'll make it easier
to verify that my patch is simply moving things, anyway.

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