Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command

From: Elijah Newren <newren@gmail.com>
Date: Jan 11, 2022, 22:25 UTC
Message-ID: <CABPp-BFei07srZBgyKs6HCm+G+hmPR-3_EkKjRK8WwGL1Uf2oA@mail.gmail.com>
In-Reply-To: <220111.86mtk2xb1y.gmgdl@evledraar.gmail.com>

On Tue, Jan 11, 2022 at 12:38 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 9 quoted lines

>
> On Tue, Jan 11 2022, Johannes Schindelin wrote:
>
> > Hi Junio,
> >
> > On Mon, 10 Jan 2022, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>

...

Show 14 quoted lines

> > No, I don't think it is a good idea to keep the original behavior around
> > indefinitely, when it is totally unclear whether there actually _is_ any
> > user of this feature out there.
>
> For what it's worth I've used it for some automation at an ex-employer
> at least once. Grepping through my #*git* IRC logs there's a few
> mentions of it, and likewise for StackOverflow.
>
> I'm not opposed to replacing it, and I think that probably in-the-wild
> users of it are almost certainly just grepping for the conflict markers
> to see if there's conflicts, or parsing which files have them etc.
>
> So if we can provide a better interface that they can use (or even make
> git merge-tree a thin wrapper...).

...

Show 7 quoted lines

> So I'd personally be much more hesitant to remove or change that, but of
> course we might still come up with good reasons for why it's worth it,
> especially with some advertised deprecation period.
>
> And isn't any doubt around that even more reason to just go with
> Elijah's plan of introducing new plumbing? I.e. is it really costing us
> to just leave these "legacy merge" plumbing built-ins

I definitely think it's worth guiding users away from the old `git merge-tree` behavior in documentation (i.e. deprecating it). That may also lead towards its eventual removal, but I'm not as worried about that.

`git merge-recursive` was actively used in various places, including in `git cherry-pick`. I had used it a few times myself in a script. I don't see a need to deprecate it currently, which naturally would push its removal (if anyone is pushing for it) even further away.

> and merge-recursive.c etc. in place?

This, however, is different. I have much stronger opinions here and I do want to eventually remove merge-recursive.c. Definitely not now, because:

  * `ort` has not been the default long enough.  We had one bug
reported this cycle where ort did the wrong thing and using
`recursive` was a good workaround.  There may be a long tail here, so
I'd like to wait a couple years for reports to trickle in.
  * `merge-recursive.c` is still hard-coded in three places in the
code; you can't even set a configuration option to choose merge-ort.c
in those places: builtin/am, builtin/merge-recursive, and
builtin/stash.

More details on that second point: All three of these use merge_recursive_generic() and need that usage to be replaced. It's on my TODO list. Although it might look like a trivial job of just copying merge_recursive_generic() and replace its merge_recursive() call with merge_ort_recursive(), that would only get you something that would die on assert()s in ort's setup. If you removed those assert()s, then you'd merely be copying the bugs in merge_recursive_generic() usage into the new world (and adding one new bug). We should instead fix the problems identified with that usage and those callers during the review of ort. So don't try to do this simplistic translation.