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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 26, 2020, 21:56 UTC
Message-ID: <20201026215622.2138981-1-jonathantanmy@google.com>
In-Reply-To: <CABPp-BEGo8qfaoz6vUs+ktvmNUZdiHjwda468Qkh5uaD-wAd4w@mail.gmail.com>

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

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

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