{"thread":{"id":"41979","subject":"[PATCH 1/6] Remove duplicate code","startedAt":"2016-04-10T06:13:34Z","lastAt":"2016-04-15T21:14:12Z","messageCount":10,"participants":["Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"283066","messageId":"1460268820-8308-1-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":null,"subject":"[PATCH 0/6] Miscellaneous merge fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:34Z","receivedAt":"2016-04-10T06:13:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series has four independent miscellaneous fixes, three split out\nfrom my RFC series at $gmane/291007 and one new fix.  These four fixes\nhave no dependencies on each other, so I can trivially split them into\nfour submissions if preferred.  I only batched them because I want my\nindex-only merge series to be able to simultaneously depend on three\nof them.\n\nThe first patch is a very minor code cleanup, which has already been\nreviewed by Junio.\n\nThe second patch is also a minor code cleanup.\n\nThe third and fourth patches test and fix a bug with octopus merges.\nSince last submitting these patches with my RFC series, I discovered\nour documentation used to explicitly state the expectation I enforced\nwith these patches.  I added a note about that in the commit message.\n\nThe fifth and sixth patches test and fix a bug with trivial merges;\nthese patches are new since my RFC series.\n\n\nElijah Newren (6):\n  Remove duplicate code\n  Avoid checking working copy when creating a virtual merge base\n  Add merge testcases for when index doesn't match HEAD\n  merge-octopus: Abort if index does not match HEAD\n  Add a testcase demonstrating a bug with trivial merges\n  builtin/merge.c: Fix a bug with trivial merges\n\n builtin/merge.c                          |   8 ++\n git-merge-octopus.sh                     |   6 ++\n merge-recursive.c                        |   8 +-\n t/t6044-merge-unrelated-index-changes.sh | 153 +++++++++++++++++++++++++++++++\n t/t7605-merge-resolve.sh                 |   6 +-\n 5 files changed, 175 insertions(+), 6 deletions(-)\n create mode 100755 t/t6044-merge-unrelated-index-changes.sh\n\n-- \n2.8.0.21.g229f62a\n"},{"id":"283063","messageId":"1460268820-8308-2-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"[PATCH 1/6] Remove duplicate code","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:35Z","receivedAt":"2016-04-10T06:13:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 51931bf (merge-recursive: Improve handling of rename target vs.\ndirectory addition, 2011-08-11) I apparently added two lines of code that\nwere immediately duplicated a few lines later.  No idea why, other than\nit seems pretty clear this was a mistake: there is no need to remove the\nsame file twice; removing it once is sufficient...especially since the\nintervening line was working with a different file entirely.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\nReviewed-by: Junio C Hamano <gitster@pobox.com>\n---\n merge-recursive.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b880ae5..d4292de 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1773,8 +1773,6 @@ static int process_entry(struct merge_options *o,\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\tif (o->call_depth)\n-\t\t\t\tremove_file_from_cache(path);\n \t\t\tupdate_file(o, 0, sha, mode, new_path);\n \t\t\tif (o->call_depth)\n \t\t\t\tremove_file_from_cache(path);\n-- \n2.8.0.21.g229f62a\n"},{"id":"283069","messageId":"1460268820-8308-3-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"[PATCH 2/6] Avoid checking working copy when creating a virtual merge base","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:36Z","receivedAt":"2016-04-10T06:13:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"There were a few cases in merge-recursive that could result in a check for\nthe presence of files in the working copy while trying to create a virtual\nmerge base.  These were rare and innocuous, but somewhat illogical.  The\ntwo cases were:\n\n  * When there was naming conflicts (e.g. a D/F conflict) and we had to\n    pick a new unique name for a file.  Since the new name is somewhat\n    arbitrary, it didn't matter that we consulted the working copy to\n    avoid picking a filename it has, but since the virtual merge base is\n    never checked out, it's a waste of time and slightly odd to do so.\n\n  * When two different files get renamed to the same name (on opposite\n    sides of the merge), we needed to delete the original filenames from\n    the cache and possibly also the working directory.  The caller's check\n    for determining whether to delete from the working directory was a\n    call to would_lose_untracked().  It turns out this didn't matter\n    because remove_file() had logic to avoid modifying the working\n    directory when creating a virtual merge base, but there is no reason\n    for the caller to check the working directory in such circumstances.\n    It's a waste of time, if not also a bit weird.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d4292de..06d31ed 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -622,7 +622,7 @@ static char *unique_path(struct merge_options *o, const char *path, const char *\n \tbase_len = newpath.len;\n \twhile (string_list_has_string(&o->current_file_set, newpath.buf) ||\n \t       string_list_has_string(&o->current_directory_set, newpath.buf) ||\n-\t       file_exists(newpath.buf)) {\n+\t       (!o->call_depth && file_exists(newpath.buf))) {\n \t\tstrbuf_setlen(&newpath, base_len);\n \t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n \t}\n@@ -1234,8 +1234,8 @@ static void conflict_rename_rename_2to1(struct merge_options *o,\n \t       a->path, c1->path, ci->branch1,\n \t       b->path, c2->path, ci->branch2);\n \n-\tremove_file(o, 1, a->path, would_lose_untracked(a->path));\n-\tremove_file(o, 1, b->path, would_lose_untracked(b->path));\n+\tremove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));\n+\tremove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));\n \n \tmfi_c1 = merge_file_special_markers(o, a, c1, &ci->ren1_other,\n \t\t\t\t\t    o->branch1, c1->path,\n-- \n2.8.0.21.g229f62a\n"},{"id":"283067","messageId":"1460268820-8308-4-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"[PATCH 3/6] Add merge testcases for when index doesn't match HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:37Z","receivedAt":"2016-04-10T06:13:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"With one exception, we require the index to exactly match the current HEAD\ncommit at the time git merge is invoked.  This expectation was even\ndocumented in git-merge.txt until commit ebef7e5 (Documentation: simplify\nHow Merge Works, 2010-01-23).  Most merge strategies enforced this\nrequirement, but it turns out not all did.  The current exceptions were\nthe following two:\n  * ff updates\n  * octopus merges\n\nff updates actually will error out if the staged change is to a path\nmodified between HEAD and the commit being merged.  If the path(s) that\nare staged are files unrelated to the changes between these two commits,\nthough, then an ff update will just keep these staged changes around after\nthe merge.  This is the one exception we expected to the abort-merge-if-\nindex-doesn't-match-HEAD rule.\n\nFor octopus merges, the rule should be enforced.  Unfortunately, the\ncurrent behavior of the code is to ignore the difference and use the\nstaged changes in place of whatever is in HEAD as it proceeds to perform\nthe merge.  So if the staged changes can be cleanly merged with all the\nother heads, then the staged changes will just be incorported into the\nresulting commit.  If the staged changes cannot be cleanly merged with all\nthe other heads, the merge is not aborted -- merge conflicts are simply\nreported as if HEAD had originally contained whatever the index did.\n\nAdd testcases that check our expectations.  A subsequent commit will\ncorrect the erroneous octopus merge behavior.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6044-merge-unrelated-index-changes.sh | 153 +++++++++++++++++++++++++++++++\n 1 file changed, 153 insertions(+)\n create mode 100755 t/t6044-merge-unrelated-index-changes.sh\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nnew file mode 100755\nindex 0000000..eed5d95\n--- /dev/null\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -0,0 +1,153 @@\n+#!/bin/sh\n+\n+test_description=\"merges with unrelated index changes\"\n+\n+. ./test-lib.sh\n+\n+# Testcase for some simple merges\n+#   A\n+#   o-----o B\n+#    \\\n+#     \\---o C\n+#      \\\n+#       \\-o D\n+#        \\\n+#         o E\n+#   Commit A: some file a\n+#   Commit B: adds file b, modifies end of a\n+#   Commit C: adds file c\n+#   Commit D: adds file d, modifies beginning of a\n+#   Commit E: renames a->subdir/a, adds subdir/e\n+\n+test_expect_success 'setup trivial merges' '\n+\tseq 1 10 >a &&\n+\tgit add a &&\n+\ttest_tick && git commit -m A &&\n+\n+\tgit branch A &&\n+\tgit branch B &&\n+\tgit branch C &&\n+\tgit branch D &&\n+\tgit branch E &&\n+\n+\tgit checkout B &&\n+\techo b >b &&\n+\techo 11 >>a &&\n+\tgit add a b &&\n+\ttest_tick && git commit -m B &&\n+\n+\tgit checkout C &&\n+\techo c >c &&\n+\tgit add c &&\n+\ttest_tick && git commit -m C &&\n+\n+\tgit checkout D &&\n+\tseq 2 10 >a &&\n+\techo d >d &&\n+\tgit add a d &&\n+\ttest_tick && git commit -m D &&\n+\n+\tgit checkout E &&\n+\tmkdir subdir &&\n+\tgit mv a subdir/a &&\n+\techo e >subdir/e &&\n+\tgit add subdir &&\n+\ttest_tick && git commit -m E\n+'\n+\n+test_expect_success 'ff update' '\n+\tgit reset --hard &&\n+\tgit checkout A^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\tgit merge E^0 &&\n+\n+\ttest_must_fail git rev-parse HEAD:random_file &&\n+\ttest \"$(git diff --name-only --cached E)\" = \"random_file\"\n+'\n+\n+test_expect_success 'ff update, important file modified' '\n+\tgit reset --hard &&\n+\tgit checkout A^0 &&\n+\n+\tmkdir subdir &&\n+\ttouch subdir/e &&\n+\tgit add subdir/e &&\n+\n+\ttest_must_fail git merge E^0\n+'\n+\n+test_expect_success 'resolve, trivial' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge -s resolve C^0\n+'\n+\n+test_expect_success 'resolve, non-trivial' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge -s resolve D^0\n+'\n+\n+test_expect_success 'recursive' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge -s recursive C^0\n+'\n+\n+test_expect_failure 'octopus, unrelated file touched' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge C^0 D^0\n+'\n+\n+test_expect_failure 'octopus, related file removed' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\tgit rm b &&\n+\n+\ttest_must_fail git merge C^0 D^0\n+'\n+\n+test_expect_failure 'octopus, related file modified' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\techo 12 >>a && git add a &&\n+\n+\ttest_must_fail git merge C^0 D^0\n+'\n+\n+test_expect_success 'ours' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge -s ours C^0\n+'\n+\n+test_expect_success 'subtree' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge -s subtree E^0\n+'\n+\n+test_done\n-- \n2.8.0.21.g229f62a\n"},{"id":"283064","messageId":"1460268820-8308-5-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"[PATCH 4/6] merge-octopus: Abort if index does not match HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:38Z","receivedAt":"2016-04-10T06:13:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren <newren@gmail.com>\n---\n git-merge-octopus.sh                     | 6 ++++++\n t/t6044-merge-unrelated-index-changes.sh | 6 +++---\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/git-merge-octopus.sh b/git-merge-octopus.sh\nindex 8643f74..dc2fd1b 100755\n--- a/git-merge-octopus.sh\n+++ b/git-merge-octopus.sh\n@@ -44,6 +44,12 @@ esac\n # MRC is the current \"merge reference commit\"\n # MRT is the current \"merge result tree\"\n \n+if ! git diff-index --quiet --cached HEAD --\n+then\n+    echo \"Error: Your local changes to the following files would be overwritten by merge\"\n+    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'\n+    exit 2\n+fi\n MRC=$(git rev-parse --verify -q $head)\n MRT=$(git write-tree)\n NON_FF_MERGE=0\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex eed5d95..20a3ffe 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -105,7 +105,7 @@ test_expect_success 'recursive' '\n \ttest_must_fail git merge -s recursive C^0\n '\n \n-test_expect_failure 'octopus, unrelated file touched' '\n+test_expect_success 'octopus, unrelated file touched' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n@@ -114,7 +114,7 @@ test_expect_failure 'octopus, unrelated file touched' '\n \ttest_must_fail git merge C^0 D^0\n '\n \n-test_expect_failure 'octopus, related file removed' '\n+test_expect_success 'octopus, related file removed' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n@@ -123,7 +123,7 @@ test_expect_failure 'octopus, related file removed' '\n \ttest_must_fail git merge C^0 D^0\n '\n \n-test_expect_failure 'octopus, related file modified' '\n+test_expect_success 'octopus, related file modified' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n-- \n2.8.0.21.g229f62a\n"},{"id":"283068","messageId":"1460268820-8308-6-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"[PATCH 5/6] Add a testcase demonstrating a bug with trivial merges","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:39Z","receivedAt":"2016-04-10T06:13:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Repeating a trivially merge more than once will leave the index out of\nsync, despite being clean before the merge and operating on the exact\nsame heads as the first run.  The recorded merge has the correct tree and\nthe working tree is brought up to date, it is just the index that is left\nas it was before the merge.  Every attempt to repeat the merge beyond the\nfirst will leave the index in the same weird out-of-sync state.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7605-merge-resolve.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh\nindex 0cb9d11..2f80037 100755\n--- a/t/t7605-merge-resolve.sh\n+++ b/t/t7605-merge-resolve.sh\n@@ -27,7 +27,7 @@ test_expect_success 'setup' '\n \tgit tag c3\n '\n \n-test_expect_success 'merge c1 to c2' '\n+merge_c1_to_c2_cmds='\n \tgit reset --hard c1 &&\n \tgit merge -s resolve c2 &&\n \ttest \"$(git rev-parse c1)\" != \"$(git rev-parse HEAD)\" &&\n@@ -41,6 +41,10 @@ test_expect_success 'merge c1 to c2' '\n \ttest 3 = $(git ls-files | wc -l)\n '\n \n+test_expect_success 'merge c1 to c2'        \"$merge_c1_to_c2_cmds\"\n+\n+test_expect_failure 'merge c1 to c2, again' \"$merge_c1_to_c2_cmds\"\n+\n test_expect_success 'merge c2 to c3 (fails)' '\n \tgit reset --hard c2 &&\n \ttest_must_fail git merge -s resolve c3\n-- \n2.8.0.21.g229f62a\n"},{"id":"283065","messageId":"1460268820-8308-7-git-send-email-newren@gmail.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"[PATCH 6/6] builtin/merge.c: Fix a bug with trivial merges","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-10T06:13:40Z","receivedAt":"2016-04-10T06:13:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"If read_tree_trivial succeeds and produces a tree that is already in the\nobject store, then the index is not written to disk, leaving it\nout-of-sync with both HEAD and the working tree.\n\nIn order to write the index back out to disk after a merge,\nwrite_index_locked() needs to be called.  For most merge strategies, this\nis done from try_merge_strategy().  For fast forward updates, this is\ndone from checkout_fast_forward().  When trivial merges work, the call to\nwrite_index_locked() is buried a little deeper:\n  merge_trivial()\n  -> write_tree_trivial()\n     -> write_cache_as_tree()\n        -> write_index_as_tree()\n           -> write_locked_index()\nHowever, it is only called when !cache_tree_fully_valid(), which is how\nthis bug is triggered.  But that also shows why this bug doesn't affect\nany other merge strategies or cases.\n\nAdd a direct call to write_index_locked() from merge_trivial() to fix\nthis issue.  Since the indirect call to write_locked_index() was\nconditional on cache_tree_fully_valid(), it won't be written twice.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/merge.c          | 8 ++++++++\n t/t7605-merge-resolve.sh | 2 +-\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 101ffef..8615343 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -819,6 +819,14 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tunsigned char result_tree[20], result_commit[20];\n \tstruct commit_list *parents, **pptr = &parents;\n+\tstatic struct lock_file lock;\n+\n+\thold_locked_index(&lock, 1);\n+\trefresh_cache(REFRESH_QUIET);\n+\tif (active_cache_changed &&\n+\t    write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\t\treturn error(_(\"Unable to write index.\"));\n+\trollback_lock_file(&lock);\n \n \twrite_tree_trivial(result_tree);\n \tprintf(_(\"Wonderful.\\n\"));\ndiff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh\nindex 2f80037..5d56c38 100755\n--- a/t/t7605-merge-resolve.sh\n+++ b/t/t7605-merge-resolve.sh\n@@ -43,7 +43,7 @@ merge_c1_to_c2_cmds='\n \n test_expect_success 'merge c1 to c2'        \"$merge_c1_to_c2_cmds\"\n \n-test_expect_failure 'merge c1 to c2, again' \"$merge_c1_to_c2_cmds\"\n+test_expect_success 'merge c1 to c2, again' \"$merge_c1_to_c2_cmds\"\n \n test_expect_success 'merge c2 to c3 (fails)' '\n \tgit reset --hard c2 &&\n-- \n2.8.0.21.g229f62a\n"},{"id":"283279","messageId":"xmqqd1pumhdr.fsf@gitster.mtv.corp.google.com","threadId":"41979","inReplyTo":"1460268820-8308-1-git-send-email-newren@gmail.com","subject":"Re: [PATCH 0/6] Miscellaneous merge fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-13T01:23:12Z","receivedAt":"2016-04-13T01:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Elijah Newren (6):\n>   Remove duplicate code\n>   Avoid checking working copy when creating a virtual merge base\n>   Add merge testcases for when index doesn't match HEAD\n>   merge-octopus: Abort if index does not match HEAD\n>   Add a testcase demonstrating a bug with trivial merges\n>   builtin/merge.c: Fix a bug with trivial merges\n\nPlease be careful when giving titles to your patches.  They will be\nshown in a context that is much wider than the area your attention\nis currently concentrated on.  E.g. does \"Remove duplicate code\"\ntell readers of \"git shortlog --no-merges v2.8.0..v2.9.0\" what the\nchange was about when it eventually appears in the upcoming release?\n\n>\n>  builtin/merge.c                          |   8 ++\n>  git-merge-octopus.sh                     |   6 ++\n>  merge-recursive.c                        |   8 +-\n>  t/t6044-merge-unrelated-index-changes.sh | 153 +++++++++++++++++++++++++++++++\n>  t/t7605-merge-resolve.sh                 |   6 +-\n>  5 files changed, 175 insertions(+), 6 deletions(-)\n>  create mode 100755 t/t6044-merge-unrelated-index-changes.sh\n"},{"id":"283281","messageId":"xmqq8u0imh4y.fsf@gitster.mtv.corp.google.com","threadId":"41979","inReplyTo":"1460268820-8308-3-git-send-email-newren@gmail.com","subject":"Re: [PATCH 2/6] Avoid checking working copy when creating a virtual merge base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-13T01:28:29Z","receivedAt":"2016-04-13T01:28:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> There were a few cases in merge-recursive that could result in a check for\n> the presence of files in the working copy while trying to create a virtual\n> merge base.  These were rare and innocuous, but somewhat illogical.  The\n> two cases were:\n>\n>   * When there was naming conflicts (e.g. a D/F conflict) and we had to\n>     pick a new unique name for a file.  Since the new name is somewhat\n>     arbitrary, it didn't matter that we consulted the working copy to\n>     avoid picking a filename it has, but since the virtual merge base is\n>     never checked out, it's a waste of time and slightly odd to do so.\n>\n>   * When two different files get renamed to the same name (on opposite\n>     sides of the merge), we needed to delete the original filenames from\n>     the cache and possibly also the working directory.  The caller's check\n>     for determining whether to delete from the working directory was a\n>     call to would_lose_untracked().  It turns out this didn't matter\n>     because remove_file() had logic to avoid modifying the working\n>     directory when creating a virtual merge base, but there is no reason\n>     for the caller to check the working directory in such circumstances.\n>     It's a waste of time, if not also a bit weird.\n\nI think \"avoid checking\" and \"waste of time\" are understatements, in\nthat they make it sound as if the current code is OK and this change\nis only to reduce waste.  But doesn't the code misbehave if you had\na file in the working tree whose path happens to be the same as the\none involved in these codepaths (iow, if they did not check these\npaths in the working tree, the internal merge may succeed, but it\nwould unnecessarily fail)?\n\nSubject: merge-recursive: do not check working tree during an internal merge\n\nor something, perhaps?\n\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  merge-recursive.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d4292de..06d31ed 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -622,7 +622,7 @@ static char *unique_path(struct merge_options *o, const char *path, const char *\n>  \tbase_len = newpath.len;\n>  \twhile (string_list_has_string(&o->current_file_set, newpath.buf) ||\n>  \t       string_list_has_string(&o->current_directory_set, newpath.buf) ||\n> -\t       file_exists(newpath.buf)) {\n> +\t       (!o->call_depth && file_exists(newpath.buf))) {\n>  \t\tstrbuf_setlen(&newpath, base_len);\n>  \t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n>  \t}\n> @@ -1234,8 +1234,8 @@ static void conflict_rename_rename_2to1(struct merge_options *o,\n>  \t       a->path, c1->path, ci->branch1,\n>  \t       b->path, c2->path, ci->branch2);\n>  \n> -\tremove_file(o, 1, a->path, would_lose_untracked(a->path));\n> -\tremove_file(o, 1, b->path, would_lose_untracked(b->path));\n> +\tremove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));\n> +\tremove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));\n>  \n>  \tmfi_c1 = merge_file_special_markers(o, a, c1, &ci->ren1_other,\n>  \t\t\t\t\t    o->branch1, c1->path,\n"},{"id":"283586","messageId":"CABPp-BEui_OVUES28TmUnWH3aGTJoMsgbAX2APVKNhx0eBAGJQ@mail.gmail.com","threadId":"41979","inReplyTo":"xmqqd1pumhdr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] Miscellaneous merge fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2016-04-15T21:14:12Z","receivedAt":"2016-04-15T21:14:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Apr 12, 2016 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> Elijah Newren (6):\n>>   Remove duplicate code\n>>   Avoid checking working copy when creating a virtual merge base\n>>   Add merge testcases for when index doesn't match HEAD\n>>   merge-octopus: Abort if index does not match HEAD\n>>   Add a testcase demonstrating a bug with trivial merges\n>>   builtin/merge.c: Fix a bug with trivial merges\n>\n> Please be careful when giving titles to your patches.  They will be\n> shown in a context that is much wider than the area your attention\n> is currently concentrated on.  E.g. does \"Remove duplicate code\"\n> tell readers of \"git shortlog --no-merges v2.8.0..v2.9.0\" what the\n> change was about when it eventually appears in the upcoming release?\n\nI will try to do better on that.  For the patch you mention, how about:\n   merge-recursive: Remove a few lines of duplicate code\n?\n\nI can resubmit the series correcting both that subject and the subject\nof the second patch using your suggestion elsewhere in this thread.\nThe other four already provide some context on their area; am I\ncorrect to assume those provide enough?\n\nElijah\n"}]}