Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle

[Elijah Newren <newren@gmail.com>]

On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
> > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
> > > Or just use the base of the virtual merge:
> > >
> > >   <<<<<<< HEAD
> > >   D
> > >   ||||||| merged common ancestors
> > >   1
> > >   =======
> > >   C
> > >   >>>>>>> C
> >
> > I think that implementing this choice would look like this (again, not
> > compiled or tested and I'm not familiar with xdiff so take it with a
> > big grain of salt):
> >
> >
> > diff --git a/ll-merge.c b/ll-merge.c
> > index 095a4d820e..dbc7f76951 100644
> > --- a/ll-merge.c
> > +++ b/ll-merge.c
> > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
> >       memset(&xmp, 0, sizeof(xmp));
> >       xmp.level = XDL_MERGE_ZEALOUS;
> >       xmp.favor = opts->variant;
> > +     if (git_xmerge_style >= 0 && opts->virtual_ancestor)
> > +             xmp.favor = XDL_MERGE_FAVOR_BASE;
>
> The only time git_xmerge_style isn't >= 0 is when no merge style has
> been configured by the user.

Yep, probably should have just been

+     if (opts->virtual_ancestor)
+             xmp.favor = XDL_MERGE_FAVOR_BASE;

Though the difference doesn't matter a lot.  Since
merge.conflictStyle=merge (which is the current default) doesn't
display the contents from the merge base in a three-way content merge,
setting xmp.favor to XDL_MERGE_FAVOR_BASE vs. leaving it as 0 for the
recursive/intermediate merges won't generally end up affecting the end
result.  It'd only matter for diff3 and zdiff3 users.


Going on a slight tangent, I think there's actually a related bug
here.  We probably should not honor XDL_MERGE_FAVOR_{OURS,THEIRS} when
opts->virtual_ancestor is true; that's just asking for trouble.  I
think it'd paradoxically result in reversing the desired behavior
(e.g. users would see what they'd consider XDL_MERGE_FAVOR_THEIRS
behavior when they asked for XDL_MERGE_FAVOR_OURS) in some cases as a
result.

> In fact, I don't see why any style should change that desired behavior.
> If you said there's issues with the "merge" style too, perhaps the above
> will help for those cases too.
>
> > > We don't have to use diff3 all the way.
> >
> > Right, thus my mention in the other email to consider adding a
> > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third
> > option, and which I've now given at least a partial patch for.  Not
> > sure if it's a crazy idea or a great idea, since I don't do very many
> > criss-cross merges myself.
>
> I thought you meant as a separate configurable flag, not something done
> by default.
>
> Now that I understand what you meant I think it could be a great idea.

If someone that does lots of criss-cross merges can comment on the
idea, and agree that it's worth a shot, I can try to turn it into real
patches.

(I might even try to investigate the zdiff3 stuff too, which sounds
like something I've wanted many times...but I'd really rather
concentrate on merge-ort until its upstreaming is finished.)