{"thread":{"id":"24685","subject":"[PATCH] Avoid rename/add conflict when contents are identical","startedAt":"2010-08-09T21:00:31Z","lastAt":"2010-09-03T18:29:32Z","messageCount":8,"participants":["Schalk, Ken","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"147555","messageId":"5C4EA6C5B30E6B45A9164CB3A26A0D326F79D9AA04@azsmsx503.amr.corp.intel.com","threadId":"24685","inReplyTo":null,"subject":"[PATCH] Avoid rename/add conflict when contents are identical","fromName":"Schalk, Ken","fromEmail":"ken.schalk@intel.com","sentAt":"2010-08-09T21:00:31Z","receivedAt":"2010-08-09T21:00:31Z","isPatch":true,"sender":{"key":"ken.schalk@intel.com","avatar":null},"body":"Skip the entire rename/add conflict case if the file added on the\nother branch has the same contents as the file being renamed. This\navoids giving the user an extra copy of the same file and presenting a\nconflict that is confusing and pointless.\n\nHere's a simple sequence that generates this kind of conflict:\n\n git init\n echo content > fileA\n git add fileA\n git commit -m Initial\n git checkout -b abranch\n mv fileA fileB\n git add fileB\n rm fileA\n ln -s fileB fileA\n git add fileA\n git commit -m Linked\n git checkout master\n git mv fileA fileB\n git add fileB\n git commit -m Moved\n git merge --no-commit abranch\n\nSigned-off-by: Ken Schalk \n---\n merge-recursive.c | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex fb6aa4a..57c7a85 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -955,6 +955,18 @@ static int process_renames(struct merge_options *o,\n ren1->pair->two : NULL,\n branch1 == o->branch1 ?\n NULL : ren1->pair->two, 1);\n+ } else if ((dst_other.mode == ren1->pair->two->mode) &&\n+ sha_eq(dst_other.sha1, ren1->pair->two->sha1)) {\n+ /* Added file on the other side\n+ identical to the file being\n+ renamed: clean merge */\n+ update_file(o, 1, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);\n+ if (!o->call_depth)\n+ update_stages(ren1_dst, NULL,\n+ branch1 == o->branch1 ?\n+ ren1->pair->two : NULL,\n+ branch1 == o->branch1 ?\n+ NULL : ren1->pair->two, 1);\n } else if (!sha_eq(dst_other.sha1, null_sha1)) {\n const char *new_path;\n clean_merge = 0;\n--\n1.7.0\n"},{"id":"147560","messageId":"AANLkTi=tnWy3w1Ek+uvS+QCr4F+An4FfZcHepiTCFGAf@mail.gmail.com","threadId":"24685","inReplyTo":"5C4EA6C5B30E6B45A9164CB3A26A0D326F79D9AA04@azsmsx503.amr.corp.intel.com","subject":"Re: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-09T21:24:25Z","receivedAt":"2010-08-09T21:24:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Aug 9, 2010 at 21:00, Schalk, Ken wrote:\n\n> Here's a simple sequence that generates this kind of conflict:\n>\n>  git init\n>  echo content > fileA\n>  git add fileA\n>  git commit -m Initial\n>  git checkout -b abranch\n>  mv fileA fileB\n>  git add fileB\n>  rm fileA\n>  ln -s fileB fileA\n>  git add fileA\n>  git commit -m Linked\n>  git checkout master\n>  git mv fileA fileB\n>  git add fileB\n>  git commit -m Moved\n>  git merge --no-commit abranch\n\nI don't know merge well enough to review the code in this patch, but\nthe patch would be much better if you turned this sequence of commands\n(slightly altered, see t/README) into a now-passing test as part of\nthe patch.\n\nt/t3030-merge-recursive.sh would be the right place to add it.\n"},{"id":"147979","messageId":"7vd3tnmht0.fsf@alter.siamese.dyndns.org","threadId":"24685","inReplyTo":"5C4EA6C5B30E6B45A9164CB3A26A0D326F79D9AA04@azsmsx503.amr.corp.intel.com","subject":"Re: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-13T01:18:19Z","receivedAt":"2010-08-13T01:18:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Schalk, Ken\" writes:\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index fb6aa4a..57c7a85 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -955,6 +955,18 @@ static int process_renames(struct merge_options *o,\n> ren1->pair->two : NULL,\n> branch1 == o->branch1 ?\n> NULL : ren1->pair->two, 1);\n> + } else if ((dst_other.mode == ren1->pair->two->mode) &&\n> + sha_eq(dst_other.sha1, ren1->pair->two->sha1)) {\n> + /* Added file on the other side\n> + identical to the file being\n> + renamed: clean merge */\n> + update_file(o, 1, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);\n> + if (!o->call_depth)\n> + update_stages(ren1_dst, NULL,\n> + branch1 == o->branch1 ?\n> + ren1->pair->two : NULL,\n> + branch1 == o->branch1 ?\n> + NULL : ren1->pair->two, 1);\n> } else if (!sha_eq(dst_other.sha1, null_sha1)) {\n> const char *new_path;\n> clean_merge = 0;\n\nThe logic seems to be clear enough. Could you write a test script instead\nof description in the commit log message, so that future changes to the\ncodebase won't break this improvement?\n\nThanks.\n"},{"id":"149145","messageId":"EF9FEAB3A4B7D245B0801936B6EF4A25262E13@azsmsx503.amr.corp.intel.com","threadId":"24685","inReplyTo":"7vd3tnmht0.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Schalk, Ken","fromEmail":"ken.schalk@intel.com","sentAt":"2010-08-27T22:14:19Z","receivedAt":"2010-08-27T22:14:19Z","isPatch":true,"sender":{"key":"ken.schalk@intel.com","avatar":null},"body":">The logic seems to be clear enough. Could you write a test script instead\n>of description in the commit log message, so that future changes to the\n>codebase won't break this improvement?\n\nHere's a revised patch which fixes a problem with the earlier one and includes a test (where Ævar Bjarmason suggested it):\n\nSkip the entire rename/add conflict case if the file added on the\nother branch has the same contents as the file being renamed. This\navoids giving the user an extra copy of the same file and presenting a\nconflict that is confusing and pointless.\n\nA simple test of this case has been added in\nt/t3030-merge-recursive.sh.\n\nSigned-off-by: Ken Schalk \n---\n merge-recursive.c | 6 ++++++\n t/t3030-merge-recursive.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 0 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex fb6aa4a..a2fba84 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -955,6 +955,12 @@ static int process_renames(struct merge_options *o,\n ren1->pair->two : NULL,\n branch1 == o->branch1 ?\n NULL : ren1->pair->two, 1);\n+ } else if ((dst_other.mode == ren1->pair->two->mode) &&\n+ sha_eq(dst_other.sha1, ren1->pair->two->sha1)) {\n+ /* Added file on the other side\n+ identical to the file being\n+ renamed: clean merge */\n+ update_file(o, 1, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);\n } else if (!sha_eq(dst_other.sha1, null_sha1)) {\n const char *new_path;\n clean_merge = 0;\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex d541544..6436582 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -23,6 +23,8 @@ test_expect_success 'setup 1' '\n git branch df-3 &&\n git branch remove &&\n git branch submod &&\n+ git branch mv &&\n+ git branch mv-ln &&\n\n echo hello >>a &&\n cp a d/e &&\n@@ -248,6 +250,25 @@ test_expect_success 'setup 7' '\n git commit -m \"make d/ a submodule\"\n '\n\n+test_expect_success 'setup 8' '\n+\n+ git checkout mv-ln &&\n+ git mv a e &&\n+ ln -s e a &&\n+ git add a e &&\n+ test_tick &&\n+ git commit -m \"rename a->e, symlink a->e\"\n+'\n+\n+test_expect_success 'setup 9' '\n+\n+ git checkout mv &&\n+ git mv a e &&\n+ git add e &&\n+ test_tick &&\n+ git commit -m \"rename a->e\"\n+'\n+\n test_expect_success 'merge-recursive simple' '\n\n rm -fr [abcd] &&\n@@ -580,4 +601,23 @@ test_expect_failure 'merge-recursive simple w/submodule result' '\n test_cmp expected actual\n '\n\n+test_expect_success 'merge-recursive rename vs. rename/symlink' '\n+\n+ git checkout -f mv &&\n+ git merge mv-ln &&\n+ ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&\n+ (\n+ echo \"100644 blob $o0 b\"\n+ echo \"100644 blob $o0 c\"\n+ echo \"100644 blob $o0 d/e\"\n+ echo \"100644 blob $o0 e\"\n+ echo \"100644 $o0 0 b\"\n+ echo \"100644 $o0 0 c\"\n+ echo \"100644 $o0 0 d/e\"\n+ echo \"100644 $o0 0 e\"\n+ ) >expected &&\n+ test_cmp expected actual\n+'\n+\n+\n test_done\n--\n1.7.0\n"},{"id":"149156","messageId":"AANLkTik2op0_Cq13EGit17ja+zCdmbM6WXJ=rfhQMnOQ@mail.gmail.com","threadId":"24685","inReplyTo":"EF9FEAB3A4B7D245B0801936B6EF4A25262E13@azsmsx503.amr.corp.intel.com","subject":"Re: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-28T10:12:09Z","receivedAt":"2010-08-28T10:12:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Aug 27, 2010 at 22:14, Schalk, Ken wrote:\n\n> +       ln -s e a &&\n\nDue to this this (and maybe all the tests) need to depend on the\nSYMLINKS prereq.\n"},{"id":"149496","messageId":"EF9FEAB3A4B7D245B0801936B6EF4A2533CEF8@azsmsx503.amr.corp.intel.com","threadId":"24685","inReplyTo":"AANLkTik2op0_Cq13EGit17ja+zCdmbM6WXJ=rfhQMnOQ@mail.gmail.com","subject":"RE: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Schalk, Ken","fromEmail":"ken.schalk@intel.com","sentAt":"2010-08-31T21:05:51Z","receivedAt":"2010-08-31T21:05:51Z","isPatch":true,"sender":{"key":"ken.schalk@intel.com","avatar":null},"body":">On Fri, Aug 27, 2010 at 22:14, Schalk, Ken wrote:\n\n>> + ln -s e a &&\n\n>Due to this this (and maybe all the tests) need to depend on the\n>SYMLINKS prereq.\n\nI used a symlink here to try and duplicate the original problem case that a git user at Intel brought to me. I admit I didn't consider the problem of testing on platforms without symlink support.\n\nI'm having a little difficulty generating a rename/add conflict inside this test without using a symlink, which seems odd to me as the symlink doesn't seem to be necessary in an experiment I did by hand. I'll either re-submit with this portion of the test conditional on symlink support, or with an alternate test that doesn't use symlinks (if I can get that to work in the test).\n\n--Ken\n"},{"id":"149589","messageId":"EF9FEAB3A4B7D245B0801936B6EF4A2533DBB8@azsmsx503.amr.corp.intel.com","threadId":"24685","inReplyTo":"AANLkTik2op0_Cq13EGit17ja+zCdmbM6WXJ=rfhQMnOQ@mail.gmail.com","subject":"RE: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Schalk, Ken","fromEmail":"ken.schalk@intel.com","sentAt":"2010-09-01T20:15:32Z","receivedAt":"2010-09-01T20:15:32Z","isPatch":true,"sender":{"key":"ken.schalk@intel.com","avatar":null},"body":">Due to this this (and maybe all the tests) need to depend on the\n>SYMLINKS prereq.\n\nHere's a third attempt with no use of symlinks in the test:\n\nSkip the entire rename/add conflict case if the file added on the\nother branch has the same contents as the file being renamed. This\navoids giving the user an extra copy of the same file and presenting a\nconflict that is confusing and pointless.\n\nA simple test of this case has been added in\nt/t3030-merge-recursive.sh.\n\nSigned-off-by: Ken Schalk \n---\n merge-recursive.c | 6 ++++++\n t/t3030-merge-recursive.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex fb6aa4a..a2fba84 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -955,6 +955,12 @@ static int process_renames(struct merge_options *o,\n ren1->pair->two : NULL,\n branch1 == o->branch1 ?\n NULL : ren1->pair->two, 1);\n+ } else if ((dst_other.mode == ren1->pair->two->mode) &&\n+ sha_eq(dst_other.sha1, ren1->pair->two->sha1)) {\n+ /* Added file on the other side\n+ identical to the file being\n+ renamed: clean merge */\n+ update_file(o, 1, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);\n } else if (!sha_eq(dst_other.sha1, null_sha1)) {\n const char *new_path;\n clean_merge = 0;\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex d541544..b23bd9f 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -23,6 +23,8 @@ test_expect_success 'setup 1' '\n git branch df-3 &&\n git branch remove &&\n git branch submod &&\n+ git branch copy &&\n+ git branch rename &&\n\n echo hello >>a &&\n cp a d/e &&\n@@ -248,6 +250,24 @@ test_expect_success 'setup 7' '\n git commit -m \"make d/ a submodule\"\n '\n\n+test_expect_success 'setup 8' '\n+\n+ git checkout rename &&\n+ git mv a e &&\n+ git add e &&\n+ test_tick &&\n+ git commit -m \"rename a->e\"\n+'\n+\n+test_expect_success 'setup 9' '\n+\n+ git checkout copy &&\n+ cp a e &&\n+ git add e &&\n+ test_tick &&\n+ git commit -m \"copy a->e\"\n+'\n+\n test_expect_success 'merge-recursive simple' '\n\n rm -fr [abcd] &&\n@@ -580,4 +600,23 @@ test_expect_failure 'merge-recursive simple w/submodule result' '\n test_cmp expected actual\n '\n\n+test_expect_success 'merge-recursive copy vs. rename' '\n+\n+ git checkout -f copy &&\n+ git merge rename &&\n+ ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&\n+ (\n+ echo \"100644 blob $o0 b\"\n+ echo \"100644 blob $o0 c\"\n+ echo \"100644 blob $o0 d/e\"\n+ echo \"100644 blob $o0 e\"\n+ echo \"100644 $o0 0 b\"\n+ echo \"100644 $o0 0 c\"\n+ echo \"100644 $o0 0 d/e\"\n+ echo \"100644 $o0 0 e\"\n+ ) >expected &&\n+ test_cmp expected actual\n+'\n+\n+\n test_done\n--\n1.7.0\n"},{"id":"149773","messageId":"7vbp8eznn7.fsf@alter.siamese.dyndns.org","threadId":"24685","inReplyTo":"EF9FEAB3A4B7D245B0801936B6EF4A2533DBB8@azsmsx503.amr.corp.intel.com","subject":"Re: [PATCH] Avoid rename/add conflict when contents are identical","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-03T18:29:32Z","receivedAt":"2010-09-03T18:29:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; the patch somehow had all tabs expanded, but I can fix it up.\n"}]}