{"thread":{"id":"24314","subject":"[PATCHv3 0/6] D/F conflict fixes","startedAt":"2010-07-07T05:20:28Z","lastAt":"2010-07-08T16:03:30Z","messageCount":17,"participants":["newren@gmail.com","Junio C Hamano","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"144990","messageId":"1278480034-22939-1-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":null,"subject":"[PATCHv3 0/6] D/F conflict fixes","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:28Z","receivedAt":"2010-07-07T05:20:28Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This patch series fixes a number of spurious directory/file conflicts\nand associated bugs appearing in cherry-pick, rebase, merge, and\nfast-export. It also has a minor robustness improvement for\nfast-import. This series includes testsuite fixes for currently known\nfailures in both t6020-merge-df.sh and t6035-merge-dir-to-symlink.sh.\n\nThe right person to review most the changes (all but the trivial\nfast-import change that Shawn already commented on, modulo one minor new\nchange) is probably Dscho. In his absence, the next most logical\nreviewer as far as I can tell is probably Junio or perhaps Shawn. I\nhate to overwork them even more, so if anyone else has some time to take\na look or even do some simple testing, it'd be much appreciated.\n\nChanges since the previous submission:\n * Added a new patch (5/6) fixing fast-export -- Shawn pointed out\n in the previous round that the bug I attributed to fast-import was\n actually a fast-export issue\n * Modified the fast-import patch (6/6) to note that it was just a\n robustness improvement rather than bugfix, and extended the patch to\n also handle regular files in addition to symlinks\n\nAlexander Gladysh (1):\n Add a rename + D/F conflict testcase\n\nElijah Newren (5):\n Add additional testcases for D/F conflicts\n merge-recursive: Fix D/F conflicts\n merge_recursive: Fix renames across paths below D/F conflicts\n fast-export: Fix output order of D/F changes\n fast-import: Improve robustness when D->F changes provided in wrong order\n\n builtin/fast-export.c | 1 +\n diff.h | 1 +\n fast-import.c | 7 +++\n merge-recursive.c | 106 ++++++++++++++++++++++++++++++++-------\n t/t3508-cherry-pick-merge-df.sh | 34 ++++++++++++\n t/t6020-merge-df.sh | 2 +-\n t/t6035-merge-dir-to-symlink.sh | 37 +++++++++++++-\n t/t9350-fast-export.sh | 24 +++++++++\n tree-diff.c | 4 +-\n 9 files changed, 194 insertions(+), 22 deletions(-)\n"},{"id":"144991","messageId":"1278480034-22939-2-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":"1278480034-22939-1-git-send-email-newren@gmail.com","subject":"[PATCHv3 1/6] Add additional testcases for D/F conflicts","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:29Z","receivedAt":"2010-07-07T05:20:29Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n\nSigned-off-by: Elijah Newren \n---\n t/t6035-merge-dir-to-symlink.sh | 37 +++++++++++++++++++++++++++++++++++--\n t/t9350-fast-export.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\nindex 3202e1d..3c176d7 100755\n--- a/t/t6035-merge-dir-to-symlink.sh\n+++ b/t/t6035-merge-dir-to-symlink.sh\n@@ -56,7 +56,7 @@ test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '\n \ttest -f a/b-2/c/d\n '\n \n-test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '\n+test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '\n \tgit reset --hard &&\n \tgit checkout baseline^0 &&\n \tgit merge -s recursive master &&\n@@ -64,6 +64,31 @@ test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '\n \ttest -f a/b-2/c/d\n '\n \n+test_expect_failure 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '\n+\tgit reset --hard &&\n+\tgit checkout master &&\n+\tgit merge -s recursive baseline^0 &&\n+\ttest -h a/b &&\n+\ttest -f a/b-2/c/d\n+'\n+\n+test_expect_success 'do not lose untracked in merge (recursive)' '\n+\tgit reset --hard &&\n+\tgit checkout baseline^0 &&\n+\ttouch a/b/c/e &&\n+\ttest_must_fail git merge -s recursive master &&\n+\ttest -f a/b/c/e &&\n+\ttest -f a/b-2/c/d\n+'\n+\n+test_expect_success 'do not lose modifications in merge (recursive)' '\n+\tgit add --all &&\n+\tgit reset --hard &&\n+\tgit checkout baseline^0 &&\n+\techo more content >> a/b/c/d &&\n+\ttest_must_fail git merge -s recursive master\n+'\n+\n test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '\n \tgit reset --hard &&\n \tgit checkout start^0 &&\n@@ -82,7 +107,7 @@ test_expect_success 'merge should not have conflicts (resolve)' '\n \ttest -f a/b/c/d\n '\n \n-test_expect_failure 'merge should not have conflicts (recursive)' '\n+test_expect_failure 'merge should not have D/F conflicts (recursive)' '\n \tgit reset --hard &&\n \tgit checkout baseline^0 &&\n \tgit merge -s recursive test2 &&\n@@ -90,4 +115,12 @@ test_expect_failure 'merge should not have conflicts (recursive)' '\n \ttest -f a/b/c/d\n '\n \n+test_expect_failure 'merge should not have F/D conflicts (recursive)' '\n+\tgit reset --hard &&\n+\tgit checkout -b foo test2 &&\n+\tgit merge -s recursive baseline^0 &&\n+\ttest -h a/b-2 &&\n+\ttest -f a/b/c/d\n+'\n+\n test_done\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex d43f37c..69179c6 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -376,4 +376,28 @@ test_expect_success 'tree_tag-obj' 'git fast-export tree_tag-obj'\n test_expect_success 'tag-obj_tag' 'git fast-export tag-obj_tag'\n test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'\n \n+test_expect_failure 'directory becomes symlink' '\n+\tgit init dirtosymlink &&\n+\tgit init result &&\n+\t(\n+\t\tcd dirtosymlink &&\n+\t\tmkdir foo &&\n+\t\tmkdir bar &&\n+\t\techo hello > foo/world &&\n+\t\techo hello > bar/world &&\n+\t\tgit add foo/world bar/world &&\n+\t\tgit commit -q -mone &&\n+\t\tgit rm -r foo &&\n+\t\tln -s bar foo &&\n+\t\tgit add foo &&\n+\t\tgit commit -q -mtwo\n+\t) &&\n+\t(\n+\t\tcd dirtosymlink &&\n+\t\tgit fast-export master -- foo |\n+\t\t(cd ../result && git fast-import --quiet)\n+\t) &&\n+\t(cd result && git show master:foo)\n+'\n+\n test_done\n-- \n1.7.1.1.10.g2e807\n"},{"id":"144992","messageId":"1278480034-22939-3-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":"1278480034-22939-1-git-send-email-newren@gmail.com","subject":"[PATCHv3 2/6] Add a rename + D/F conflict testcase","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:30Z","receivedAt":"2010-07-07T05:20:30Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Alexander Gladysh \n\nThis is a simple testcase where both sides of the rename are paths involved\nin (separate) D/F merge conflicts\n\nSigned-off-by: Alexander Gladysh \nSigned-off-by: Elijah Newren \n---\nCopying my additional comments from round 2:\n\n As noted previously this is simply a testcase Alexander sent to the\n list on March 8 which I'm submitting in the form of a testsuite\n addition. He's happy to have his testcase added to the testsuite\n and/or even submit it himself if that makes more sense -- just let\n us know what is wanted. I'm less familiar with format-patch and am;\n hopefully this submission comes across with him recorded as the\n author.\n\n t/t3508-cherry-pick-merge-df.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 files changed, 34 insertions(+), 0 deletions(-)\n create mode 100755 t/t3508-cherry-pick-merge-df.sh\n\ndiff --git a/t/t3508-cherry-pick-merge-df.sh b/t/t3508-cherry-pick-merge-df.sh\nnew file mode 100755\nindex 0000000..646a56d\n--- /dev/null\n+++ b/t/t3508-cherry-pick-merge-df.sh\n@@ -0,0 +1,34 @@\n+#!/bin/sh\n+\n+test_description='Test cherry-pick with directory/file conflicts'\n+. ./test-lib.sh\n+\n+test_expect_success 'Setup rename across paths each below D/F conflicts' '\n+\tmkdir a &&\n+\ttouch a/f &&\n+\tgit add a &&\n+\tgit commit -m \"a\" &&\n+\n+\tmkdir b &&\n+\tln -s ../a b/a &&\n+\tgit add b &&\n+\tgit commit -m \"b\" &&\n+\n+\tgit checkout -b branch &&\n+\trm b/a &&\n+\tmv a b/ &&\n+\tln -s b/a a &&\n+\tgit add . &&\n+\tgit commit -m \"swap\" &&\n+\n+\ttouch f1 &&\n+\tgit add f1 &&\n+\tgit commit -m \"f1\"\n+'\n+\n+test_expect_failure 'Cherry-pick succeeds with rename across D/F conflicts' '\n+\tgit checkout master &&\n+\tgit cherry-pick branch\n+'\n+\n+test_done\n-- \n1.7.1.1.10.g2e807\n"},{"id":"144993","messageId":"1278480034-22939-4-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":"1278480034-22939-1-git-send-email-newren@gmail.com","subject":"[PATCHv3 3/6] merge-recursive: Fix D/F conflicts","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:31Z","receivedAt":"2010-07-07T05:20:31Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe D/F conflicts that can be automatically resolved (file or directory\nunmodified on one side of history), have the nice property that\nprocess_entry() can correctly handle all subpaths of the D/F conflict. In\nthe case of D->F conversions, it will correctly delete all non-conflicting\nfiles below the relevant directory and the directory itself (note that both\nuntracked and conflicting files below the directory will prevent its\nremoval). So if we handle D/F conflicts after all other conflicts, they\nbecome fairly simple to handle -- we just need to check for whether or not\na path (file/directory) is in the way of creating the new content. We do\nthis by having process_entry() defer handling such entries to a subsequent\nprocess_df_entry() step.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 93 ++++++++++++++++++++++++++++++++-------\n t/t6035-merge-dir-to-symlink.sh | 8 ++--\n 2 files changed, 81 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 206c103..865729a 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1072,6 +1072,7 @@ static int process_entry(struct merge_options *o,\n \tunsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);\n \tunsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);\n \n+\tentry->processed = 1;\n \tif (o_sha && (!a_sha || !b_sha)) {\n \t\t/* Case A: Deleted in one */\n \t\tif ((!a_sha && !b_sha) ||\n@@ -1104,33 +1105,28 @@ static int process_entry(struct merge_options *o,\n \t} else if ((!o_sha && a_sha && !b_sha) ||\n \t\t (!o_sha && !a_sha && b_sha)) {\n \t\t/* Case B: Added in one. */\n-\t\tconst char *add_branch;\n-\t\tconst char *other_branch;\n \t\tunsigned mode;\n \t\tconst unsigned char *sha;\n-\t\tconst char *conf;\n \n \t\tif (a_sha) {\n-\t\t\tadd_branch = o->branch1;\n-\t\t\tother_branch = o->branch2;\n \t\t\tmode = a_mode;\n \t\t\tsha = a_sha;\n-\t\t\tconf = \"file/directory\";\n \t\t} else {\n-\t\t\tadd_branch = o->branch2;\n-\t\t\tother_branch = o->branch1;\n \t\t\tmode = b_mode;\n \t\t\tsha = b_sha;\n-\t\t\tconf = \"directory/file\";\n \t\t}\n \t\tif (string_list_has_string(&o->current_directory_set, path)) {\n-\t\t\tconst char *new_path = unique_path(o, path, add_branch);\n-\t\t\tclean_merge = 0;\n-\t\t\toutput(o, 1, \"CONFLICT (%s): There is a directory with name %s in %s. \"\n-\t\t\t \"Adding %s as %s\",\n-\t\t\t conf, path, other_branch, path, new_path);\n-\t\t\tremove_file(o, 0, path, 0);\n-\t\t\tupdate_file(o, 0, sha, mode, new_path);\n+\t\t\t/* Handle D->F conflicts after all subfiles */\n+\t\t\tentry->processed = 0;\n+\t\t\t/* But get any file out of the way now, so conflicted\n+\t\t\t * entries below the directory of the same name can\n+\t\t\t * be put in the working directory.\n+\t\t\t */\n+\t\t\tif (a_sha)\n+\t\t\t\toutput(o, 2, \"Removing %s\", path);\n+\t\t\t/* do not touch working file if it did not exist */\n+\t\t\tremove_file(o, 0, path, !a_sha);\n+\t\t\treturn 1; /* Assume clean till processed */\n \t\t} else {\n \t\t\toutput(o, 2, \"Adding %s\", path);\n \t\t\tupdate_file(o, 1, sha, mode, path);\n@@ -1178,6 +1174,64 @@ static int process_entry(struct merge_options *o,\n \treturn clean_merge;\n }\n \n+/* Per entry merge function for D/F conflicts, to be called only after\n+ * all files below dir have been processed. We do this because in the\n+ * cases we can cleanly resolve D/F conflicts, process_entry() can clean\n+ * out all the files below the directory for us.\n+ */\n+static int process_df_entry(struct merge_options *o,\n+\t\t\t const char *path, struct stage_data *entry)\n+{\n+\tint clean_merge = 1;\n+\tunsigned o_mode = entry->stages[1].mode;\n+\tunsigned a_mode = entry->stages[2].mode;\n+\tunsigned b_mode = entry->stages[3].mode;\n+\tunsigned char *o_sha = stage_sha(entry->stages[1].sha, o_mode);\n+\tunsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);\n+\tunsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);\n+\n+\t/* We currently only handle D->F cases */\n+\tassert((!o_sha && a_sha && !b_sha) ||\n+\t (!o_sha && !a_sha && b_sha));\n+\n+\tconst char *add_branch;\n+\tconst char *other_branch;\n+\tunsigned mode;\n+\tconst unsigned char *sha;\n+\tconst char *conf;\n+\n+\tentry->processed = 1;\n+\n+\tif (a_sha) {\n+\t\tadd_branch = o->branch1;\n+\t\tother_branch = o->branch2;\n+\t\tmode = a_mode;\n+\t\tsha = a_sha;\n+\t\tconf = \"file/directory\";\n+\t} else {\n+\t\tadd_branch = o->branch2;\n+\t\tother_branch = o->branch1;\n+\t\tmode = b_mode;\n+\t\tsha = b_sha;\n+\t\tconf = \"directory/file\";\n+\t}\n+\tstruct stat st;\n+\tif (lstat(path, &st) == 0 && S_ISDIR(st.st_mode)) {\n+\t\tconst char *new_path = unique_path(o, path, add_branch);\n+\t\tclean_merge = 0;\n+\t\toutput(o, 1, \"CONFLICT (%s): There is a directory with name %s in %s. \"\n+\t\t \"Adding %s as %s\",\n+\t\t conf, path, other_branch, path, new_path);\n+\t\tremove_file(o, 0, path, 0);\n+\t\tupdate_file(o, 0, sha, mode, new_path);\n+\t} else {\n+\t\toutput(o, 2, \"Adding %s\", path);\n+\t\tupdate_file(o, 1, sha, mode, path);\n+\t}\n+\n+\treturn clean_merge;\n+}\n+\n struct unpack_trees_error_msgs get_porcelain_error_msgs(void)\n {\n \tstruct unpack_trees_error_msgs msgs = {\n@@ -1249,6 +1303,13 @@ int merge_trees(struct merge_options *o,\n \t\t\t\t&& !process_entry(o, path, e))\n \t\t\t\tclean = 0;\n \t\t}\n+\t\tfor (i = 0; i < entries->nr; i++) {\n+\t\t\tconst char *path = entries->items[i].string;\n+\t\t\tstruct stage_data *e = entries->items[i].util;\n+\t\t\tif (!e->processed\n+\t\t\t\t&& !process_df_entry(o, path, e))\n+\t\t\t\tclean = 0;\n+\t\t}\n \n \t\tstring_list_clear(re_merge, 0);\n \t\tstring_list_clear(re_head, 0);\ndiff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\nindex 3c176d7..384547d 100755\n--- a/t/t6035-merge-dir-to-symlink.sh\n+++ b/t/t6035-merge-dir-to-symlink.sh\n@@ -56,7 +56,7 @@ test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '\n \ttest -f a/b-2/c/d\n '\n \n-test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '\n+test_expect_success 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '\n \tgit reset --hard &&\n \tgit checkout baseline^0 &&\n \tgit merge -s recursive master &&\n@@ -64,7 +64,7 @@ test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recurs\n \ttest -f a/b-2/c/d\n '\n \n-test_expect_failure 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '\n+test_expect_success 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '\n \tgit reset --hard &&\n \tgit checkout master &&\n \tgit merge -s recursive baseline^0 &&\n@@ -107,7 +107,7 @@ test_expect_success 'merge should not have conflicts (resolve)' '\n \ttest -f a/b/c/d\n '\n \n-test_expect_failure 'merge should not have D/F conflicts (recursive)' '\n+test_expect_success 'merge should not have D/F conflicts (recursive)' '\n \tgit reset --hard &&\n \tgit checkout baseline^0 &&\n \tgit merge -s recursive test2 &&\n@@ -115,7 +115,7 @@ test_expect_failure 'merge should not have D/F conflicts (recursive)' '\n \ttest -f a/b/c/d\n '\n \n-test_expect_failure 'merge should not have F/D conflicts (recursive)' '\n+test_expect_success 'merge should not have F/D conflicts (recursive)' '\n \tgit reset --hard &&\n \tgit checkout -b foo test2 &&\n \tgit merge -s recursive baseline^0 &&\n-- \n1.7.1.1.10.g2e807\n"},{"id":"144994","messageId":"1278480034-22939-5-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":"1278480034-22939-1-git-send-email-newren@gmail.com","subject":"[PATCHv3 4/6] merge_recursive: Fix renames across paths below D/F conflicts","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:32Z","receivedAt":"2010-07-07T05:20:32Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe rename logic in process_renames() handles renames and merging of file\ncontents and then marks files as processed. However, there may be higher\nstage entries left in the index for other reasons (e.g., due to D/F\nconflicts). By checking for such cases and marking the entry as not\nprocessed, it allows process_entry() later to look at it and handle those\nhigher stages.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 13 +++++++++++--\n t/t3508-cherry-pick-merge-df.sh | 2 +-\n t/t6020-merge-df.sh | 2 +-\n 3 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 865729a..ecddd9e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1019,14 +1019,23 @@ static int process_renames(struct merge_options *o,\n \n \t\t\t\tif (mfi.clean &&\n \t\t\t\t sha_eq(mfi.sha, ren1->pair->two->sha1) &&\n-\t\t\t\t mfi.mode == ren1->pair->two->mode)\n+\t\t\t\t mfi.mode == ren1->pair->two->mode) {\n \t\t\t\t\t/*\n \t\t\t\t\t * This messaged is part of\n \t\t\t\t\t * t6022 test. If you change\n \t\t\t\t\t * it update the test too.\n \t\t\t\t\t */\n \t\t\t\t\toutput(o, 3, \"Skipped %s (merged same as existing)\", ren1_dst);\n-\t\t\t\telse {\n+\n+\t\t\t\t\t/* There may be higher stage entries left\n+\t\t\t\t\t * in the index (e.g. due to a D/F\n+\t\t\t\t\t * conflict) that need to be resolved.\n+\t\t\t\t\t */\n+\t\t\t\t\tfor (i=1; i<=3; ++i) {\n+\t\t\t\t\t\tif (ren1->dst_entry->stages[i].mode)\n+\t\t\t\t\t\t\tren1->dst_entry->processed = 0;\n+\t\t\t\t\t}\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);\n \t\t\t\t\tif (mfi.merge)\ndiff --git a/t/t3508-cherry-pick-merge-df.sh b/t/t3508-cherry-pick-merge-df.sh\nindex 646a56d..5895ea3 100755\n--- a/t/t3508-cherry-pick-merge-df.sh\n+++ b/t/t3508-cherry-pick-merge-df.sh\n@@ -26,7 +26,7 @@ test_expect_success 'Setup rename across paths each below D/F conflicts' '\n \tgit commit -m \"f1\"\n '\n \n-test_expect_failure 'Cherry-pick succeeds with rename across D/F conflicts' '\n+test_expect_success 'Cherry-pick succeeds with rename across D/F conflicts' '\n \tgit checkout master &&\n \tgit cherry-pick branch\n '\ndiff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh\nindex e71c687..490d397 100755\n--- a/t/t6020-merge-df.sh\n+++ b/t/t6020-merge-df.sh\n@@ -22,7 +22,7 @@ git commit -m \"File: dir\"'\n \n test_expect_code 1 'Merge with d/f conflicts' 'git merge \"merge msg\" B master'\n \n-test_expect_failure 'F/D conflict' '\n+test_expect_success 'F/D conflict' '\n \tgit reset --hard &&\n \tgit checkout master &&\n \trm .git/index &&\n-- \n1.7.1.1.10.g2e807\n"},{"id":"144995","messageId":"1278480034-22939-6-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":"1278480034-22939-1-git-send-email-newren@gmail.com","subject":"[PATCHv3 5/6] fast-export: Fix output order of D/F changes","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:33Z","receivedAt":"2010-07-07T05:20:33Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe fast-import stream format requires incremental changes which take place\nimmediately, meaning that for D->F conversions all files below the relevant\ndirectory must be deleted before the resulting file of the same name is\ncreated. Reversing the order can result in fast-import silently deleting\nthe file right after creating it, resulting in the file missing from the\nresulting repository.\n\nWe correct the order by instructing the diff_tree machinery to compare\nentries using df_name_compare instead of base_name_compare.\n\nSigned-off-by: Elijah Newren \n---\n builtin/fast-export.c | 1 +\n diff.h | 1 +\n t/t9350-fast-export.sh | 2 +-\n tree-diff.c | 4 +++-\n 4 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex c6dd71a..de6349f 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -614,6 +614,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\tdie(\"revision walk setup failed\");\n \trevs.diffopt.format_callback = show_filemodify;\n \tDIFF_OPT_SET(&revs.diffopt, RECURSIVE);\n+\tDIFF_OPT_SET(&revs.diffopt, COMPARE_DF_EQUAL);\n \twhile ((commit = get_revision(&revs))) {\n \t\tif (has_unshown_parent(commit)) {\n \t\t\tadd_object_array(&commit->object, NULL, &commits);\ndiff --git a/diff.h b/diff.h\nindex 6a71013..6e1a1db 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -71,6 +71,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_SUBMODULE_LOG (1 << 23)\n #define DIFF_OPT_DIRTY_SUBMODULES (1 << 24)\n #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)\n+#define DIFF_OPT_COMPARE_DF_EQUAL (1 << 26)\n \n #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 69179c6..1ee1461 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -376,7 +376,7 @@ test_expect_success 'tree_tag-obj' 'git fast-export tree_tag-obj'\n test_expect_success 'tag-obj_tag' 'git fast-export tag-obj_tag'\n test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'\n \n-test_expect_failure 'directory becomes symlink' '\n+test_expect_success 'directory becomes symlink' '\n \tgit init dirtosymlink &&\n \tgit init result &&\n \t(\ndiff --git a/tree-diff.c b/tree-diff.c\nindex fe9f52c..03f93b7 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -40,7 +40,9 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \n \tpathlen1 = tree_entry_len(path1, sha1);\n \tpathlen2 = tree_entry_len(path2, sha2);\n-\tcmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n+\tcmp = DIFF_OPT_TST(opt, COMPARE_DF_EQUAL) ?\n+\t df_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2) :\n+\t base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n \tif (cmp < 0) {\n \t\tshow_entry(opt, \"-\", t1, base, baselen);\n \t\treturn -1;\n-- \n1.7.1.1.10.g2e807\n"},{"id":"144996","messageId":"1278480034-22939-7-git-send-email-newren@gmail.com","threadId":"24314","inReplyTo":"1278480034-22939-1-git-send-email-newren@gmail.com","subject":"[PATCHv3 6/6] fast-import: Improve robustness when D->F changes provided in wrong order","fromName":"","fromEmail":"newren@gmail.com","sentAt":"2010-07-07T05:20:34Z","receivedAt":"2010-07-07T05:20:34Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen older versions of fast-export came across a directory changing to a\nsymlink (or regular file), it would output the changes in the form\n M 120000 :239821 dir-changing-to-symlink\n D dir-changing-to-symlink/filename1\nWhen fast-import sees the first line, it deletes the directory named\ndir-changing-to-symlink (and any files below it) and creates a symlink in\nits place. When fast-import came across the second line, it was previously\ntrying to remove the file and relevant leading directories in\ntree_content_remove(), and as a side effect it would delete the symlink\nthat was just created. This resulted in the symlink silently missing from\nthe resulting repository.\n\nTo improve robustness, we ignore file deletions underneath directory names\nthat correspond to non-directories. This can also be viewed as a minor\noptimization: since there cannot be a file and a directory with the same\nname in the same directory, the file clearly can't exist so nothing needs\nto be done to delete it.\n\nSigned-off-by: Elijah Newren \n---\nQuoting Shawn's comments on the previous version of this patch:\n\n I'm not against making the input parser more robust...\n\n It probably makes sense to still do this in fast-import, deleting\n something that doesn't exist is probably OK, its still going to\n be deleted in the end anyway.\n\nI'm guessing that probably qualifies as an Acked-by, but I'm not quite\nsure. However, I made a minor change to the patch: the previous\nversion only handled directory->symlink changes (S_ISLNK), whereas\nthis one also handles directory->(normal)file changes and\ndirectory->gitlink changes (!S_ISDIR).\n\n fast-import.c | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 309f2c5..c8c717a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1528,6 +1528,13 @@ static int tree_content_remove(\n \tfor (i = 0; i < t->entry_count; i++) {\n \t\te = t->entries[i];\n \t\tif (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {\n+\t\t\tif (slash1 && !S_ISDIR(e->versions[1].mode))\n+\t\t\t\t/* If p names a file in some subdirectory, and a\n+\t\t\t\t * file or symlink matching the name of the\n+\t\t\t\t * parent directory of p exists, then p cannot\n+\t\t\t\t * exist and need not be deleted.\n+\t\t\t\t */\n+\t\t\t\treturn 1;\n \t\t\tif (!slash1 || !S_ISDIR(e->versions[1].mode))\n \t\t\t\tgoto del_entry;\n \t\t\tif (!e->tree)\n-- \n1.7.1.1.10.g2e807\n"},{"id":"145057","messageId":"7vocejt25d.fsf@alter.siamese.dyndns.org","threadId":"24314","inReplyTo":"1278480034-22939-2-git-send-email-newren@gmail.com","subject":"Re: [PATCHv3 1/6] Add additional testcases for D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-07T21:25:50Z","receivedAt":"2010-07-07T21:25:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"newren@gmail.com writes:\n\n> From: Elijah Newren \n>\n> Signed-off-by: Elijah Newren \n> ---\n> t/t6035-merge-dir-to-symlink.sh | 37 +++++++++++++++++++++++++++++++++++--\n> t/t9350-fast-export.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 59 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\n> index 3202e1d..3c176d7 100755\n> --- a/t/t6035-merge-dir-to-symlink.sh\n> +++ b/t/t6035-merge-dir-to-symlink.sh\n> @@ -56,7 +56,7 @@ test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '\n> \ttest -f a/b-2/c/d\n> '\n> \n> -test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '\n> +test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '\n\nIf you retitle this, you would probably want to retitle the corresponding\ntest for resolve (the one before this test) the same way.\n\n> +test_expect_failure 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '\n> +\tgit reset --hard &&\n> +\tgit checkout master &&\n> +\tgit merge -s recursive baseline^0 &&\n> +\ttest -h a/b &&\n> +\ttest -f a/b-2/c/d\n> +'\n\nIt may be a good idea to see what resolve does for all these new\ntests.\n\nYou would not want to do the above test while \"master\" branch is checked\nout. Once this is fixed, the commit subsequent tests refer to as \"master\"\nwill be a different one, and that affects their outcome in a big way.\nAvoid unnecessarily making tests depend on earlier ones.\n\n> +test_expect_failure 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '\n> +\tgit reset --hard &&\n> +\tgit checkout master &&\n\nLikewise.\n\n> +\tgit merge -s recursive baseline^0 &&\n> +\ttest -h a/b &&\n> +\ttest -f a/b-2/c/d\n> +'\n> +\n> +test_expect_success 'do not lose untracked in merge (recursive)' '\n> +\tgit reset --hard &&\n> +\tgit checkout baseline^0 &&\n> +\ttouch a/b/c/e &&\n> +\ttest_must_fail git merge -s recursive master &&\n> +\ttest -f a/b/c/e &&\n> +\ttest -f a/b-2/c/d\n> +'\n\nI suspect resolve may fail this one, but as you don't have a test for it, we\nwon't know...\n\n> +test_expect_success 'do not lose modifications in merge (recursive)' '\n> +\tgit add --all &&\n> +\tgit reset --hard &&\n> +\tgit checkout baseline^0 &&\n> +\techo more content >> a/b/c/d &&\n> +\ttest_must_fail git merge -s recursive master\n> +'\n> +\n> test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '\n> \tgit reset --hard &&\n> \tgit checkout start^0 &&\n\n> @@ -82,7 +107,7 @@ test_expect_success 'merge should not have conflicts (resolve)' '\n> \ttest -f a/b/c/d\n> '\n> \n> -test_expect_failure 'merge should not have conflicts (recursive)' '\n> +test_expect_failure 'merge should not have D/F conflicts (recursive)' '\n\nThe same comment on retitling applies here.\n"},{"id":"145058","messageId":"7vhbkbt24d.fsf@alter.siamese.dyndns.org","threadId":"24314","inReplyTo":"1278480034-22939-3-git-send-email-newren@gmail.com","subject":"Re: [PATCHv3 2/6] Add a rename + D/F conflict testcase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-07T21:26:26Z","receivedAt":"2010-07-07T21:26:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"newren@gmail.com writes:\n\n> From: Alexander Gladysh \n>\n> This is a simple testcase where both sides of the rename are paths involved\n> in (separate) D/F merge conflicts\n>\n> Signed-off-by: Alexander Gladysh \n> Signed-off-by: Elijah Newren \n> ---\n> Copying my additional comments from round 2:\n>\n> As noted previously this is simply a testcase Alexander sent to the\n> list on March 8 which I'm submitting in the form of a testsuite\n> addition. He's happy to have his testcase added to the testsuite\n> and/or even submit it himself if that makes more sense -- just let\n> us know what is wanted. I'm less familiar with format-patch and am;\n> hopefully this submission comes across with him recorded as the\n> author.\n>\n> t/t3508-cherry-pick-merge-df.sh | 34 ++++++++++++++++++++++++++++++++++\n\nAs t3508 is already used elsewhere I'll rename this to 3509, with minor\nstyle fixes as well.\n\n> diff --git a/t/t3508-cherry-pick-merge-df.sh b/t/t3508-cherry-pick-merge-df.sh\n> new file mode 100755\n> index 0000000..646a56d\n> --- /dev/null\n> +++ b/t/t3508-cherry-pick-merge-df.sh\n> @@ -0,0 +1,34 @@\n> ...\n> +\ttouch f1 &&\n> +\tgit add f1 &&\n> +\tgit commit -m \"f1\"\n> +'\n> +\n> +test_expect_failure 'Cherry-pick succeeds with rename across D/F conflicts' '\n> +\tgit checkout master &&\n> +\tgit cherry-pick branch\n> +'\n\nThis is curious. \"branch\" only adds a totally unrelated path to its\nparent, and picking it should be trivial. I wonder what it does\nhere if we used the resolve strategy...\n"},{"id":"145063","messageId":"7v630rt1h3.fsf@alter.siamese.dyndns.org","threadId":"24314","inReplyTo":"1278480034-22939-4-git-send-email-newren@gmail.com","subject":"Re: [PATCHv3 3/6] merge-recursive: Fix D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-07T21:40:24Z","receivedAt":"2010-07-07T21:40:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"newren@gmail.com writes:\n\n> From: Elijah Newren \n>\n> The D/F conflicts that can be automatically resolved (file or directory\n> unmodified on one side of history), have the nice property that\n> process_entry() can correctly handle all subpaths of the D/F conflict. In\n> the case of D->F conversions, it will correctly delete all non-conflicting\n> files below the relevant directory and the directory itself (note that both\n> untracked and conflicting files below the directory will prevent its\n> removal). So if we handle D/F conflicts after all other conflicts, they\n> become fairly simple to handle -- we just need to check for whether or not\n> a path (file/directory) is in the way of creating the new content. We do\n> this by having process_entry() defer handling such entries to a subsequent\n> process_df_entry() step.\n\nThe basic idea of this patch to process all removals first and then\nprocess additions in a separate step feels very sound. It is exactly the\nsame strategy that is used by \"git apply\" to avoid a similar issue of\napplying a patch that removes \"a/b\" and creates \"a\" at the same time.\n\nAnd it feels both correct and sufficient to have this additional logic\nonly in the \"Added in one, but the path happens to be a directory on the\nother branch\" codepath. The only case you will end up having to create D\n(blob) where D used to be a directory (hence needing to make sure\neverything under D/ is gone) is when you are the only one adding D (and\nthe other party is not adding nor modifying it---if it were, it means that\nthe other party also wants D as a blob and not a directory, and there\nwon't be D/F conflict in such a case).\n\n> Signed-off-by: Elijah Newren \n> ---\n> merge-recursive.c | 93 ++++++++++++++++++++++++++++++++-------\n> t/t6035-merge-dir-to-symlink.sh | 8 ++--\n> 2 files changed, 81 insertions(+), 20 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 206c103..865729a 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1072,6 +1072,7 @@ static int process_entry(struct merge_options *o,\n> \tunsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);\n> \tunsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);\n> \n> +\tentry->processed = 1;\n\nIn the original code, process_renames() used to use this to signal the\nloop that drives process_entry() that the stage data has been dealt with,\nand this function was unaware of the flag. Now it uses it to communicate\nthe same fact to its downstream (process_df_entry), which makes sense, but\nsomehow it made me feel a bit uneasy.\n\n> @@ -1178,6 +1174,64 @@ static int process_entry(struct merge_options *o,\n> \treturn clean_merge;\n> }\n> \n> +/* Per entry merge function for D/F conflicts, to be called only after\n> + * all files below dir have been processed. We do this because in the\n> + * cases we can cleanly resolve D/F conflicts, process_entry() can clean\n> + * out all the files below the directory for us.\n> + */\n\n/*\n * Please format large comment like\n * this.\n */\n\nStyle aside, what happens when we cannot cleanly resolve D/F conflicts,\ni.e. when some untracked/modified paths still remain after the earlier\nremoval? We would signal conflict but at that point what happens to the\nfiles in the working tree that are involved in this new codepath? They\nare already gone, I presume, and they at least match the original index\nentries, so not much is lost, but what would the recovery procedure out of\nthe resulting mess? \"reset --hard\"?\n\n> +static int process_df_entry(struct merge_options *o,\n> +\t\t\t const char *path, struct stage_data *entry)\n> +{\n> +\tint clean_merge = 1;\n> +\tunsigned o_mode = entry->stages[1].mode;\n> +\tunsigned a_mode = entry->stages[2].mode;\n> +\tunsigned b_mode = entry->stages[3].mode;\n> +\tunsigned char *o_sha = stage_sha(entry->stages[1].sha, o_mode);\n> +\tunsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);\n> +\tunsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);\n> +\n> +\t/* We currently only handle D->F cases */\n> +\tassert((!o_sha && a_sha && !b_sha) ||\n> +\t (!o_sha && !a_sha && b_sha));\n> +\tconst char *add_branch;\n> +\tconst char *other_branch;\n> +\tunsigned mode;\n> +\tconst unsigned char *sha;\n> +\tconst char *conf;\n\nAs an assert becomes code under debugging build, do not put one before\ndeclarations like this.\n\nAlso \"struct stat st\" later in this file needs to be lifted up for the\nsame reason to avoid decl-after-statement.\n\nThanks.\n"},{"id":"145060","messageId":"7vy6dnrmvu.fsf@alter.siamese.dyndns.org","threadId":"24314","inReplyTo":"1278480034-22939-6-git-send-email-newren@gmail.com","subject":"Re: [PATCHv3 5/6] fast-export: Fix output order of D/F changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-07T21:40:53Z","receivedAt":"2010-07-07T21:40:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"newren@gmail.com writes:\n\n> From: Elijah Newren \n>\n> The fast-import stream format requires incremental changes which take place\n> immediately, meaning that for D->F conversions all files below the relevant\n> directory must be deleted before the resulting file of the same name is\n> created. Reversing the order can result in fast-import silently deleting\n> the file right after creating it, resulting in the file missing from the\n> resulting repository.\n>\n> We correct the order by instructing the diff_tree machinery to compare\n> entries using df_name_compare instead of base_name_compare.\n\nI am not so sure about this one.\n\nYou can be walking two trees, one of which has \"b-1\" (blob), \"b-2\" (blob)\nand then \"b\" (tree), while the other one has \"b\" (blob), \"b-1\" (blob) and\nthen \"b-2\" (blob). The patch tells the machinery that \"b\" (tree) sorts\njust like \"b\" (blob) only during comparison, but the actual data stream it\nis walking is sorted differently. Without some form of lookahead, can you\nreliably \"correct the order\"?\n"},{"id":"145081","messageId":"AANLkTinVyfWEqJqZgpiXv0hvMLELLb-UThi-0W2izGQc@mail.gmail.com","threadId":"24314","inReplyTo":"7vocejt25d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 1/6] Add additional testcases for D/F conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-07-08T03:07:53Z","receivedAt":"2010-07-08T03:07:53Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nThanks for the thorough reviews!\n\nOn Wed, Jul 7, 2010 at 3:25 PM, Junio C Hamano wrote:\n>> -test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '\n>> +test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '\n>\n> If you retitle this, you would probably want to retitle the corresponding\n> test for resolve (the one before this test) the same way.\n\n> It may be a good idea to see what resolve does for all these new\n> tests.\n\nI'd be happy to make these and the other changes you suggested, but I\nnotice that you've already done so in pu with Fixup commits for this\nand the other patches. Should I squash those in for a resend of the\nseries?\n\nAlso, do you want me to split the series as you did with the testsuite\naddition patches separate from the fixes?\n\nThanks,\nElijah\n"},{"id":"145084","messageId":"AANLkTim1thwRF5boWeuhRsKpnY3DHAHq1ixIB-5GBIYA@mail.gmail.com","threadId":"24314","inReplyTo":"7v630rt1h3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/6] merge-recursive: Fix D/F conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-07-08T04:34:50Z","receivedAt":"2010-07-08T04:34:50Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 7, 2010 at 3:40 PM, Junio C Hamano wrote:\n> Style aside, what happens when we cannot cleanly resolve D/F conflicts,\n> i.e. when some untracked/modified paths still remain after the earlier\n> removal?  We would signal conflict but at that point what happens to the\n> files in the working tree that are involved in this new codepath?  They\n> are already gone, I presume, and they at least match the original index\n> entries, so not much is lost, but what would the recovery procedure out of\n> the resulting mess?  \"reset --hard\"?\n\nThat's a really good point, and yes all those files are already gone.\nHowever, isn't this already a problem with current git? git does not\nabort when it detects the D/F conflict, it simply creates the file\nwith an alternate name such as directory/file~branch, and then\ncontinues processing all the other entries -- resulting in all those\nsame files under the directory being deleted. So I don't think this\npatch creates any new problem.\n\nIf you want to resolve such a merge by keeping the directory and the\nfiles under it, it is possible, though not at all obvious or\nstraightforward:\n$ git rm --cached file-that-used-to-be-dir\n$ git status file-that-used-to-be-dir | grep deleted | sed -e\ns/.*deleted:// | xargs git checkout $(git merge-base HEAD $(cat\n.git/MERGE_HEAD)) --\n\nI think we could fix this up, now that we have the D/F conflict\nresolution being handled last: When process_df_entry() detects that\nthere's a directory in the way of the file that needs to be created\n(and instead warns and creates it with an alternate name), it could\nalso restore those deleted entries at that time. It seems like a bit\nof unnecessary work to delete and then restore files, but I'm not sure\nhow to get around it given the basic strategy of having unpack_trees\ndo what it can and then trying to fix it up separately.\n\n\nElijah\n"},{"id":"145085","messageId":"AANLkTiklqqxUhTBd4In-VQed_SMQ6X0nk2j2ysE20bmD@mail.gmail.com","threadId":"24314","inReplyTo":"7vy6dnrmvu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 5/6] fast-export: Fix output order of D/F changes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-07-08T04:36:38Z","receivedAt":"2010-07-08T04:36:38Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 7, 2010 at 3:40 PM, Junio C Hamano wrote:\n> newren@gmail.com writes:\n>\n>> From: Elijah Newren \n>>\n>> The fast-import stream format requires incremental changes which take place\n>> immediately, meaning that for D->F conversions all files below the relevant\n>> directory must be deleted before the resulting file of the same name is\n>> created.  Reversing the order can result in fast-import silently deleting\n>> the file right after creating it, resulting in the file missing from the\n>> resulting repository.\n>>\n>> We correct the order by instructing the diff_tree machinery to compare\n>> entries using df_name_compare instead of base_name_compare.\n>\n> I am not so sure about this one.\n>\n> You can be walking two trees, one of which has \"b-1\" (blob), \"b-2\" (blob)\n> and then \"b\" (tree), while the other one has \"b\" (blob), \"b-1\" (blob) and\n> then \"b-2\" (blob).  The patch tells the machinery that \"b\" (tree) sorts\n> just like \"b\" (blob) only during comparison, but the actual data stream it\n> is walking is sorted differently.  Without some form of lookahead, can you\n> reliably \"correct the order\"?\n\nDoh! Yep, you're right. My patch would not handle that case\ncorrectly. I'll dig further.\n"},{"id":"145087","messageId":"AANLkTimLnUHMlXwLwuHsKPibw53lbi4x_LPEgmYtbW9V@mail.gmail.com","threadId":"24314","inReplyTo":"7vhbkbt24d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 2/6] Add a rename + D/F conflict testcase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-07-08T05:04:38Z","receivedAt":"2010-07-08T05:04:38Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 7, 2010 at 3:26 PM, Junio C Hamano wrote:\n>> diff --git a/t/t3508-cherry-pick-merge-df.sh b/t/t3508-cherry-pick-merge-df.sh\n>> new file mode 100755\n>> index 0000000..646a56d\n>> --- /dev/null\n>> +++ b/t/t3508-cherry-pick-merge-df.sh\n>> @@ -0,0 +1,34 @@\n>> ...\n>> +     touch f1 &&\n>> +     git add f1 &&\n>> +     git commit -m \"f1\"\n>> +'\n>> +\n>> +test_expect_failure 'Cherry-pick succeeds with rename across D/F conflicts' '\n>> +     git checkout master &&\n>> +     git cherry-pick branch\n>> +'\n>\n> This is curious.  \"branch\" only adds a totally unrelated path to its\n> parent, and picking it should be trivial.  I wonder what it does\n> here if we used the resolve strategy...\n\nIt works with the resolve strategy. I could add a testcase for it in\nthe next resend, though using the --strategy option will make the\nseries depend on master and make it no longer apply to maint. Is that\nan issue?\n"},{"id":"145089","messageId":"7voceiqzk3.fsf@alter.siamese.dyndns.org","threadId":"24314","inReplyTo":"AANLkTiklqqxUhTBd4In-VQed_SMQ6X0nk2j2ysE20bmD@mail.gmail.com","subject":"Re: [PATCHv3 5/6] fast-export: Fix output order of D/F changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-08T06:04:44Z","receivedAt":"2010-07-08T06:04:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>>> We correct the order by instructing the diff_tree machinery to compare\n>>> entries using df_name_compare instead of base_name_compare.\n>\n> Doh! Yep, you're right. My patch would not handle that case\n> correctly.\n\nIf all you want is to force a particular order of paths in the output\n(e.g. depth first) in this one single application, the cleanest way might\nbe to let the diffcore do its work and at the very end sort the elements\nin the diff_queued_diff to your liking (c.f. diffcore_fix_diff_index()\nthat uses diffnamecmp() to sort the list).\n"},{"id":"145135","messageId":"7vaaq2q7u5.fsf@alter.siamese.dyndns.org","threadId":"24314","inReplyTo":"AANLkTinVyfWEqJqZgpiXv0hvMLELLb-UThi-0W2izGQc@mail.gmail.com","subject":"Re: [PATCHv3 1/6] Add additional testcases for D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-08T16:03:30Z","receivedAt":"2010-07-08T16:03:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I'd be happy to make these and the other changes you suggested, but I\n> notice that you've already done so in pu with Fixup commits for this\n> and the other patches. Should I squash those in for a resend of the\n> series?\n\nYes, please; patches being in 'pu' does not have much more weight than\nthem being in the mailing list archive. The patches should have warts\nthese separate \"fixup\" patches had to correct removed before they advance.\n\nThanks.\n"}]}