merge recursive and code movement

merge recursive and code movement

[Jay Soffian]

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.

Re: merge recursive and code movement

[Jeff King]

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:

diff --git a/merge-recursive.c b/merge-recursive.c
index 2a048c6..ed574e6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -429,6 +429,7 @@ static struct string_list *get_renames(struct merge_options *o,
 			    1000;
 	opts.rename_score = o->rename_score;
 	opts.show_rename_progress = o->show_rename_progress;
+	opts.break_opt = 0;
 	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	if (diff_setup_done(&opts) < 0)
 		die("diff setup failed");

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

Re: merge recursive and code movement

[Jeff King]

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

Re: merge recursive and code movement

[Jeff King]

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

So this is what I ended up with:

diff --git a/merge-recursive.c b/merge-recursive.c
index 8e82a8b..af42530 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -426,6 +426,7 @@ static struct string_list *get_renames(struct merge_options *o,
 			    1000;
 	opts.rename_score = o->rename_score;
 	opts.show_rename_progress = o->show_rename_progress;
+	opts.break_opt = 0;
 	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	if (diff_setup_done(&opts) < 0)
 		die("diff setup failed");
@@ -706,6 +707,17 @@ static void update_file(struct merge_options *o,
 	update_file_flags(o, sha, mode, path, o->call_depth || clean, !o->call_depth);
 }
 
+static int update_or_remove(struct merge_options *o,
+			    const unsigned char *sha1, unsigned mode,
+			    const char *path, int update_wd)
+{
+	if (is_null_sha1(sha1))
+		return remove_file(o, 1, path, !update_wd);
+
+	update_file_flags(o, sha1, mode, path, 1, update_wd);
+	return 0;
+}
+
 /* Low level file merging, update and removal */
 
 struct merge_file_info {
@@ -1049,7 +1061,10 @@ static int process_renames(struct merge_options *o,
 			int renamed_stage = a_renames == renames1 ? 2 : 3;
 			int other_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

Re: merge recursive and code movement

[Jeff King]

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

[PATCH 1/3] t3030: fix accidental success in symlink rename

[Jeff King]

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 <peff@peff.net>
---
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(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 34794f8..e686f04 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -267,7 +267,8 @@ test_expect_success 'setup 8' '
 		ln -s e a &&
 		git add a e &&
 		test_tick &&
-		git commit -m "rename a->e, symlink a->e"
+		git commit -m "rename a->e, symlink a->e" &&
+		oln=`printf e | git hash-object --stdin`
 	fi
 '
 
@@ -630,16 +631,18 @@ test_expect_success 'merge-recursive copy vs. rename' '
 
 if test_have_prereq SYMLINKS
 then
-	test_expect_success 'merge-recursive rename vs. rename/symlink' '
+	test_expect_failure 'merge-recursive rename vs. rename/symlink' '
 
 		git checkout -f rename &&
 		git merge rename-ln &&
 		( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
 		(
+			echo "120000 blob $oln	a"
 			echo "100644 blob $o0	b"
 			echo "100644 blob $o0	c"
 			echo "100644 blob $o0	d/e"
 			echo "100644 blob $o0	e"
+			echo "120000 $oln 0	a"
 			echo "100644 $o0 0	b"
 			echo "100644 $o0 0	c"
 			echo "100644 $o0 0	d/e"
-- 
1.7.4.41.g423da.dirty

[PATCH 2/3] merge: handle renames with replacement content

[Jeff King]

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 <peff@peff.net>
---
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(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 8e82a8b..16263b0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -706,6 +706,64 @@ static void update_file(struct merge_options *o,
 	update_file_flags(o, sha, mode, path, o->call_depth || clean, !o->call_depth);
 }
 
+static int update_or_remove(struct merge_options *o,
+			    const unsigned char *sha1, unsigned mode,
+			    const char *path, int update_wd)
+{
+	if (is_null_sha1(sha1))
+		return remove_file(o, 1, path, !update_wd);
+
+	update_file_flags(o, sha1, mode, path, 1, update_wd);
+	return 0;
+}
+
+static void merge_rename_source(struct merge_options *o,
+				       const char *path,
+				       struct stage_data *d)
+{
+	if (is_null_sha1(d->stages[2].sha)) {
+		/*
+		 * If both were real renames (not from a broken pair), we can
+		 * stop caring about the path. We don't touch the working
+		 * directory, though. The path must be gone in HEAD, so there
+		 * is no point (and anything we did delete would be an
+		 * untracked file).
+		 */
+		if (is_null_sha1(d->stages[3].sha)) {
+			remove_file(o, 1, path, 1);
+			return;
+		}
+
+		/*
+		 * If "ours" was a real rename, but the other side came
+		 * from a broken pair, then their version is the right
+		 * resolution (because we have no content, ours having been
+		 * renamed away, and they have new content).
+		 */
+		update_file_flags(o, d->stages[3].sha, d->stages[3].mode,
+				  path, 1, 1);
+		return;
+	}
+
+	/*
+	 * Now we have the opposite. "theirs" is a real rename, but ours
+	 * is from a broken pair. We resolve in favor of us, but we don't
+	 * need to touch the working directory.
+	 */
+	if (is_null_sha1(d->stages[3].sha)) {
+		update_file_flags(o, d->stages[2].sha, d->stages[2].mode,
+				  path, 1, 0);
+		return;
+	}
+
+	/*
+	 * Otherwise, both came from broken pairs. We need to do an actual
+	 * merge on the entries. We can just mark it as unprocessed and
+	 * the regular code will handle it.
+	 */
+	d->processed = 0;
+}
+
 /* Low level file merging, update and removal */
 
 struct merge_file_info {
@@ -1025,7 +1083,7 @@ static int process_renames(struct merge_options *o,
 							      ren1->dst_entry,
 							      ren2->dst_entry);
 			} else {
-				remove_file(o, 1, ren1_src, 1);
+				merge_rename_source(o, ren1_src, ren1->src_entry);
 				update_stages_and_entry(ren1_dst,
 							ren1->dst_entry,
 							ren1->pair->one,
@@ -1049,7 +1107,10 @@ static int process_renames(struct merge_options *o,
 			int renamed_stage = a_renames == renames1 ? 2 : 3;
 			int other_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;
-- 
1.7.4.41.g423da.dirty

[PATCH 3/3] merge: turn on rewrite detection

[Jeff King]

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 <peff@peff.net>
---
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

diff --git a/merge-recursive.c b/merge-recursive.c
index 16263b0..cd42c47 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -426,6 +426,7 @@ static struct string_list *get_renames(struct merge_options *o,
 			    1000;
 	opts.rename_score = o->rename_score;
 	opts.show_rename_progress = o->show_rename_progress;
+	opts.break_opt = 0;
 	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	if (diff_setup_done(&opts) < 0)
 		die("diff setup failed");
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index e686f04..43ff220 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -631,7 +631,7 @@ test_expect_success 'merge-recursive copy vs. rename' '
 
 if test_have_prereq SYMLINKS
 then
-	test_expect_failure 'merge-recursive rename vs. rename/symlink' '
+	test_expect_success 'merge-recursive rename vs. rename/symlink' '
 
 		git checkout -f rename &&
 		git merge rename-ln &&
diff --git a/t/t6039-merge-break.sh b/t/t6039-merge-break.sh
new file mode 100755
index 0000000..d3dabf5
--- /dev/null
+++ b/t/t6039-merge-break.sh
@@ -0,0 +1,174 @@
+#!/bin/sh
+
+test_description='merging with renames from broken pairs
+
+This is based on a real-world practice of moving a header file to a
+new location, but installing a "replacement" file that points to
+the old one. We need break detection in the merge to find the
+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'
+	for i in 0 1 2 3 4; do
+		for j in 0 1 2 3 4 5 6 7 8 9; 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() {
+	git reset --hard &&
+	git checkout master &&
+	git reset --hard base &&
+	git clean -f &&
+	{ git branch -D topic || true; }
+}
+
+test_expect_success 'setup baseline' '
+	mksample >sample.h &&
+	git add sample.h &&
+	git commit -m "add sample.h" &&
+	git tag base
+'
+
+setup_rename_plus_tweak() {
+	reset &&
+	mvsample sample.h new.h &&
+	mkreplacement new.h >sample.h &&
+	git add sample.h new.h &&
+	git commit -m 'rename sample.h to new.h, with replacement' &&
+	git checkout -b topic base &&
+	tweak sample.h &&
+	git commit -a -m 'tweak sample.h'
+}
+
+check_tweak_result() {
+	mksample >expect.orig &&
+	mvsample expect.orig expect &&
+	tweak expect &&
+	test_cmp expect new.h &&
+	mkreplacement new.h >expect &&
+	test_cmp expect sample.h
+}
+
+test_expect_success 'merge rename to tweak finds rename' '
+	setup_rename_plus_tweak &&
+	git merge master &&
+	check_tweak_result
+'
+
+test_expect_success 'merge tweak to rename finds rename' '
+	setup_rename_plus_tweak &&
+	git checkout master &&
+	git merge topic &&
+	check_tweak_result
+'
+
+setup_double_rename_one_replacement() {
+	setup_rename_plus_tweak &&
+	mvsample sample.h new.h &&
+	git add new.h &&
+	git commit -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 &&
+	git merge master &&
+	check_tweak_result
+'
+
+test_expect_success 'merge rename/tweak to rename (one replacement)' '
+	setup_double_rename_one_replacement &&
+	git checkout master &&
+	git merge topic &&
+	check_tweak_result
+'
+
+setup_double_rename_two_replacements_same() {
+	setup_rename_plus_tweak &&
+	mvsample sample.h new.h &&
+	mkreplacement new.h >sample.h &&
+	git add sample.h new.h &&
+	git commit -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 &&
+	git merge master &&
+	check_tweak_result
+'
+
+test_expect_success 'merge rename/tweak to rename (two replacements, same)' '
+	setup_double_rename_two_replacements_same &&
+	git checkout master &&
+	git merge topic &&
+	check_tweak_result
+'
+
+setup_double_rename_two_replacements_diff() {
+	setup_rename_plus_tweak &&
+	mvsample sample.h new.h &&
+	mkreplacement diff.h >sample.h &&
+	git add sample.h new.h &&
+	git commit -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_fail git merge master &&
+	cat >expect <<-\EOF &&
+	#ifndef SAMPLE_H
+	#define SAMPLE_H
+	<<<<<<< HEAD
+	#include "diff.h"
+	=======
+	#include "new.h"
+	>>>>>>> master
+	#endif /* SAMPLE_H */
+	EOF
+	test_cmp expect sample.h
+'
+
+test_expect_success 'merge rename to rename/tweak (two replacements, diff)' '
+	setup_double_rename_two_replacements_diff &&
+	git checkout master &&
+	test_must_fail git merge topic &&
+	cat >expect <<-\EOF &&
+	#ifndef SAMPLE_H
+	#define SAMPLE_H
+	<<<<<<< HEAD
+	#include "new.h"
+	=======
+	#include "diff.h"
+	>>>>>>> topic
+	#endif /* SAMPLE_H */
+	EOF
+	test_cmp expect sample.h
+'
+
+test_done
-- 
1.7.4.41.g423da.dirty

Re: merge recursive and code movement

[Jay Soffian]

On Fri, Mar 25, 2011 at 12:00 PM, Jeff King <peff@peff.net> 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.

Re: [PATCH 1/3] t3030: fix accidental success in symlink rename

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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".

I read this five times but still couldn't figure out that you meant that
the other side 'added a symlink "a" to allow people keep referring to "e"
with the old name "a"' until I actually read the actual test you are
describing here.

Besides, /we have merge/s/have//, I think.

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

I agree.

We should treat structural changes and do a 3-way on that, and then
another 3-way on content changes, treating them as an independent thing.
One side has "create 'e' out of 'a', removing 'a'" and "_create_ 'a', that
is unrelated to the original 'a'", the other side has "create 'e' out of
'a', removing 'a'", so the end result should be that we do both,
i.e. "create 'e' out of 'a', removing 'a'" and "create 'a'".  At the
content level, the result in 'e' may have to be decided by 3-way.  The
result in 'a' should be a clean merge taken from the former "with b/c
link" branch, as this is not even a create (by the side that added a
backward compatibility symbolic link) vs a delete (by pure-rename side)
conflict.

Re: [PATCH 1/3] t3030: fix accidental success in symlink rename

[Jeff King]

On Fri, Mar 25, 2011 at 10:42:05AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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".
> 
> I read this five times but still couldn't figure out that you meant that
> the other side 'added a symlink "a" to allow people keep referring to "e"
> with the old name "a"' until I actually read the actual test you are
> describing here.

Hmph. I edited it to try to be more clear, and obviously left in a typo.
I clearly need to proofread more.

> Besides, /we have merge/s/have//, I think.

It was actually s/have merge/merge.  So what I intended to write was:

  In this test, we merge two branches. On one branch, we renamed "a" to
  "e". On the other, we renamed "a" to "e" and then added a symlink "a"
  pointing to "e".

If that's not clear enough, then feel free to swap it out for something
better.

> > The only sensible resolution is to keep the symlink.
> 
> I agree.
> 
> We should treat structural changes and do a 3-way on that, and then
> another 3-way on content changes, treating them as an independent thing.
> One side has "create 'e' out of 'a', removing 'a'" and "_create_ 'a', that
> is unrelated to the original 'a'", the other side has "create 'e' out of
> 'a', removing 'a'", so the end result should be that we do both,
> i.e. "create 'e' out of 'a', removing 'a'" and "create 'a'".  At the
> content level, the result in 'e' may have to be decided by 3-way.  The
> result in 'a' should be a clean merge taken from the former "with b/c
> link" branch, as this is not even a create (by the side that added a
> backward compatibility symbolic link) vs a delete (by pure-rename side)
> conflict.

Good, I think we are on the same page. Hopefully you will find my 2/3
correct at least in spirit, then, if not implementation. :)

-Peff

RE: [PATCH 1/3] t3030: fix accidental success in symlink rename

[Schalk, Ken]

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

> I agree.

> We should treat structural changes and do a 3-way on that, and then
> another 3-way on content changes, treating them as an independent
> thing.
> One side has "create 'e' out of 'a', removing 'a'" and "_create_ 'a',
> that
> is unrelated to the original 'a'", the other side has "create 'e' out
> of
> 'a', removing 'a'", so the end result should be that we do both,
> i.e. "create 'e' out of 'a', removing 'a'" and "create 'a'".  At the
> content level, the result in 'e' may have to be decided by 3-way.  The
> result in 'a' should be a clean merge taken from the former "with b/c
> link" branch, as this is not even a create (by the side that added a
> backward compatibility symbolic link) vs a delete (by pure-rename side)
> conflict.

I completely agree, keeping the symlink would be the right thing to do.  When I worked on the patch which added that test, my only concern was eliminating the rename/add conflict on "e" (which seemed pointless, since the content of "e" was identical in both branches).

--Ken Schalk

Re: merge recursive and code movement

[Techlive Zheng]

So, Is there any progress on these patches, I am currently need this
functionality very much, will these be merged into master?

Re: merge recursive and code movement

[Jeff King]

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/175932

  http://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