Re: [PATCH 1/2] Change default merge backend from recursive to ort

From: Elijah Newren <newren@gmail.com>
Date: Aug 2, 2021, 16:33 UTC
Message-ID: <CABPp-BFD4wMyaWwcfjWAUQSV2fTwCKRviengSRsf=LV5u39anw@mail.gmail.com>
In-Reply-To: <87h7g7on6q.fsf@evledraar.gmail.com>

On Mon, Aug 2, 2021 at 9:56 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 22 quoted lines

>
> On Sun, Aug 01 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> > [...]
> > @@ -3968,7 +3968,7 @@ static int do_merge(struct repository *r,
> >       o.branch2 = ref_name.buf;
> >       o.buffer_output = 2;
> >
> > -     if (opts->strategy && !strcmp(opts->strategy, "ort")) {
> > +     if (!opts->strategy || strcmp(opts->strategy, "recursive")) {
> >               /*
> >                * TODO: Should use merge_incore_recursive() and
> >                * merge_switch_to_result(), skipping the call to
>
> I might spot more tiny issues, but it looks like our error messaging
> needs updating for 14c4586c2df (merge,rebase,revert: select ort or
> recursive by config or environment, 2020-11-02).
>
> I.e. we die on "Unknown option for merge-recursive", presumably that
> should be updated to indicate that we might call one of
> merge_recursive() or merge_ort_recursive() now.

Ooh, good catch. I think I'd prefer to reword this to "Unknown strategy option: -X%s"

> And perhaps this in sequencer.c:
>
>     that represents the "current" state for merge-recursive[...]

Yeah, it's just a comment but it should still be updated. I'll s/merge-recursive/the merge machinery/ for this one.

I tried to look for other error messages or comments similar to these two but didn't find anything. I might have missed something, though.

I'll get these fixed up with the next submission.