Re: [PATCH 0/4] Beginning of new merge strategy: New API, empty implementation

From: Elijah Newren <newren@gmail.com>
Date: Oct 27, 2020, 00:52 UTC
Message-ID: <CABPp-BFUS39pmB0S79WfOAiOmeM7gLrvog=b6zYrGRCo_RQr7Q@mail.gmail.com>
In-Reply-To: <20201026215622.2138981-1-jonathantanmy@google.com>

On Mon, Oct 26, 2020 at 2:56 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 21 quoted lines

>
> > Hi,
> >
> > On Wed, Oct 21, 2020 at 6:22 AM Elijah Newren via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > In this series, I try to show the new merge API I have developed in
> > > merge-ort and show how it differs from that provided by merge-recursive. I
> > > do this in four steps, each corresponding to a patch:
> >
> > I should probably call out that even if folks don't have time to
> > review patches, I'm particularly interested in opinions on the
> > following two questions:
> >   * Are the "pull.twohead" and "GIT_TEST_MERGE_ALGORITHM" names in
> > patch 4 good/bad/ugly?  (especially the mapping from "pull" to revert,
> > cherry-pick, rebase, and merge?)
>
> I think GIT_TEST_MERGE_ALGORITHM is fine. I think extending
> "pull.twohead" is ugly - perhaps we could get away with not doing
> anything about it for now, and once this feature is ready, we could add
> a new config parameter specially for this.

Yeah, it is kinda ugly. I was thinking of adding "merge.backend" when I discovered that "pull.twohead" already existed. I didn't know if two configuration knobs doing the same thing was uglier, or using that old ugly misnomer was. Maybe we can use the merge.backend name or something like it once merge-ort is further along.

Show 5 quoted lines

> >   * Is it too weird to have a temporary/hidden builtin, in patch 3?
> > If so, what is a good alternative?
>
> An alternative is to implement it as part of test-tool - see t/helper/
> for more information.

Oh, interesting idea; I hadn't thought of that. However, It seems like a bit of a misfit there too -- fast-rebase isn't used in the testsuite anywhere and I didn't have plans to add it. My plan was for fast-rebase to go away soon (perhaps as soon as the merge-ort implementation lands), probably long before merge-recursive.c goes away. fast-rebase exists just to (1) demonstrate the API and (2) demonstrate some performance benchmarks I used to guide my performance decisions. Once merge-ort has fully landed, reason (2) becomes obsolete. Reason (1) might still exist in part, but hopefully we can start updating sequencer.c to use those ideas and then make fast-rebase fully obsolete.

> I'm not very familiar with the merge code, but overall this looks like a
> good, clear design. Thanks especially for showing how this code can be
> used (in patch 3) and a comparison to the old API (in patch 2).

Thanks for taking a look.  :-)