From: Jay Soffian <hidden> Date: 2016-06-15 22:50:52
There's a use case that merge recursive doesn't seem to handle, and I
wonder how difficult it would be to add.
Say you have a merge between OURS and THEIRS, with common ancestor BASE.
Between BASE and THEIRS, a file named header.h has the following changes:
# Rename header.h to header_new.h
git mv header.h header_new.h
# Minor edits to account for the rename such as fixing the
# include guard:
perl -pi -e 's/HEADER_H_/HEADER_NEW_H_/' header_new.h
# Drop a compatibility header.h in place till we can fix all the
# files which include header.h
cat > header.h <<-__EOF__
#ifndef HEADER_H_
#define HEADER_H_
#include "header_hew.h"
#endif // HEADER_H_
__EOF__
git add header.h header_new.h
git commit -m 'rename header.h to header_new.h'
Meanwhile, between BASE and OURS, a few minor changes are made to
header.h. This could be as little as a single line change in the
middle of the header.h.
Now you merge THEIRS to OURS. Git will just show header.h in conflict.
99% of the time I can do the following:
git diff MERGE_BASE... header.h | patch header_new.h
git checkout --theirs header.h
git add header.h header_new.h
But it would seem like this is something merge recursive should be
capable of handling on its own.
j.
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
On Thu, Mar 24, 2011 at 05:18:20PM -0400, Jay Soffian wrote:
There's a use case that merge recursive doesn't seem to handle, and I
wonder how difficult it would be to add.
I don't think it's that hard. In your example:
Say you have a merge between OURS and THEIRS, with common ancestor BASE.
Between BASE and THEIRS, a file named header.h has the following changes:
# Rename header.h to header_new.h
git mv header.h header_new.h
# Minor edits to account for the rename such as fixing the
# include guard:
perl -pi -e 's/HEADER_H_/HEADER_NEW_H_/' header_new.h
# Drop a compatibility header.h in place till we can fix all the
# files which include header.h
cat > header.h <<-__EOF__
#ifndef HEADER_H_
#define HEADER_H_
#include "header_hew.h"
#endif // HEADER_H_
__EOF__
You have a move coupled with a rewritten version of a file. So without
copy or break detection, we won't consider the original header.h as a
possible rename source.
git add header.h header_new.h
git commit -m 'rename header.h to header_new.h'
Meanwhile, between BASE and OURS, a few minor changes are made to
header.h. This could be as little as a single line change in the
middle of the header.h.
Now you merge THEIRS to OURS. Git will just show header.h in conflict.
Right. merge-recursive won't detect the rename here.
In your case, I think merge-recursive doing break detection is the right
solution. It realizes that header.h has been rewritten and considers it
as a source candidate for the rename to header_new.
Copy detection might also work, but I don't think it makes sense in a
merge setting. If I copy "foo.h" to "bar.h", and meanwhile you make a
change to "foo.h", there is no reason to think the change should apply
to bar.h instead of foo.h (you might perhaps think it could apply to
_both_, but that is a different story).
So break detection is the only thing that makes sense to me. This
one-liner does what you want:
And I tested it on this:
-- >8 --
#!/bin/sh
rm -rf repo
git init repo && cd repo
# Sample header.
cp /path/to/your/git/revision.h .
git add revision.h
git commit -m 'add revision.h'
# Move and tweak header.
git mv revision.h foo.h
perl -pi -e 's/REVISION_H/FOO_H/' foo.h
# And put in replacement header.
cat >revision.h <<'EOF'
#ifndef REVISION_H
#define REVISION_H
#include "foo.h"
#endif /* REVISION_H */
EOF
# And commit.
git add revision.h foo.h
git commit -m 'rename revision.h to foo.h'
# Now make a minor change on a side branch.
git checkout -b other HEAD^
sed -i '/REV_TREE_SAME/i/* some comment */' revision.h
git commit -a -m 'tweak revision.h'
git merge master
-- 8< --
It _almost_ works. The merge completes automatically, and the tweak ends
up in foo.h, as you expect. But the merge silently deletes the
placeholder revision.h!
I suspect it is a problem of merge-recursive either not handling the
broken filepair properly, or perhaps reading too much into what a rename
means. I haven't dug further.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
On Fri, Mar 25, 2011 at 05:37:58AM -0400, Jeff King wrote:
It _almost_ works. The merge completes automatically, and the tweak ends
up in foo.h, as you expect. But the merge silently deletes the
placeholder revision.h!
I suspect it is a problem of merge-recursive either not handling the
broken filepair properly, or perhaps reading too much into what a rename
means. I haven't dug further.
Ah, found it. In process_renames, we explicitly call remove_file() on
the source, which is assuming the rename did not come from a broken
pair. What we actually want to do, I think, is to just take the changes
from the renaming side literally. There's no point in doing a 3-way
merge because the other side's changes will end up applied to the rename
destination. It just happens that without break_opt, the renaming sides
change is _always_ a deletion, or else it would not have been a rename
candidate. So the current code is a special case for that rule.
Now, as far as how to do that, I haven't a clue. I've been staring at
merge-recursive code for 30 minutes. ;)
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
On Fri, Mar 25, 2011 at 06:12:04AM -0400, Jeff King wrote:
Ah, found it. In process_renames, we explicitly call remove_file() on
the source, which is assuming the rename did not come from a broken
pair. What we actually want to do, I think, is to just take the changes
from the renaming side literally. There's no point in doing a 3-way
merge because the other side's changes will end up applied to the rename
destination. It just happens that without break_opt, the renaming sides
change is _always_ a deletion, or else it would not have been a rename
candidate. So the current code is a special case for that rule.
Now, as far as how to do that, I haven't a clue. I've been staring at
merge-recursive code for 30 minutes. ;)
@@ -1049,7 +1061,10 @@ static int process_renames(struct merge_options *o,intrenamed_stage=a_renames==renames1?2:3;intother_stage=a_renames==renames1?3:2;-remove_file(o,1,ren1_src,o->call_depth||renamed_stage==2);+update_or_remove(o,+ren1->src_entry->stages[renamed_stage].sha,+ren1->src_entry->stages[renamed_stage].mode,+ren1_src,renamed_stage==3);hashcpy(src_other.sha1,ren1->src_entry->stages[other_stage].sha);src_other.mode=ren1->src_entry->stages[other_stage].mode;
It passes my test, and it doesn't break anything in t/. Yay.
There's one other call to remove_file in process_renames. It's for the
case that both sides renamed the same file to the same destination. I
think there we need to actually compare the two sides. If only one side
still has something at the source path, then we can take that side
(since the other side renamed away the file). But if they both have it
(i.e., they both installed a replacement), then we need to do the usual
3-way merge on that replacement. I'm not sure if we'd have to do that
ourselves, or if we can just punt and the rest of the merge machinery
will handle the entry. I'll have to write some tests, I think.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
On Fri, Mar 25, 2011 at 07:12:25AM -0400, Jeff King wrote:
It passes my test, and it doesn't break anything in t/. Yay.
There's one other call to remove_file in process_renames. It's for the
case that both sides renamed the same file to the same destination. I
think there we need to actually compare the two sides. If only one side
still has something at the source path, then we can take that side
(since the other side renamed away the file). But if they both have it
(i.e., they both installed a replacement), then we need to do the usual
3-way merge on that replacement. I'm not sure if we'd have to do that
ourselves, or if we can just punt and the rest of the merge machinery
will handle the entry. I'll have to write some tests, I think.
OK, I figured it out. I was thrown off by test failures in t3030, but I
think that test is actually wrong; it documents what happens, but not
really what we _want_ to have happen.
So this is the patch series I ended up with:
[1/3]: t3030: fix accidental success in symlink rename
[2/3]: merge: handle renames with replacement content
[3/3]: merge: turn on rewrite detection
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
In this test, we have merge two branches. On one branch, we
renamed "a" to "e". On the other, we renamed "a" to "e" and
then added a symlink pointing at "a" pointing to "e".
The results for the test indicate that the merge should
succeed, but also that "a" should no longer exist. Since
both sides renamed "a" to the same destination, we will end
up comparing those destinations for content.
But what about what's left? One side (the rename only),
replaced "a" with nothing. The other side replaced it with a
symlink. The common base must also be nothing, because any
"a" before this was meaningless (it was totally unrelated
content that ended up getting renamed).
The only sensible resolution is to keep the symlink. The
rename-only side didn't touch the content versus the common
base, and the other side added content. The 3-way merge
dictates that we take the side with a change.
And this gives the overall merge an intuitive result. One
side made one change (a rename), and the other side made two
changes: an identical rename, and an addition (that just
happened to be at the same spot). The end result should
contain both changes.
Signed-off-by: Jeff King <redacted>
---
Ken, I'm cc'ing you as the test in question is yours. The
context is that I want to turn on break detection in
merge-recursive, but doing so makes your test fail.
t/t3030-merge-recursive.sh | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
We generally think of a rename as removing one path entirely
and placing similar content at a new path. In other words,
after a rename, the original path is now empty. But that is
not necessarily the case with rewrite detection (which is
not currently possible to do for merge-recursive).
The current merge code blindly removes paths that are used
as rename sources; however, we should check to see if there
is useful content at that path. There are basically two
interesting cases:
1. One side renames a path, but also puts new content
(or a symlink) at the same path. We want to detect the
rename, and have changes from the other side applied to
the rename destination. The new content at the original
path should be left untouched.
The current code just calls remove_file, but that
ignores the concept that the renaming side may put
something else useful there. We should detect this case
and either remove (if no new content), or put the new
content in place at the original path.
2. Both sides renamed and installed new content at the
original path. If they didn't rename to the same
destination, it is a conflict, and we already mark it
as such. But if it's the same destination, then it's
not a conflict; the renamed content will be merged at
the new destination.
For the new content at the original path, we have to do
a 3-way merge. The base must be the null sha1, because
this "slot" for content didn't exist before (it was
taken up by the content which got renamed away). So if
only one side installed new content, that content
automatically wins. If both sides did, and it is the
same content, then that content is OK. But if the
content is different, then we have a conflict and
should do the usual conflict-markers thing.
This patch implements the semantics described above, which
lays the groundwork for turning on rewrite detection.
Signed-off-by: Jeff King <redacted>
---
I split this one out for easier review. The semantics I am
proposing are a superset of the current "remove" behavior (it's just
that without break detection on, you can't trigger the other cases). But
by putting this in before enabling break detection, you can test easily
that the new code isn't breaking any existing behavior.
merge-recursive.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 63 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:50:52
We currently don't do break-detection at all in
merge-recursive. But there are some cases where it would
provide a more useful merge result.
For example, consider this case (which is the basis for the
new tests in t6039):
1. You rename a header file foo.h to bar.h. You install a
new foo.h that includes bar.h (for compatibility).
2. Another branch makes changes to foo.h.
When you merge, you want the changes the other branch made
to foo.h to migrate to the rename destination, bar.h, just
as you would if you hadn't installed that compatibility
header.
Similarly, you want the compatibility header left untouched.
The other side's changes all ended up in bar.h, so there is
no reason to conflict with the new content in foo.h.
This patch turns on break detection for merge-recursive. In
addition to new tests in t6039, it makes a similar test in
t3030 pass.
Signed-off-by: Jeff King <redacted>
---
I hope the tests are readable. I thought it was important to test the
merges in both directions (because an early iteration screwed that up),
which led to factoring out a lot of the setup and checking code.
merge-recursive.c | 1 +
t/t3030-merge-recursive.sh | 2 +-
t/t6039-merge-break.sh | 174 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 176 insertions(+), 1 deletions(-)
create mode 100755 t/t6039-merge-break.sh
@@ -0,0 +1,174 @@+#!/bin/sh++test_description='mergingwithrenamesfrombrokenpairs++Thisisbasedonareal-worldpracticeofmovingaheaderfiletoa+newlocation,butinstallinga"replacement"filethatpointsto+theoldone.Weneedbreakdetectioninthemergetofindthe+rename.+'+../test-lib.sh++# A fake header file; it needs a fair bit of content+# for break detection and inexact rename detection to work.+mksample(){+echo'#ifndef SAMPLE_H'+echo'#define SAMPLE_H'+foriin01234;do+forjin0123456789;do+echo"extern fun$i$j();"+done+done+echo'#endif /* SAMPLE_H */'+}++mvsample(){+sed's/SAMPLE_H/NEW_H/'"$1">"$2"&&+rm"$1"+}++# A replacement sample header file that references a new one.+mkreplacement(){+echo'#ifndef SAMPLE_H'+echo'#define SAMPLE_H'+echo"#include \"$1\""+echo'#endif /* SAMPLE_H */'+}++# Tweak the header file in a minor way.+tweak(){+sed's,42.*,& /* secret of something-or-other */,'"$1">"$1.tmp"&&+mv"$1.tmp""$1"+}++reset(){+gitreset--hard&&+gitcheckoutmaster&&+gitreset--hardbase&&+gitclean-f&&+{gitbranch-Dtopic||true;}+}++test_expect_success'setup baseline''+mksample>sample.h&&+gitaddsample.h&&+gitcommit-m"add sample.h"&&+gittagbase+'++setup_rename_plus_tweak(){+reset&&+mvsamplesample.hnew.h&&+mkreplacementnew.h>sample.h&&+gitaddsample.hnew.h&&+gitcommit-m'rename sample.h to new.h, with replacement'&&+gitcheckout-btopicbase&&+tweaksample.h&&+gitcommit-a-m'tweak sample.h'+}++check_tweak_result(){+mksample>expect.orig&&+mvsampleexpect.origexpect&&+tweakexpect&&+test_cmpexpectnew.h&&+mkreplacementnew.h>expect&&+test_cmpexpectsample.h+}++test_expect_success'merge rename to tweak finds rename''+setup_rename_plus_tweak&&+gitmergemaster&&+check_tweak_result+'++test_expect_success'merge tweak to rename finds rename''+setup_rename_plus_tweak&&+gitcheckoutmaster&&+gitmergetopic&&+check_tweak_result+'++setup_double_rename_one_replacement(){+setup_rename_plus_tweak&&+mvsamplesample.hnew.h&&+gitaddnew.h&&+gitcommit-a-m'rename sample.h to new.h (no replacement)'+}++test_expect_success'merge rename to rename/tweak (one replacement)''+setup_double_rename_one_replacement&&+gitmergemaster&&+check_tweak_result+'++test_expect_success'merge rename/tweak to rename (one replacement)''+setup_double_rename_one_replacement&&+gitcheckoutmaster&&+gitmergetopic&&+check_tweak_result+'++setup_double_rename_two_replacements_same(){+setup_rename_plus_tweak&&+mvsamplesample.hnew.h&&+mkreplacementnew.h>sample.h&&+gitaddsample.hnew.h&&+gitcommit-m'rename sample.h to new.h with replacement (same)'+}++test_expect_success'merge rename to rename/tweak (two replacements, same)''+setup_double_rename_two_replacements_same&&+gitmergemaster&&+check_tweak_result+'++test_expect_success'merge rename/tweak to rename (two replacements, same)''+setup_double_rename_two_replacements_same&&+gitcheckoutmaster&&+gitmergetopic&&+check_tweak_result+'++setup_double_rename_two_replacements_diff(){+setup_rename_plus_tweak&&+mvsamplesample.hnew.h&&+mkreplacementdiff.h>sample.h&&+gitaddsample.hnew.h&&+gitcommit-m'rename sample.h to new.h with replacement (diff)'+}++test_expect_success'merge rename to rename/tweak (two replacements, diff)''+setup_double_rename_two_replacements_diff&&+test_must_failgitmergemaster&&+cat>expect<<-\EOF&&+#ifndef SAMPLE_H+#define SAMPLE_H+<<<<<<<HEAD+#include "diff.h"+=======+#include "new.h"+>>>>>>>master+#endif /* SAMPLE_H */+EOF+test_cmpexpectsample.h+'++test_expect_success'merge rename to rename/tweak (two replacements, diff)''+setup_double_rename_two_replacements_diff&&+gitcheckoutmaster&&+test_must_failgitmergetopic&&+cat>expect<<-\EOF&&+#ifndef SAMPLE_H+#define SAMPLE_H+<<<<<<<HEAD+#include "new.h"+=======+#include "diff.h"+>>>>>>>topic+#endif /* SAMPLE_H */+EOF+test_cmpexpectsample.h+'++test_done
From: Jeff King <hidden> Date: 2016-06-15 22:54:15
On Mon, Jul 16, 2012 at 12:17:26AM +0000, Techlive Zheng wrote:
So, Is there any progress on these patches, I am currently need this
functionality very much, will these be merged into master?
No. Turning on break detection in merge-recursive triggered bugs
elsewhere in merge-recursive. See these followup posts:
http://article.gmane.org/gmane.comp.version-control.git/175932http://article.gmane.org/gmane.comp.version-control.git/175990
The merge-recursive code is sufficiently horrific that I have been
successfully putting off digging back into the problem for a whole
year. :)
Until those problems are resolved, the patches have too many regressions
to go into master.
-Peff
PS If you are going to reply to a year-old thread, it is probably a good
idea to give some context in your message, and to cc the involved
parties. Most of us use threaded mail readers, but not everybody keeps a
year of archives around. The original discussion and patches were here:
http://thread.gmane.org/gmane.comp.version-control.git/169944
From: Jay Soffian <hidden> Date: 2016-08-11 17:46:29
On Fri, Mar 25, 2011 at 12:00 PM, Jeff King [off-list ref] wrote:
OK, I figured it out. I was thrown off by test failures in t3030, but I
think that test is actually wrong; it documents what happens, but not
really what we _want_ to have happen.
So this is the patch series I ended up with:
[1/3]: t3030: fix accidental success in symlink rename
[2/3]: merge: handle renames with replacement content
[3/3]: merge: turn on rewrite detection
I read through all three of these and, from my superficial
understanding of the merge code, they look correct.
I'll test these out on some actual merges as soon as I can. (Probably
next week.)
Thank you for this series.