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

Elijah Newren wrote:

On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras
[off-list ref] wrote:

quoted

Elijah Newren wrote:

quoted

On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras [off-list ref] wrote:

quoted

quoted

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.

OK, so:

  if (git_xmerge_style > 0 && opts->virtual_ancestor)

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.

Maybe write a test-case and find out?

quoted

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.

quoted

quoted

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 would flip the conditional: if nobody that does lots of criss-crosses
objects...

If we waited for a tiny minority of users to speak up before doing
something we migth wait forever.

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

Well, I just re-sent the patch:

https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/ (local)

Cheers.

-- 
Felipe Contreras