{"thread":{"id":"26855","subject":"merge recursive and code movement","startedAt":"2011-03-24T21:18:20Z","lastAt":"2012-07-16T12:26:40Z","messageCount":14,"participants":["Jay Soffian","Jeff King","Junio C Hamano","Schalk, Ken","Techlive Zheng"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"164255","messageId":"AANLkTi=h6jUsjqXofd0QeWbNBjc9DeodJJ3FN7caW4XC@mail.gmail.com","threadId":"26855","inReplyTo":null,"subject":"merge recursive and code movement","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-03-24T21:18:20Z","receivedAt":"2011-03-24T21:18:20Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"There's a use case that merge recursive doesn't seem to handle, and I\nwonder how difficult it would be to add.\n\nSay you have a merge between OURS and THEIRS, with common ancestor BASE.\n\nBetween BASE and THEIRS, a file named header.h has the following changes:\n\n  # Rename header.h to header_new.h\n  git mv header.h header_new.h\n\n  # Minor edits to account for the rename such as fixing the\n  # include guard:\n  perl -pi -e 's/HEADER_H_/HEADER_NEW_H_/' header_new.h\n\n  # Drop a compatibility header.h in place till we can fix all the\n  # files which include header.h\n  cat > header.h <<-__EOF__\n\t#ifndef HEADER_H_\n\t#define HEADER_H_\n\t#include \"header_hew.h\"\n\t#endif // HEADER_H_\n  __EOF__\n\n  git add header.h header_new.h\n  git commit -m 'rename header.h to header_new.h'\n\nMeanwhile, between BASE and OURS, a few minor changes are made to\nheader.h. This could be as little as a single line change in the\nmiddle of the header.h.\n\nNow you merge THEIRS to OURS. Git will just show header.h in conflict.\n99% of the time I can do the following:\n\n  git diff MERGE_BASE... header.h | patch header_new.h\n  git checkout --theirs header.h\n  git add header.h header_new.h\n\nBut it would seem like this is something merge recursive should be\ncapable of handling on its own.\n\nj.\n"},{"id":"164285","messageId":"20110325093758.GA9047@sigill.intra.peff.net","threadId":"26855","inReplyTo":"AANLkTi=h6jUsjqXofd0QeWbNBjc9DeodJJ3FN7caW4XC@mail.gmail.com","subject":"Re: merge recursive and code movement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T09:37:58Z","receivedAt":"2011-03-25T09:37:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2011 at 05:18:20PM -0400, Jay Soffian wrote:\n\n> There's a use case that merge recursive doesn't seem to handle, and I\n> wonder how difficult it would be to add.\n\nI don't think it's that hard. In your example:\n\n> Say you have a merge between OURS and THEIRS, with common ancestor BASE.\n> \n> Between BASE and THEIRS, a file named header.h has the following changes:\n> \n>   # Rename header.h to header_new.h\n>   git mv header.h header_new.h\n> \n>   # Minor edits to account for the rename such as fixing the\n>   # include guard:\n>   perl -pi -e 's/HEADER_H_/HEADER_NEW_H_/' header_new.h\n> \n>   # Drop a compatibility header.h in place till we can fix all the\n>   # files which include header.h\n>   cat > header.h <<-__EOF__\n> \t#ifndef HEADER_H_\n> \t#define HEADER_H_\n> \t#include \"header_hew.h\"\n> \t#endif // HEADER_H_\n>   __EOF__\n\nYou have a move coupled with a rewritten version of a file. So without\ncopy or break detection, we won't consider the original header.h as a\npossible rename source.\n\n>   git add header.h header_new.h\n>   git commit -m 'rename header.h to header_new.h'\n> \n> Meanwhile, between BASE and OURS, a few minor changes are made to\n> header.h. This could be as little as a single line change in the\n> middle of the header.h.\n> \n> Now you merge THEIRS to OURS. Git will just show header.h in conflict.\n\nRight. merge-recursive won't detect the rename here.\n\nIn your case, I think merge-recursive doing break detection is the right\nsolution. It realizes that header.h has been rewritten and considers it\nas a source candidate for the rename to header_new.\n\nCopy detection might also work, but I don't think it makes sense in a\nmerge setting. If I copy \"foo.h\" to \"bar.h\", and meanwhile you make a\nchange to \"foo.h\", there is no reason to think the change should apply\nto bar.h instead of foo.h (you might perhaps think it could apply to\n_both_, but that is a different story).\n\nSo break detection is the only thing that makes sense to me.  This\none-liner does what you want:\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2a048c6..ed574e6 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -429,6 +429,7 @@ static struct string_list *get_renames(struct merge_options *o,\n \t\t\t    1000;\n \topts.rename_score = o->rename_score;\n \topts.show_rename_progress = o->show_rename_progress;\n+\topts.break_opt = 0;\n \topts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \tif (diff_setup_done(&opts) < 0)\n \t\tdie(\"diff setup failed\");\n\nAnd I tested it on this:\n\n-- >8 --\n#!/bin/sh\n\nrm -rf repo\n\ngit init repo && cd repo\n\n# Sample header.\ncp /path/to/your/git/revision.h .\ngit add revision.h\ngit commit -m 'add revision.h'\n\n# Move and tweak header.\ngit mv revision.h foo.h\nperl -pi -e 's/REVISION_H/FOO_H/' foo.h\n\n# And put in replacement header.\ncat >revision.h <<'EOF'\n#ifndef REVISION_H\n#define REVISION_H\n#include \"foo.h\"\n#endif /* REVISION_H */\nEOF\n\n# And commit.\ngit add revision.h foo.h\ngit commit -m 'rename revision.h to foo.h'\n\n# Now make a minor change on a side branch.\ngit checkout -b other HEAD^\nsed -i '/REV_TREE_SAME/i/* some comment */' revision.h\ngit commit -a -m 'tweak revision.h'\n\ngit merge master\n-- 8< --\n\nIt _almost_ works. The merge completes automatically, and the tweak ends\nup in foo.h, as you expect. But the merge silently deletes the\nplaceholder revision.h!\n\nI suspect it is a problem of merge-recursive either not handling the\nbroken filepair properly, or perhaps reading too much into what a rename\nmeans. I haven't dug further.\n\n-Peff\n"},{"id":"164289","messageId":"20110325101204.GB9047@sigill.intra.peff.net","threadId":"26855","inReplyTo":"20110325093758.GA9047@sigill.intra.peff.net","subject":"Re: merge recursive and code movement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T10:12:04Z","receivedAt":"2011-03-25T10:12:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2011 at 05:37:58AM -0400, Jeff King wrote:\n\n> It _almost_ works. The merge completes automatically, and the tweak ends\n> up in foo.h, as you expect. But the merge silently deletes the\n> placeholder revision.h!\n> \n> I suspect it is a problem of merge-recursive either not handling the\n> broken filepair properly, or perhaps reading too much into what a rename\n> means. I haven't dug further.\n\nAh, found it. In process_renames, we explicitly call remove_file() on\nthe source, which is assuming the rename did not come from a broken\npair. What we actually want to do, I think, is to just take the changes\nfrom the renaming side literally. There's no point in doing a 3-way\nmerge because the other side's changes will end up applied to the rename\ndestination.  It just happens that without break_opt, the renaming sides\nchange is _always_ a deletion, or else it would not have been a rename\ncandidate. So the current code is a special case for that rule.\n\nNow, as far as how to do that, I haven't a clue. I've been staring at\nmerge-recursive code for 30 minutes. ;)\n\n-Peff\n"},{"id":"164292","messageId":"20110325111225.GC9047@sigill.intra.peff.net","threadId":"26855","inReplyTo":"20110325101204.GB9047@sigill.intra.peff.net","subject":"Re: merge recursive and code movement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T11:12:25Z","receivedAt":"2011-03-25T11:12:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2011 at 06:12:04AM -0400, Jeff King wrote:\n\n> Ah, found it. In process_renames, we explicitly call remove_file() on\n> the source, which is assuming the rename did not come from a broken\n> pair. What we actually want to do, I think, is to just take the changes\n> from the renaming side literally. There's no point in doing a 3-way\n> merge because the other side's changes will end up applied to the rename\n> destination.  It just happens that without break_opt, the renaming sides\n> change is _always_ a deletion, or else it would not have been a rename\n> candidate. So the current code is a special case for that rule.\n> \n> Now, as far as how to do that, I haven't a clue. I've been staring at\n> merge-recursive code for 30 minutes. ;)\n\nSo this is what I ended up with:\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8e82a8b..af42530 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -426,6 +426,7 @@ static struct string_list *get_renames(struct merge_options *o,\n \t\t\t    1000;\n \topts.rename_score = o->rename_score;\n \topts.show_rename_progress = o->show_rename_progress;\n+\topts.break_opt = 0;\n \topts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \tif (diff_setup_done(&opts) < 0)\n \t\tdie(\"diff setup failed\");\n@@ -706,6 +707,17 @@ static void update_file(struct merge_options *o,\n \tupdate_file_flags(o, sha, mode, path, o->call_depth || clean, !o->call_depth);\n }\n \n+static int update_or_remove(struct merge_options *o,\n+\t\t\t    const unsigned char *sha1, unsigned mode,\n+\t\t\t    const char *path, int update_wd)\n+{\n+\tif (is_null_sha1(sha1))\n+\t\treturn remove_file(o, 1, path, !update_wd);\n+\n+\tupdate_file_flags(o, sha1, mode, path, 1, update_wd);\n+\treturn 0;\n+}\n+\n /* Low level file merging, update and removal */\n \n struct merge_file_info {\n@@ -1049,7 +1061,10 @@ static int process_renames(struct merge_options *o,\n \t\t\tint renamed_stage = a_renames == renames1 ? 2 : 3;\n \t\t\tint other_stage =   a_renames == renames1 ? 3 : 2;\n \n-\t\t\tremove_file(o, 1, ren1_src, o->call_depth || renamed_stage == 2);\n+\t\t\tupdate_or_remove(o,\n+\t\t\t\tren1->src_entry->stages[renamed_stage].sha,\n+\t\t\t\tren1->src_entry->stages[renamed_stage].mode,\n+\t\t\t\tren1_src, renamed_stage == 3);\n \n \t\t\thashcpy(src_other.sha1, ren1->src_entry->stages[other_stage].sha);\n \t\t\tsrc_other.mode = ren1->src_entry->stages[other_stage].mode;\n\nIt passes my test, and it doesn't break anything in t/. Yay.\n\nThere's one other call to remove_file in process_renames. It's for the\ncase that both sides renamed the same file to the same destination.  I\nthink there we need to actually compare the two sides. If only one side\nstill has something at the source path, then we can take that side\n(since the other side renamed away the file). But if they both have it\n(i.e., they both installed a replacement), then we need to do the usual\n3-way merge on that replacement. I'm not sure if we'd have to do that\nourselves, or if we can just punt and the rest of the merge machinery\nwill handle the entry. I'll have to write some tests, I think.\n\n-Peff\n"},{"id":"164302","messageId":"20110325160013.GA25851@sigill.intra.peff.net","threadId":"26855","inReplyTo":"20110325111225.GC9047@sigill.intra.peff.net","subject":"Re: merge recursive and code movement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T16:00:13Z","receivedAt":"2011-03-25T16:00:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2011 at 07:12:25AM -0400, Jeff King wrote:\n\n> It passes my test, and it doesn't break anything in t/. Yay.\n> \n> There's one other call to remove_file in process_renames. It's for the\n> case that both sides renamed the same file to the same destination.  I\n> think there we need to actually compare the two sides. If only one side\n> still has something at the source path, then we can take that side\n> (since the other side renamed away the file). But if they both have it\n> (i.e., they both installed a replacement), then we need to do the usual\n> 3-way merge on that replacement. I'm not sure if we'd have to do that\n> ourselves, or if we can just punt and the rest of the merge machinery\n> will handle the entry. I'll have to write some tests, I think.\n\nOK, I figured it out. I was thrown off by test failures in t3030, but I\nthink that test is actually wrong; it documents what happens, but not\nreally what we _want_ to have happen.\n\nSo this is the patch series I ended up with:\n\n  [1/3]: t3030: fix accidental success in symlink rename\n  [2/3]: merge: handle renames with replacement content\n  [3/3]: merge: turn on rewrite detection\n\n-Peff\n"},{"id":"164303","messageId":"20110325160326.GA26635@sigill.intra.peff.net","threadId":"26855","inReplyTo":"20110325160013.GA25851@sigill.intra.peff.net","subject":"[PATCH 1/3] t3030: fix accidental success in symlink rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T16:03:26Z","receivedAt":"2011-03-25T16:03:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In this test, we have merge two branches. On one branch, we\nrenamed \"a\" to \"e\". On the other, we renamed \"a\" to \"e\" and\nthen added a symlink pointing at \"a\" pointing to \"e\".\n\nThe results for the test indicate that the merge should\nsucceed, but also that \"a\" should no longer exist. Since\nboth sides renamed \"a\" to the same destination, we will end\nup comparing those destinations for content.\n\nBut what about what's left? One side (the rename only),\nreplaced \"a\" with nothing. The other side replaced it with a\nsymlink. The common base must also be nothing, because any\n\"a\" before this was meaningless (it was totally unrelated\ncontent that ended up getting renamed).\n\nThe only sensible resolution is to keep the symlink. The\nrename-only side didn't touch the content versus the common\nbase, and the other side added content. The 3-way merge\ndictates that we take the side with a change.\n\nAnd this gives the overall merge an intuitive result.  One\nside made one change (a rename), and the other side made two\nchanges: an identical rename, and an addition (that just\nhappened to be at the same spot). The end result should\ncontain both changes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nKen, I'm cc'ing you as the test in question is yours. The\ncontext is that I want to turn on break detection in\nmerge-recursive, but doing so makes your test fail.\n\n t/t3030-merge-recursive.sh |    7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 34794f8..e686f04 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -267,7 +267,8 @@ test_expect_success 'setup 8' '\n \t\tln -s e a &&\n \t\tgit add a e &&\n \t\ttest_tick &&\n-\t\tgit commit -m \"rename a->e, symlink a->e\"\n+\t\tgit commit -m \"rename a->e, symlink a->e\" &&\n+\t\toln=`printf e | git hash-object --stdin`\n \tfi\n '\n \n@@ -630,16 +631,18 @@ test_expect_success 'merge-recursive copy vs. rename' '\n \n if test_have_prereq SYMLINKS\n then\n-\ttest_expect_success 'merge-recursive rename vs. rename/symlink' '\n+\ttest_expect_failure 'merge-recursive rename vs. rename/symlink' '\n \n \t\tgit checkout -f rename &&\n \t\tgit merge rename-ln &&\n \t\t( git ls-tree -r HEAD ; git ls-files -s ) >actual &&\n \t\t(\n+\t\t\techo \"120000 blob $oln\ta\"\n \t\t\techo \"100644 blob $o0\tb\"\n \t\t\techo \"100644 blob $o0\tc\"\n \t\t\techo \"100644 blob $o0\td/e\"\n \t\t\techo \"100644 blob $o0\te\"\n+\t\t\techo \"120000 $oln 0\ta\"\n \t\t\techo \"100644 $o0 0\tb\"\n \t\t\techo \"100644 $o0 0\tc\"\n \t\t\techo \"100644 $o0 0\td/e\"\n-- \n1.7.4.41.g423da.dirty\n"},{"id":"164304","messageId":"20110325160647.GB26635@sigill.intra.peff.net","threadId":"26855","inReplyTo":"20110325160013.GA25851@sigill.intra.peff.net","subject":"[PATCH 2/3] merge: handle renames with replacement content","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T16:06:47Z","receivedAt":"2011-03-25T16:06:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally think of a rename as removing one path entirely\nand placing similar content at a new path. In other words,\nafter a rename, the original path is now empty. But that is\nnot necessarily the case with rewrite detection (which is\nnot currently possible to do for merge-recursive).\n\nThe current merge code blindly removes paths that are used\nas rename sources; however, we should check to see if there\nis useful content at that path.  There are basically two\ninteresting cases:\n\n  1. One side renames a path, but also puts new content\n     (or a symlink) at the same path. We want to detect the\n     rename, and have changes from the other side applied to\n     the rename destination. The new content at the original\n     path should be left untouched.\n\n     The current code just calls remove_file, but that\n     ignores the concept that the renaming side may put\n     something else useful there. We should detect this case\n     and either remove (if no new content), or put the new\n     content in place at the original path.\n\n  2. Both sides renamed and installed new content at the\n     original path. If they didn't rename to the same\n     destination, it is a conflict, and we already mark it\n     as such. But if it's the same destination, then it's\n     not a conflict; the renamed content will be merged at\n     the new destination.\n\n     For the new content at the original path, we have to do\n     a 3-way merge. The base must be the null sha1, because\n     this \"slot\" for content didn't exist before (it was\n     taken up by the content which got renamed away). So if\n     only one side installed new content, that content\n     automatically wins. If both sides did, and it is the\n     same content, then that content is OK. But if the\n     content is different, then we have a conflict and\n     should do the usual conflict-markers thing.\n\nThis patch implements the semantics described above, which\nlays the groundwork for turning on rewrite detection.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI split this one out for easier review. The semantics I am\nproposing are a superset of the current \"remove\" behavior (it's just\nthat without break detection on, you can't trigger the other cases). But\nby putting this in before enabling break detection, you can test easily\nthat the new code isn't breaking any existing behavior.\n\n merge-recursive.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 63 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8e82a8b..16263b0 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -706,6 +706,64 @@ static void update_file(struct merge_options *o,\n \tupdate_file_flags(o, sha, mode, path, o->call_depth || clean, !o->call_depth);\n }\n \n+static int update_or_remove(struct merge_options *o,\n+\t\t\t    const unsigned char *sha1, unsigned mode,\n+\t\t\t    const char *path, int update_wd)\n+{\n+\tif (is_null_sha1(sha1))\n+\t\treturn remove_file(o, 1, path, !update_wd);\n+\n+\tupdate_file_flags(o, sha1, mode, path, 1, update_wd);\n+\treturn 0;\n+}\n+\n+static void merge_rename_source(struct merge_options *o,\n+\t\t\t\t       const char *path,\n+\t\t\t\t       struct stage_data *d)\n+{\n+\tif (is_null_sha1(d->stages[2].sha)) {\n+\t\t/*\n+\t\t * If both were real renames (not from a broken pair), we can\n+\t\t * stop caring about the path. We don't touch the working\n+\t\t * directory, though. The path must be gone in HEAD, so there\n+\t\t * is no point (and anything we did delete would be an\n+\t\t * untracked file).\n+\t\t */\n+\t\tif (is_null_sha1(d->stages[3].sha)) {\n+\t\t\tremove_file(o, 1, path, 1);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If \"ours\" was a real rename, but the other side came\n+\t\t * from a broken pair, then their version is the right\n+\t\t * resolution (because we have no content, ours having been\n+\t\t * renamed away, and they have new content).\n+\t\t */\n+\t\tupdate_file_flags(o, d->stages[3].sha, d->stages[3].mode,\n+\t\t\t\t  path, 1, 1);\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * Now we have the opposite. \"theirs\" is a real rename, but ours\n+\t * is from a broken pair. We resolve in favor of us, but we don't\n+\t * need to touch the working directory.\n+\t */\n+\tif (is_null_sha1(d->stages[3].sha)) {\n+\t\tupdate_file_flags(o, d->stages[2].sha, d->stages[2].mode,\n+\t\t\t\t  path, 1, 0);\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * Otherwise, both came from broken pairs. We need to do an actual\n+\t * merge on the entries. We can just mark it as unprocessed and\n+\t * the regular code will handle it.\n+\t */\n+\td->processed = 0;\n+}\n+\n /* Low level file merging, update and removal */\n \n struct merge_file_info {\n@@ -1025,7 +1083,7 @@ static int process_renames(struct merge_options *o,\n \t\t\t\t\t\t\t      ren1->dst_entry,\n \t\t\t\t\t\t\t      ren2->dst_entry);\n \t\t\t} else {\n-\t\t\t\tremove_file(o, 1, ren1_src, 1);\n+\t\t\t\tmerge_rename_source(o, ren1_src, ren1->src_entry);\n \t\t\t\tupdate_stages_and_entry(ren1_dst,\n \t\t\t\t\t\t\tren1->dst_entry,\n \t\t\t\t\t\t\tren1->pair->one,\n@@ -1049,7 +1107,10 @@ static int process_renames(struct merge_options *o,\n \t\t\tint renamed_stage = a_renames == renames1 ? 2 : 3;\n \t\t\tint other_stage =   a_renames == renames1 ? 3 : 2;\n \n-\t\t\tremove_file(o, 1, ren1_src, o->call_depth || renamed_stage == 2);\n+\t\t\tupdate_or_remove(o,\n+\t\t\t\tren1->src_entry->stages[renamed_stage].sha,\n+\t\t\t\tren1->src_entry->stages[renamed_stage].mode,\n+\t\t\t\tren1_src, renamed_stage == 3);\n \n \t\t\thashcpy(src_other.sha1, ren1->src_entry->stages[other_stage].sha);\n \t\t\tsrc_other.mode = ren1->src_entry->stages[other_stage].mode;\n-- \n1.7.4.41.g423da.dirty\n"},{"id":"164305","messageId":"20110325160810.GC26635@sigill.intra.peff.net","threadId":"26855","inReplyTo":"20110325160013.GA25851@sigill.intra.peff.net","subject":"[PATCH 3/3] merge: turn on rewrite detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T16:08:10Z","receivedAt":"2011-03-25T16:08:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently don't do break-detection at all in\nmerge-recursive. But there are some cases where it would\nprovide a more useful merge result.\n\nFor example, consider this case (which is the basis for the\nnew tests in t6039):\n\n  1. You rename a header file foo.h to bar.h. You install a\n     new foo.h that includes bar.h (for compatibility).\n\n  2. Another branch makes changes to foo.h.\n\nWhen you merge, you want the changes the other branch made\nto foo.h to migrate to the rename destination, bar.h, just\nas you would if you hadn't installed that compatibility\nheader.\n\nSimilarly, you want the compatibility header left untouched.\nThe other side's changes all ended up in bar.h, so there is\nno reason to conflict with the new content in foo.h.\n\nThis patch turns on break detection for merge-recursive. In\naddition to new tests in t6039, it makes a similar test in\nt3030 pass.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI hope the tests are readable. I thought it was important to test the\nmerges in both directions (because an early iteration screwed that up),\nwhich led to factoring out a lot of the setup and checking code.\n\n merge-recursive.c          |    1 +\n t/t3030-merge-recursive.sh |    2 +-\n t/t6039-merge-break.sh     |  174 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 176 insertions(+), 1 deletions(-)\n create mode 100755 t/t6039-merge-break.sh\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 16263b0..cd42c47 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -426,6 +426,7 @@ static struct string_list *get_renames(struct merge_options *o,\n \t\t\t    1000;\n \topts.rename_score = o->rename_score;\n \topts.show_rename_progress = o->show_rename_progress;\n+\topts.break_opt = 0;\n \topts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \tif (diff_setup_done(&opts) < 0)\n \t\tdie(\"diff setup failed\");\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex e686f04..43ff220 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -631,7 +631,7 @@ test_expect_success 'merge-recursive copy vs. rename' '\n \n if test_have_prereq SYMLINKS\n then\n-\ttest_expect_failure 'merge-recursive rename vs. rename/symlink' '\n+\ttest_expect_success 'merge-recursive rename vs. rename/symlink' '\n \n \t\tgit checkout -f rename &&\n \t\tgit merge rename-ln &&\ndiff --git a/t/t6039-merge-break.sh b/t/t6039-merge-break.sh\nnew file mode 100755\nindex 0000000..d3dabf5\n--- /dev/null\n+++ b/t/t6039-merge-break.sh\n@@ -0,0 +1,174 @@\n+#!/bin/sh\n+\n+test_description='merging with renames from broken pairs\n+\n+This is based on a real-world practice of moving a header file to a\n+new location, but installing a \"replacement\" file that points to\n+the old one. We need break detection in the merge to find the\n+rename.\n+'\n+. ./test-lib.sh\n+\n+# A fake header file; it needs a fair bit of content\n+# for break detection and inexact rename detection to work.\n+mksample() {\n+\techo '#ifndef SAMPLE_H'\n+\techo '#define SAMPLE_H'\n+\tfor i in 0 1 2 3 4; do\n+\t\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\techo \"extern fun$i$j();\"\n+\t\tdone\n+\tdone\n+\techo '#endif /* SAMPLE_H */'\n+}\n+\n+mvsample() {\n+\tsed 's/SAMPLE_H/NEW_H/' \"$1\" >\"$2\" &&\n+\trm \"$1\"\n+}\n+\n+# A replacement sample header file that references a new one.\n+mkreplacement() {\n+\techo '#ifndef SAMPLE_H'\n+\techo '#define SAMPLE_H'\n+\techo \"#include \\\"$1\\\"\"\n+\techo '#endif /* SAMPLE_H */'\n+}\n+\n+# Tweak the header file in a minor way.\n+tweak() {\n+\tsed 's,42.*,& /* secret of something-or-other */,' \"$1\" >\"$1.tmp\" &&\n+\tmv \"$1.tmp\" \"$1\"\n+}\n+\n+reset() {\n+\tgit reset --hard &&\n+\tgit checkout master &&\n+\tgit reset --hard base &&\n+\tgit clean -f &&\n+\t{ git branch -D topic || true; }\n+}\n+\n+test_expect_success 'setup baseline' '\n+\tmksample >sample.h &&\n+\tgit add sample.h &&\n+\tgit commit -m \"add sample.h\" &&\n+\tgit tag base\n+'\n+\n+setup_rename_plus_tweak() {\n+\treset &&\n+\tmvsample sample.h new.h &&\n+\tmkreplacement new.h >sample.h &&\n+\tgit add sample.h new.h &&\n+\tgit commit -m 'rename sample.h to new.h, with replacement' &&\n+\tgit checkout -b topic base &&\n+\ttweak sample.h &&\n+\tgit commit -a -m 'tweak sample.h'\n+}\n+\n+check_tweak_result() {\n+\tmksample >expect.orig &&\n+\tmvsample expect.orig expect &&\n+\ttweak expect &&\n+\ttest_cmp expect new.h &&\n+\tmkreplacement new.h >expect &&\n+\ttest_cmp expect sample.h\n+}\n+\n+test_expect_success 'merge rename to tweak finds rename' '\n+\tsetup_rename_plus_tweak &&\n+\tgit merge master &&\n+\tcheck_tweak_result\n+'\n+\n+test_expect_success 'merge tweak to rename finds rename' '\n+\tsetup_rename_plus_tweak &&\n+\tgit checkout master &&\n+\tgit merge topic &&\n+\tcheck_tweak_result\n+'\n+\n+setup_double_rename_one_replacement() {\n+\tsetup_rename_plus_tweak &&\n+\tmvsample sample.h new.h &&\n+\tgit add new.h &&\n+\tgit commit -a -m 'rename sample.h to new.h (no replacement)'\n+}\n+\n+test_expect_success 'merge rename to rename/tweak (one replacement)' '\n+\tsetup_double_rename_one_replacement &&\n+\tgit merge master &&\n+\tcheck_tweak_result\n+'\n+\n+test_expect_success 'merge rename/tweak to rename (one replacement)' '\n+\tsetup_double_rename_one_replacement &&\n+\tgit checkout master &&\n+\tgit merge topic &&\n+\tcheck_tweak_result\n+'\n+\n+setup_double_rename_two_replacements_same() {\n+\tsetup_rename_plus_tweak &&\n+\tmvsample sample.h new.h &&\n+\tmkreplacement new.h >sample.h &&\n+\tgit add sample.h new.h &&\n+\tgit commit -m 'rename sample.h to new.h with replacement (same)'\n+}\n+\n+test_expect_success 'merge rename to rename/tweak (two replacements, same)' '\n+\tsetup_double_rename_two_replacements_same &&\n+\tgit merge master &&\n+\tcheck_tweak_result\n+'\n+\n+test_expect_success 'merge rename/tweak to rename (two replacements, same)' '\n+\tsetup_double_rename_two_replacements_same &&\n+\tgit checkout master &&\n+\tgit merge topic &&\n+\tcheck_tweak_result\n+'\n+\n+setup_double_rename_two_replacements_diff() {\n+\tsetup_rename_plus_tweak &&\n+\tmvsample sample.h new.h &&\n+\tmkreplacement diff.h >sample.h &&\n+\tgit add sample.h new.h &&\n+\tgit commit -m 'rename sample.h to new.h with replacement (diff)'\n+}\n+\n+test_expect_success 'merge rename to rename/tweak (two replacements, diff)' '\n+\tsetup_double_rename_two_replacements_diff &&\n+\ttest_must_fail git merge master &&\n+\tcat >expect <<-\\EOF &&\n+\t#ifndef SAMPLE_H\n+\t#define SAMPLE_H\n+\t<<<<<<< HEAD\n+\t#include \"diff.h\"\n+\t=======\n+\t#include \"new.h\"\n+\t>>>>>>> master\n+\t#endif /* SAMPLE_H */\n+\tEOF\n+\ttest_cmp expect sample.h\n+'\n+\n+test_expect_success 'merge rename to rename/tweak (two replacements, diff)' '\n+\tsetup_double_rename_two_replacements_diff &&\n+\tgit checkout master &&\n+\ttest_must_fail git merge topic &&\n+\tcat >expect <<-\\EOF &&\n+\t#ifndef SAMPLE_H\n+\t#define SAMPLE_H\n+\t<<<<<<< HEAD\n+\t#include \"new.h\"\n+\t=======\n+\t#include \"diff.h\"\n+\t>>>>>>> topic\n+\t#endif /* SAMPLE_H */\n+\tEOF\n+\ttest_cmp expect sample.h\n+'\n+\n+test_done\n-- \n1.7.4.41.g423da.dirty\n"},{"id":"293748","messageId":"AANLkTimWm6eim2u-SDrot_VtMuUf=wvf_P5DuYpuV-m5@mail.gmail.com","threadId":"26855","inReplyTo":"20110325160013.GA25851@sigill.intra.peff.net","subject":"Re: merge recursive and code movement","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-03-25T17:32:22Z","receivedAt":"2011-03-25T17:32:22Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Mar 25, 2011 at 12:00 PM, Jeff King <peff@peff.net> wrote:\n>\n> OK, I figured it out. I was thrown off by test failures in t3030, but I\n> think that test is actually wrong; it documents what happens, but not\n> really what we _want_ to have happen.\n>\n> So this is the patch series I ended up with:\n>\n>  [1/3]: t3030: fix accidental success in symlink rename\n>  [2/3]: merge: handle renames with replacement content\n>  [3/3]: merge: turn on rewrite detection\n\nI read through all three of these and, from my superficial\nunderstanding of the merge code, they look correct.\n\nI'll test these out on some actual merges as soon as I can. (Probably\nnext week.)\n\nThank you for this series.\n\n"},{"id":"164310","messageId":"7v1v1v6qs2.fsf@alter.siamese.dyndns.org","threadId":"26855","inReplyTo":"20110325160326.GA26635@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] t3030: fix accidental success in symlink rename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-25T17:42:05Z","receivedAt":"2011-03-25T17:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> In this test, we have merge two branches. On one branch, we\n> renamed \"a\" to \"e\". On the other, we renamed \"a\" to \"e\" and\n> then added a symlink pointing at \"a\" pointing to \"e\".\n\nI read this five times but still couldn't figure out that you meant that\nthe other side 'added a symlink \"a\" to allow people keep referring to \"e\"\nwith the old name \"a\"' until I actually read the actual test you are\ndescribing here.\n\nBesides, /we have merge/s/have//, I think.\n\n> The results for the test indicate that the merge should\n> succeed, but also that \"a\" should no longer exist. Since\n> both sides renamed \"a\" to the same destination, we will end\n> up comparing those destinations for content.\n>\n> But what about what's left? One side (the rename only),\n> replaced \"a\" with nothing. The other side replaced it with a\n> symlink. The common base must also be nothing, because any\n> \"a\" before this was meaningless (it was totally unrelated\n> content that ended up getting renamed).\n>\n> The only sensible resolution is to keep the symlink.\n\nI agree.\n\nWe should treat structural changes and do a 3-way on that, and then\nanother 3-way on content changes, treating them as an independent thing.\nOne side has \"create 'e' out of 'a', removing 'a'\" and \"_create_ 'a', that\nis unrelated to the original 'a'\", the other side has \"create 'e' out of\n'a', removing 'a'\", so the end result should be that we do both,\ni.e. \"create 'e' out of 'a', removing 'a'\" and \"create 'a'\".  At the\ncontent level, the result in 'e' may have to be decided by 3-way.  The\nresult in 'a' should be a clean merge taken from the former \"with b/c\nlink\" branch, as this is not even a create (by the side that added a\nbackward compatibility symbolic link) vs a delete (by pure-rename side)\nconflict.\n"},{"id":"164312","messageId":"20110325175124.GA24513@sigill.intra.peff.net","threadId":"26855","inReplyTo":"7v1v1v6qs2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] t3030: fix accidental success in symlink rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T17:51:24Z","receivedAt":"2011-03-25T17:51:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2011 at 10:42:05AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > In this test, we have merge two branches. On one branch, we\n> > renamed \"a\" to \"e\". On the other, we renamed \"a\" to \"e\" and\n> > then added a symlink pointing at \"a\" pointing to \"e\".\n> \n> I read this five times but still couldn't figure out that you meant that\n> the other side 'added a symlink \"a\" to allow people keep referring to \"e\"\n> with the old name \"a\"' until I actually read the actual test you are\n> describing here.\n\nHmph. I edited it to try to be more clear, and obviously left in a typo.\nI clearly need to proofread more.\n\n> Besides, /we have merge/s/have//, I think.\n\nIt was actually s/have merge/merge.  So what I intended to write was:\n\n  In this test, we merge two branches. On one branch, we renamed \"a\" to\n  \"e\". On the other, we renamed \"a\" to \"e\" and then added a symlink \"a\"\n  pointing to \"e\".\n\nIf that's not clear enough, then feel free to swap it out for something\nbetter.\n\n> > The only sensible resolution is to keep the symlink.\n> \n> I agree.\n> \n> We should treat structural changes and do a 3-way on that, and then\n> another 3-way on content changes, treating them as an independent thing.\n> One side has \"create 'e' out of 'a', removing 'a'\" and \"_create_ 'a', that\n> is unrelated to the original 'a'\", the other side has \"create 'e' out of\n> 'a', removing 'a'\", so the end result should be that we do both,\n> i.e. \"create 'e' out of 'a', removing 'a'\" and \"create 'a'\".  At the\n> content level, the result in 'e' may have to be decided by 3-way.  The\n> result in 'a' should be a clean merge taken from the former \"with b/c\n> link\" branch, as this is not even a create (by the side that added a\n> backward compatibility symbolic link) vs a delete (by pure-rename side)\n> conflict.\n\nGood, I think we are on the same page. Hopefully you will find my 2/3\ncorrect at least in spirit, then, if not implementation. :)\n\n-Peff\n"},{"id":"164321","messageId":"EF9FEAB3A4B7D245B0801936B6EF4A251C397FCE@azsmsx503.amr.corp.intel.com","threadId":"26855","inReplyTo":"7v1v1v6qs2.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/3] t3030: fix accidental success in symlink rename","fromName":"Schalk, Ken","fromEmail":"ken.schalk@intel.com","sentAt":"2011-03-25T18:25:32Z","receivedAt":"2011-03-25T18:25:32Z","isPatch":true,"sender":{"key":"ken.schalk@intel.com","avatar":null},"body":"> > The results for the test indicate that the merge should\n> > succeed, but also that \"a\" should no longer exist. Since\n> > both sides renamed \"a\" to the same destination, we will end\n> > up comparing those destinations for content.\n\n> > But what about what's left? One side (the rename only),\n> > replaced \"a\" with nothing. The other side replaced it with a\n> > symlink. The common base must also be nothing, because any\n> > \"a\" before this was meaningless (it was totally unrelated\n> > content that ended up getting renamed).\n\n> > The only sensible resolution is to keep the symlink.\n\n> I agree.\n\n> We should treat structural changes and do a 3-way on that, and then\n> another 3-way on content changes, treating them as an independent\n> thing.\n> One side has \"create 'e' out of 'a', removing 'a'\" and \"_create_ 'a',\n> that\n> is unrelated to the original 'a'\", the other side has \"create 'e' out\n> of\n> 'a', removing 'a'\", so the end result should be that we do both,\n> i.e. \"create 'e' out of 'a', removing 'a'\" and \"create 'a'\".  At the\n> content level, the result in 'e' may have to be decided by 3-way.  The\n> result in 'a' should be a clean merge taken from the former \"with b/c\n> link\" branch, as this is not even a create (by the side that added a\n> backward compatibility symbolic link) vs a delete (by pure-rename side)\n> conflict.\n\nI 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).\n\n--Ken Schalk\n"},{"id":"195091","messageId":"loom.20120716T021344-246@post.gmane.org","threadId":"26855","inReplyTo":"20110325160013.GA25851@sigill.intra.peff.net","subject":"Re: merge recursive and code movement","fromName":"Techlive Zheng","fromEmail":"techlivezheng@gmail.com","sentAt":"2012-07-16T00:17:26Z","receivedAt":"2012-07-16T00:17:26Z","isPatch":false,"sender":{"key":"techlivezheng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/816673?v=4"},"body":"So, Is there any progress on these patches, I am currently need this\nfunctionality very much, will these be merged into master?\n"},{"id":"195118","messageId":"20120716122640.GC4962@sigill.intra.peff.net","threadId":"26855","inReplyTo":"loom.20120716T021344-246@post.gmane.org","subject":"Re: merge recursive and code movement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-16T12:26:40Z","receivedAt":"2012-07-16T12:26:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 12:17:26AM +0000, Techlive Zheng wrote:\n\n> So, Is there any progress on these patches, I am currently need this\n> functionality very much, will these be merged into master?\n\nNo. Turning on break detection in merge-recursive triggered bugs\nelsewhere in merge-recursive. See these followup posts:\n\n  http://article.gmane.org/gmane.comp.version-control.git/175932\n\n  http://article.gmane.org/gmane.comp.version-control.git/175990\n\nThe merge-recursive code is sufficiently horrific that I have been\nsuccessfully putting off digging back into the problem for a whole\nyear. :)\n\nUntil those problems are resolved, the patches have too many regressions\nto go into master.\n\n-Peff\n\nPS If you are going to reply to a year-old thread, it is probably a good\nidea to give some context in your message, and to cc the involved\nparties. Most of us use threaded mail readers, but not everybody keeps a\nyear of archives around. The original discussion and patches were here:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/169944\n"}]}