I think we close it. If something similar shows up, we can open a new bug.

Can this be resolved, or is there more to fix? Changeset is in main.

pushed in crew (9514cbb6e4f6).

Do we need some other fixes?

No, let's go ahead with this.

@mpm: do you know a better explanation of what the patch does ? or is there some
other fundamental problem ?

@mpm:

Can I push the bdiff patch ? It normalize the hunk it founds by pushing them
toward the end.
That way if you have
a:
x
x
y
x
x

b:
x
x
y
x
y
x
x

the diff will always be the same and not dependent of the context.

I'm not looking for my conversioning control tool to be smart enough to
understand the content of my files. There are enough compilers in the world who
tries to do so. Example #1 from Mads (msg7101) modifies different sets of lines,
so I don't see any reason for it to trigger a conflict.

That said, I'm glade with the patch from tonfa which does solves my problem.

And thus, I consider that issue resolved, any blockers not to push it to -stable ?

I screwed up my local test. I'm getting 'error' here too.

It's correct by the definition of 3-way merge. As far as 3-way merge can see
(and thus any tool you can use with hg), the result 'error' is correct.

And I also consider it the correct behavior for hg. The alternatives are:

a) use a significantly better automated merge algorithm 

b) forego automatic merging

I consider the pain caused by (b) to be MUCH worse than the pain caused by the
relatively rare case of incorrect automated merges. And basically every SCM
since at least RCS has made the same choice.

As for (a), it's not clear that a suitable algorithm exists.

For the record, I _do_ get the result "error" that mpm wants. FWIW that is not
what I want. I think that that is a bug, thus the naming - which now is quite
confusing.

Mpm, just to make sure that I understand you correctly: You consider it OK that
a change explicitly made in the tip changeset silently is removed in a merge
immediately after?

For the record, the test in msg7148 does:

simplemerge("good", "good", "error")

which should give "error", but gives "good".

re msg7148:

Yes, this is a bug. The result should be 'error' with no conflicts. Apparently
simplemerge is busted. Not sure if the same bustedness is to blame for the other
issues though.

I distilled the following test case from a real war story. It touches the same
problem. Should I create another isse for it?

hg init .
echo good > file
hg add file
hg ci -m 'initial good in main branch'
# in other branch
echo error > file
hg ci -m 'error in other branch'
# back to main branch
hg up -C -r 0
echo error >> file
hg ci -m 'error also in main branch'
echo good > file
hg ci -m 'fix error in main branch'
# Merge immediately to make sure the fix is propagated to other branches
hg --debug merge
# Was our fix silently forgotten?
hg diff
# Yes, shit happened when the error was made twice. But that was in the past,
and none of the bad changes had meta data to justify that they override the fix.

I agree.

But there is no such things as a trivial filemerge. You can never trust an
automatic merge. (Sorry for repeating myself, but I really do think that that is
important.)

(Merges between parents who touches distinct sets of files are "more trivial".
In some cases the "trivial" merge will be wrong in this case too, but I do think
that it makes sense to make a pragmatic distinction between these two cases. The
nontrivial merges I focus on happens to be exactly the ones where it makes sense
to use a merge tool to merge and review.)

I think we need here to make the difference between a trivial merge and a non
trivial merge. I want to be able to trust the merge on a file that did not
thrown my merger in the air, and I don't care if the merger is thrown too often ...

I usually check (double check, triple check) my merges, but that one, as said
was a big one, and I only checked the file that had conflicts. Hopefully, the
compilation process told me about that one, but who knows which one did not
triggered a compilation error ...

Anyway, further investigations shown that the problem occurs if the hunk is
wrapped around some similar lines (\n in the posted example) which the algorithm
treat as common context, so, the (idea of the) patch from tonfa seems good to me
(did not reviewed it at all). Running simplemerge.Merge3Text.find_sync_regions
highlight the problem.

Tonfa: Yes, but I think the reasoning goes the other way:
No automated merge tool will do the right thing for both cases, so we _should_
disable (silent!) simplemerge by default.
Or slightly rephrased to emphasize my point: Simplemerge is good (and getting
better), but it shouldn't be silent by default.

So we could disable simplemerge by default, but then be aware that no automated
merge tool will do the right thing for both cases.

mergetest2.hg contains another slightly artificial but not unrealistic scenario.

1.0.1 seems to merge it in strange ways; structurally similar sections are
merged differently. That might be one part of what tonfas patch fixes.

But it is my understanding that with tonfas patch then mercurial intentionally
merges the heads in what turns out to be the "wrong" way.

The following fixes it for me, it "normalize" the diffs (tries to "push" hunk to
the end).

diff --git a/mercurial/bdiff.c b/mercurial/bdiff.c
--- a/mercurial/bdiff.c
+++ b/mercurial/bdiff.c
@@ -240,6 +240,7 @@
 static struct hunklist diff(struct line *a, int an, struct line *b, int bn)
 {
 	struct hunklist l;
+	struct hunk *curr;
 	struct pos *pos;
 	int t;
 
@@ -259,6 +260,30 @@
 	}
 
 	free(pos);
+
+	for (curr = l.base; curr != l.head; curr++) {
+		struct hunk *next = curr+1;
+		int shift = 0;
+
+		if (next == l.head)
+			break;
+
+		if (curr->a2 == next->a1)
+			while (curr->a2+shift < an && curr->b2+shift < bn
+			       && !cmp(a+curr->a2+shift, b+curr->b2+shift))
+				shift++;
+		else if (curr->b2 == next->b1)
+			while (curr->b2+shift < bn && curr->a2+shift < an
+			       && !cmp(b+curr->b2+shift, a+curr->a2+shift))
+				shift++;
+		if (!shift)
+			continue;
+		curr->b2 += shift;
+		next->b1 += shift;
+		curr->a2 += shift;
+		next->a1 += shift;
+	}
+
 	return l;
 }

What platform are you on and which mercurial distribution do you use? And what
merge tool? What is the output of "debugconfig"?


I have tested on a reduced test case with mercurial-1.0.1-4.fc9.i386:

hg clone -r ce0d2ade64f0 ~/Download/mergetest.hg mergetest
cd mergetest

The merge can be repeated and debugged with

hg up -C 5fe2240ba89b
hg --debug -v merge -r 328376af27c1

I get a better - but more manual - merge experience with meld with

hg --config merge-tools.meld.priority=10000 --config
merge-tools.meld.premerge=False merge -r 328376af27c1

I feel lucky and get the merge you expect with kdiff3 with

hg --config merge-tools.kdiff3.priority=10000 --config
merge-tools.kdiff3.premerge=False merge -r 328376af27c1

But that is pure luck; I get a more reliable merge with

hg --debug -v --config merge-tools.kdiff3.premerge=False --config
merge-tools.kdiff3.priority=10000 --config merge-tools.kdiff3.args='$base $local
$other -o $output' merge -r 328376af27c1


My premature conclusion is:

By default mercurial merge uses "premerge"; the builtin simplemerge is tried
first, and if it is successful by its own standards then the external mergetool
isn't used. I would rather call that "try-simplemerge-only" or
"simplemerge-sufficient". I don't think having "premerge" enabled by default is
safe.

kdiff3.args in the sample mergetools.rc uses "--auto" which does the same as
"premerge". Kdiff3 do however use another merge strategy and merges the testcase
different from simplemerge. But I don't think "--auto" is a safe default.

It is good practice to always review a diff before committing. And merges should
be reviewed twice, diffed to both parents. That is even more important when the
default isn't as safe as it could be. This advice could be given in tutorial and
documentation.

You consider the result of simplemerge wrong, and thus it _is_ wrong. But in
other cases exactly the same result could have been the right one. Simplemerge
is ok, but no merge tool is perfect, and no merge tool can be trusted blindly. I
think the primary purpose of merge tools are to visualize what is going on. If
you can't see and understand how and why it has been merged then it doesn't
matter how close to perfect the merge tool is.

By the way: When merging with for example meld then I could like to use the
unperfect result of something like simplemerge (or diff3) to be used as "base";
that is what I would call "premerge".

It would be very convenient if "diff" had a "--switch-parent" option like
"export" has; that would make it easier to make a second review just before
commiting.


Best Regards
Mads
Mercurial user Evaluating SafeSign ;-)

This bug shown up as part of a big merge, some files were conflicting, and some
other not. It occurred that I made exactly the same change than my colleague on
some files (but on a different branch). The changes are about some added lines.
(prototype function declaration in a C header file). When merging those two
branches, the magic occurs.

At the end, this file did not thrown the merge tool in the air, Mercurial
silently performed the merge without any complain, but the merged file has the
change twice. (twice the function declared in my header file).

I managed to reproduce it in the attached repo.

This repository has a couple of heads, my two first try were unsucessfull in
reproducing the bug (Thus the "sucess" commit message), but the two last one
show the bug. ("failed" commit message)

I categorize this bug as critical as it made me lose confidence in the merge
process.