{"thread":{"id":"25032","subject":"[PATCHv2 1/2] t3509: Add rename + D/F conflict testcase that recursive strategy fails","startedAt":"2010-09-08T06:40:39Z","lastAt":"2010-09-10T00:26:25Z","messageCount":5,"participants":["Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"150280","messageId":"1283928041-9882-1-git-send-email-newren@gmail.com","threadId":"25032","inReplyTo":null,"subject":"[PATCHv2 0/2] Fix resolvable rename + D/F conflict testcases","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-09-08T06:40:39Z","receivedAt":"2010-09-08T06:40:39Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This fixes an issue reported by Nick (no lastname given) on the\nmailing list, as well as a closely related issue in the handling of\nrename + directory/file conflicts, namely where a filename on one side\nof the rename is a directory name on the other side of the merge.\n\nChange since v1:\n  * Split unrelated 2/3 patch out into a separate submission\n\nElijah Newren (2):\n  t3509: Add rename + D/F conflict testcase that recursive strategy\n    fails\n  merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir\n\n merge-recursive.c               |   16 +++++-----\n t/t3509-cherry-pick-merge-df.sh |   66 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 74 insertions(+), 8 deletions(-)\n\n-- \n1.7.3.rc0.8.g2ec3f\n"},{"id":"150279","messageId":"1283928041-9882-2-git-send-email-newren@gmail.com","threadId":"25032","inReplyTo":"1283928041-9882-1-git-send-email-newren@gmail.com","subject":"[PATCHv2 1/2] t3509: Add rename + D/F conflict testcase that recursive strategy fails","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-09-08T06:40:40Z","receivedAt":"2010-09-08T06:40:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When one side of a file rename matches a directory name on the other side,\nthe recursive merge strategy will fail.  This is true even if the merge is\ntrivially resolvable.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t3509-cherry-pick-merge-df.sh |   66 +++++++++++++++++++++++++++++++++++++++\n 1 files changed, 66 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh\nindex a5ccdbf..eb5826f 100755\n--- a/t/t3509-cherry-pick-merge-df.sh\n+++ b/t/t3509-cherry-pick-merge-df.sh\n@@ -32,4 +32,70 @@ test_expect_success SYMLINKS 'Cherry-pick succeeds with rename across D/F confli\n \tgit cherry-pick branch\n '\n \n+test_expect_success 'Setup rename with file on one side matching directory name on other' '\n+\tgit checkout --orphan nick-testcase &&\n+\tgit rm -rf . &&\n+\n+\t>empty &&\n+\tgit add empty &&\n+\tgit commit -m \"Empty file\" &&\n+\n+\tgit checkout -b simple &&\n+\tmv empty file &&\n+\tmkdir empty &&\n+\tmv file empty &&\n+\tgit add empty/file &&\n+\tgit commit -m \"Empty file under empty dir\" &&\n+\n+\techo content >newfile &&\n+\tgit add newfile &&\n+\tgit commit -m \"New file\"\n+'\n+\n+test_expect_success 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (resolve)' '\n+\tgit reset --hard &&\n+\tgit checkout -q nick-testcase^0 &&\n+\tgit cherry-pick --strategy=resolve simple\n+'\n+\n+test_expect_failure 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (recursive)' '\n+\tgit reset --hard &&\n+\tgit checkout -q nick-testcase^0 &&\n+\tgit cherry-pick --strategy=recursive simple\n+'\n+\n+test_expect_success 'Setup rename with file on one side matching different dirname on other' '\n+\tgit reset --hard &&\n+\tgit checkout --orphan mergeme &&\n+\tgit rm -rf . &&\n+\n+\tmkdir sub &&\n+\tmkdir othersub &&\n+\techo content > sub/file &&\n+\techo foo > othersub/whatever &&\n+\tgit add -A &&\n+\tgit commit -m \"Common commmit\" &&\n+\n+\tgit rm -rf othersub &&\n+\tgit mv sub/file othersub &&\n+\tgit commit -m \"Commit to merge\" &&\n+\n+\tgit checkout -b newhead mergeme~1 &&\n+\t>independent-change &&\n+\tgit add independent-change &&\n+\tgit commit -m \"Completely unrelated change\"\n+'\n+\n+test_expect_success 'Cherry-pick with rename to different D/F conflict succeeds (resolve)' '\n+\tgit reset --hard &&\n+\tgit checkout -q newhead^0 &&\n+\tgit cherry-pick --strategy=resolve mergeme\n+'\n+\n+test_expect_failure 'Cherry-pick with rename to different D/F conflict succeeds (recursive)' '\n+\tgit reset --hard &&\n+\tgit checkout -q newhead^0 &&\n+\tgit cherry-pick --strategy=recursive mergeme\n+'\n+\n test_done\n-- \n1.7.3.rc0.8.g2ec3f\n"},{"id":"150281","messageId":"1283928041-9882-3-git-send-email-newren@gmail.com","threadId":"25032","inReplyTo":"1283928041-9882-1-git-send-email-newren@gmail.com","subject":"[PATCHv2 2/2] merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-09-08T06:40:41Z","receivedAt":"2010-09-08T06:40:41Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In merge-recursive.c, whenever there was a rename where a file name on one\nside of the rename matches a directory name on the other side of the merge,\nthen the very first check that\n  string_list_has_string(&o->current_directory_set, ren1_dst)\nwould trigger forcing it into marking it as a rename/directory conflict.\n\nHowever, if the path is only renamed on one side and a simple three-way\nmerge between the separate files resolves cleanly, then we don't need to\nmark it as a rename/directory conflict.  So, we can simply move the check\nfor rename/directory conflicts after we've verified that there isn't a\nrename/rename conflict and that a threeway content merge doesn't work.\n\nThis changes the particular error message one gets in the case where the\ndirectory name that a file on one side of the rename matches is not also\npart of the rename pair.  For example, with commits containing the files:\n  COMMON    -> (HEAD,           MERGE )\n  ---------    ---------------  -------\n  sub/file1 -> (sub/file1,      newsub)\n  <NULL>    -> (newsub/newfile, <NULL>)\nthen previously when one tried to merge MERGE into HEAD, one would get\n  CONFLICT (rename/directory): Rename sub/file1->newsub in HEAD  directory newsub added in merge\n  Renaming sub/file1 to newsub~HEAD instead\n  Adding newsub/newfile\n  Automatic merge failed; fix conflicts and then commit the result.\nAfter this patch, the error message will instead become:\n  Removing newsub\n  Adding newsub/newfile\n  CONFLICT (file/directory): There is a directory with name newsub in merge. Adding newsub as newsub~HEAD\n  Automatic merge failed; fix conflicts and then commit the result.\nThat makes more sense to me, because git can't know that there's a conflict\nuntil after it's tried resolving paths involving newsub/newfile to see if\nthey are still in the way at the end (and if newsub/newfile is not in the\nway at the end, there should be no conflict at all, which did not hold with\ngit previously).\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c               |   16 ++++++++--------\n t/t3509-cherry-pick-merge-df.sh |    4 ++--\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 20e1779..5ea6d11 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -935,14 +935,7 @@ static int process_renames(struct merge_options *o,\n \n \t\t\ttry_merge = 0;\n \n-\t\t\tif (string_list_has_string(&o->current_directory_set, ren1_dst)) {\n-\t\t\t\tclean_merge = 0;\n-\t\t\t\toutput(o, 1, \"CONFLICT (rename/directory): Rename %s->%s in %s \"\n-\t\t\t\t       \" directory %s added in %s\",\n-\t\t\t\t       ren1_src, ren1_dst, branch1,\n-\t\t\t\t       ren1_dst, branch2);\n-\t\t\t\tconflict_rename_dir(o, ren1, branch1);\n-\t\t\t} else if (sha_eq(src_other.sha1, null_sha1)) {\n+\t\t\tif (sha_eq(src_other.sha1, null_sha1)) {\n \t\t\t\tclean_merge = 0;\n \t\t\t\toutput(o, 1, \"CONFLICT (rename/delete): Rename %s->%s in %s \"\n \t\t\t\t       \"and deleted in %s\",\n@@ -1034,6 +1027,13 @@ static int process_renames(struct merge_options *o,\n \t\t\t\t\tif (!ren1->dst_entry->stages[2].mode !=\n \t\t\t\t\t    !ren1->dst_entry->stages[3].mode)\n \t\t\t\t\t\tren1->dst_entry->processed = 0;\n+\t\t\t\t} else if (string_list_has_string(&o->current_directory_set, ren1_dst)) {\n+\t\t\t\t\tclean_merge = 0;\n+\t\t\t\t\toutput(o, 1, \"CONFLICT (rename/directory): Rename %s->%s in %s \"\n+\t\t\t\t\t       \" directory %s added in %s\",\n+\t\t\t\t\t       ren1_src, ren1_dst, branch1,\n+\t\t\t\t\t       ren1_dst, branch2);\n+\t\t\t\t\tconflict_rename_dir(o, ren1, branch1);\n \t\t\t\t} else {\n \t\t\t\t\tif (mfi.merge || !mfi.clean)\n \t\t\t\t\t\toutput(o, 1, \"Renaming %s => %s\", ren1_src, ren1_dst);\ndiff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh\nindex eb5826f..948ca1b 100755\n--- a/t/t3509-cherry-pick-merge-df.sh\n+++ b/t/t3509-cherry-pick-merge-df.sh\n@@ -58,7 +58,7 @@ test_expect_success 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (reso\n \tgit cherry-pick --strategy=resolve simple\n '\n \n-test_expect_failure 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (recursive)' '\n+test_expect_success 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (recursive)' '\n \tgit reset --hard &&\n \tgit checkout -q nick-testcase^0 &&\n \tgit cherry-pick --strategy=recursive simple\n@@ -92,7 +92,7 @@ test_expect_success 'Cherry-pick with rename to different D/F conflict succeeds\n \tgit cherry-pick --strategy=resolve mergeme\n '\n \n-test_expect_failure 'Cherry-pick with rename to different D/F conflict succeeds (recursive)' '\n+test_expect_success 'Cherry-pick with rename to different D/F conflict succeeds (recursive)' '\n \tgit reset --hard &&\n \tgit checkout -q newhead^0 &&\n \tgit cherry-pick --strategy=recursive mergeme\n-- \n1.7.3.rc0.8.g2ec3f\n"},{"id":"150282","messageId":"AANLkTin0KidU3og7SVEVUAaRfMzafEvvme_vP-0V773E@mail.gmail.com","threadId":"25032","inReplyTo":"1283928041-9882-1-git-send-email-newren@gmail.com","subject":"Re: [PATCHv2 0/2] Fix resolvable rename + D/F conflict testcases","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-09-08T06:41:26Z","receivedAt":"2010-09-08T06:41:26Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 8, 2010 at 12:40 AM, Elijah Newren <newren@gmail.com> wrote:\n> This fixes an issue reported by Nick (no lastname given) on the\n> mailing list, as well as a closely related issue in the handling of\n> rename + directory/file conflicts, namely where a filename on one side\n> of the rename is a directory name on the other side of the merge.\n>\n> Change since v1:\n>  * Split unrelated 2/3 patch out into a separate submission\n\n...oh, and now the series is based on master instead of next.  (I\nwould base it on maint, but since I was adding another testcase to\nt3509 and t3509 does not yet exist in maint, I couldn't as easily put\nit there.)\n"},{"id":"150424","messageId":"7vhbhyjvf2.fsf@alter.siamese.dyndns.org","threadId":"25032","inReplyTo":"1283928041-9882-3-git-send-email-newren@gmail.com","subject":"Re: [PATCHv2 2/2] merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-10T00:26:25Z","receivedAt":"2010-09-10T00:26:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> ... git can't know that there's a conflict\n> until after it's tried resolving paths involving newsub/newfile to see if\n> they are still in the way at the end (and if newsub/newfile is not in the\n> way at the end, there should be no conflict at all, which did not hold with\n> git previously).\n\nI'll queue this patch to 'pu', but anybody who wrote the above to\ncorrectly arriave at the crux of the issue in his analysis would know that\nthis is another band-aid on top of band-aid.  The approach merge-recursive\ntakes to first grab the set of directories and files fundamentally is\nwrong---it should be resolving the paths in-core first and then look at\nthe result to ignore a directory that has become empty.\n"}]}