Elijah Newren wrote:
On Fri, Jun 11, 2021 at 8:32 AM Felipe Contreras [off-list ref]
wrote:
quoted
Sergey Organov wrote:
quoted
Junio C Hamano [off-list ref] writes:
quoted
git init repo &&
cd repo &&
echo 1 > content &&
git add content &&
git commit -m 1 content &&
git checkout -b A master &&
echo A > content &&
git commit -m A content &&
git checkout -b B master &&
echo B > content &&
git commit -m B content &&
git checkout -b C A &&
git rev-parse B >.git/MERGE_HEAD &&
echo C > content &&
git commit -m C -a &&
git checkout -b D A &&
git rev-parse B >.git/MERGE_HEAD &&
echo D > content &&
git commit -m D -a &&
git -c merge.conflictstyle=diff3 merge -m final C &&
cat content
Right, here you do not have a unique merge base; you have two of them: A &
B (it's possible to have three or more as well). To do a three-way merge,
you need a single base commit. So, whenever you have more than one merge
base, both merge-recursive and merge-ort will merge the merge bases to get
a virtual merge base. There's always a risk that the merge bases don't
have a unique merge base either, forcing the algorithm to recurse. This
behavior is where the merge algorithm 'recursive' got its name from (and
which also appears in ort's name -- "Ostensibly Recursive's Twin").
You could decide to just pick one of the merge-bases at random, and yield a
different set of surprises including silently merging in favor of one side
when the two sides did things differently. That's problematic.
Instead of using a merge base (a recent-as-possible common commit), you
could decide to instead just try to find a unique common base, regardless
of how ancient it is. Using ancient commits as the base is a step towards
just doing a two-way merge (treating the histories as completely
independent and throwing merge conflicts whenever any files aren't
identical on the two sides). Sure, it's not as bad, but it does yield
massive amounts of useless conflicts. So this is problematic too.
The alternative to the above two options was the
make-a-virtual-merge-base-by-merging-merge-bases strategy. It apparently
was very successful.
OK. That makes sense.
But it does mean that merge bases can have conflict markers in them.
But why? And even if they do, why do they have to be diff3 conflict
markers?
This would be more human-friendly:
<<<<<<< HEAD
D
||||||| merged common ancestors
<<<<<<<<< Temporary merge branch 1
B
=========
A
>>>>>>>>> Temporary merge branch 2
=======
C
>>>>>>> C
Or just put a stub conflict marker:
<<<<<<< HEAD
D
||||||| merged common ancestors
<<<<<<<<< Temporary merge >>>>>>>>>
=======
C
>>>>>>> C
Or just use the base of the virtual merge:
<<<<<<< HEAD
D
||||||| merged common ancestors
1
=======
C
>>>>>>> C
We don't have to use diff3 all the way.
Cheers.
--
Felipe Contreras
On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras [off-list ref] wrote:
Elijah Newren wrote:
quoted
On Fri, Jun 11, 2021 at 8:32 AM Felipe Contreras [off-list ref]
wrote:
...
quoted
The alternative to the above two options was the
make-a-virtual-merge-base-by-merging-merge-bases strategy. It apparently
was very successful.
OK. That makes sense.
quoted
But it does mean that merge bases can have conflict markers in them.
But why? And even if they do, why do they have to be diff3 conflict
markers?
This could be changed; I suspect it just was a natural consequence of how
the code was built. (Recursive means there's not a separate code-path for
merging the merge-bases, so they get the same merge style by default.)
This would be more human-friendly:
<<<<<<< HEAD
D
||||||| merged common ancestors
<<<<<<<<< Temporary merge branch 1
B
=========
A
>>>>>>>>> Temporary merge branch 2
=======
C
>>>>>>> C
I suspect that would be as easy as this (not compiled or tested):
diff --git a/ll-merge.c b/ll-merge.c
index 095a4d820e..bdd129cbd6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -131,7 +131,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
xmp.level = XDL_MERGE_ZEALOUS;
xmp.favor = opts->variant;
xmp.xpp.flags = opts->xdl_opts;
- if (git_xmerge_style >= 0)
+ if (git_xmerge_style >= 0 && !opts->virtual_ancestor)
xmp.style = git_xmerge_style;
if (marker_size > 0)
xmp.marker_size = marker_size;
Or just put a stub conflict marker:
<<<<<<< HEAD
D
||||||| merged common ancestors
<<<<<<<<< Temporary merge >>>>>>>>>
=======
C
>>>>>>> C
I don't know what would be involved to do this one; I think it wouldn't
be too hard, but I don't think we'd want to pursue this option.
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;
xmp.xpp.flags = opts->xdl_opts;
if (git_xmerge_style >= 0)
xmp.style = git_xmerge_style;
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 8629ae287c..b8d1a536c2 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -62,6 +62,7 @@ extern "C" {
#define XDL_MERGE_FAVOR_OURS 1
#define XDL_MERGE_FAVOR_THEIRS 2
#define XDL_MERGE_FAVOR_UNION 3
+#define XDL_MERGE_FAVOR_BASE 4
/* merge output styles */
#define XDL_MERGE_DIFF3 1diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 95871a0b6e..a8dc42595a 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -313,6 +313,9 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
if (m->mode & 2)
size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,
dest ? dest + size : NULL);
+ } else if (m->mode == 4) {
+ size += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 0,
+ dest ? dest + size : NULL);
} else
continue;
i = m->i1 + m->chg1;
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.
Hope that helps,
Elijah
Elijah Newren wrote:
On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras [off-list ref] wrote:
quoted hunk
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.
I don't see why this:
git -c merge.conflictstyle=merge merge
Should have a different behavior than this:
git merge
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
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.
Cheers.
--
Felipe Contreras
On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras
[off-list ref] wrote:
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.
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.
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 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.)
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/
Cheers.
--
Felipe Contreras