{"thread":{"id":"48632","subject":"[RFC PATCH 3/7] merge-recursive: make sure when we say we abort that we actually abort","startedAt":"2018-06-03T06:58:20Z","lastAt":"2018-07-11T03:11:54Z","messageCount":30,"participants":["Elijah Newren","Ramsay Jones","brian m. carlson","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"349109","messageId":"20180603065810.23841-4-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 3/7] merge-recursive: make sure when we say we abort that we actually abort","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:06Z","receivedAt":"2018-06-03T06:58:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 65170c07d4 (\"merge-recursive: avoid incorporating uncommitted\nchanges in a merge\", 2017-12-21), it was noted that there was a special\ncase when merge-recursive didn't rely on unpack_trees() to enforce the\nindex == HEAD requirement, and thus that it needed to do that enforcement\nitself. Unfortunately, it returned the wrong exit status, signalling that\nthe merge completed but had conflicts, rather than that it was aborted.\nFix the return code, and while we're at it, change the error message to\nmatch what unpack_trees() would have printed.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 4 ++--\n t/t6044-merge-unrelated-index-changes.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex ac27abbd4c..b3deb7b182 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3264,9 +3264,9 @@ int merge_trees(struct merge_options *o,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tif (!o->call_depth && index_has_changes(&sb)) {\n-\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n+\t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n \t\t\t sb.buf);\n-\t\t\treturn 0;\n+\t\t\treturn -1;\n \t\t}\n \t\toutput(o, 0, _(\"Already up to date!\"));\n \t\t*result = head;\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 92ec552558..3876cfa4fa 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -116,7 +116,7 @@ test_expect_success 'recursive' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n-test_expect_failure 'recursive, when merge branch matches merge base' '\n+test_expect_success 'recursive, when merge branch matches merge base' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349110","messageId":"20180603065810.23841-3-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 2/7] t6044: add a testcase for index matching head, when head doesn't match HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:05Z","receivedAt":"2018-06-03T06:58:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The `git merge-recursive` command allows the user to directly specify\nthree commits to merge -- base, head, and remote. (More than three can be\nspecified in the case of multiple merge bases.) Note that since the user\nis allowed to specify head, it need not match HEAD.\n\nVirtually every test and script in the current git.git codebase calls `git\nmerge-recursive` with head=HEAD, and likely external callers do as well,\nwhich is why this has gone unnoticed. There is one notable\ncounter-example: git-stash.sh. However, git-stash called `git\nmerge-recursive` with an index that matches the expected merge result,\nwhich happens to be a currently allowed exception to the \"index must match\nhead\" rule, so this never triggered an error previously.\n\nSince we would like to tighten up the \"index must match head\" rule, we\nneed to make sure we are comparing to the correct head. Add a testcase\nthat demonstrates the failure when we check the wrong HEAD.\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex f9c2f8179e..92ec552558 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n+test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n+\tgit reset --hard &&\n+\tgit checkout C^0 &&\n+\n+\t# Make index match B\n+\tgit diff C B | git apply --cached &&\n+\t# Merge B & F, with B as \"head\"\n+\tgit merge-recursive A -- B F > out &&\n+\ttest_i18ngrep \"Already up to date\" out\n+'\n+\n test_expect_success 'octopus, unrelated file touched' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349111","messageId":"20180603065810.23841-6-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 5/7] t6044: add more testcases with staged changes before a merge is invoked","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:08Z","receivedAt":"2018-06-03T06:58:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"According to Documentation/git-merge.txt,\n\n ...[merge will] abort if there are any changes registered in the index\n relative to the `HEAD` commit. (One exception is when the changed index\n entries are in the state that would result from the merge already.)\n\nAdd some tests showing that this exception, while it does accurately state\nwhat would be a safe condition under which we could allow the merge to\nproceed, is not what is actually implemented.\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 29 ++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 1d43712c52..e609f14f87 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -137,6 +137,35 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \ttest_i18ngrep \"Already up to date\" out\n '\n \n+test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\tmkdir subdir &&\n+\ttest_seq 1 10 >subdir/a &&\n+\tgit add subdir/a &&\n+\n+\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n+\t# Since subdir/a matches neither HEAD nor what the merge would write\n+\t# to that file, the merge should fail to avoid overwriting what is\n+\t# currently found in subdir/a\n+\ttest_must_fail git merge -s recursive E^0\n+'\n+\n+test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\tmkdir subdir &&\n+\ttest_seq 1 11 >subdir/a &&\n+\tgit add subdir/a &&\n+\n+\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n+\t# Since subdir/a matches what the merge would write to that file,\n+\t# the merge should be safe to proceed\n+\tgit merge -s recursive E^0\n+'\n+\n test_expect_success 'octopus, unrelated file touched' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349112","messageId":"20180603065810.23841-1-newren@gmail.com","threadId":"48632","inReplyTo":null,"subject":"[RFC PATCH 0/7] merge requirement: index matches head","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:03Z","receivedAt":"2018-06-03T06:58:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Between working on some other things, I happened to be reading\nDocumentation/git-merge.txt and ran across the part that says:\n\n ...[merge will] abort if there are any changes registered in the\n index relative to the `HEAD` commit. (One exception is when the\n changed index entries are in the state that would result from the\n merge already.)\n\nI was pretty sure this statement was wrong, but did some digging to\nuncover the details and the history. What I thought would turn into a\nsimple three-line documentation fix, ballooned into this patch series.\n\nThis series might be best read in a different order; I'm not yet sure\nthe right way to structure it. But:\n\n * Patch 5 demonstrates one of the ways that the parenthetical\n sentence is wrong (desirable perhaps, but not what is implemented)\n\n * Patch 7 explains the history, the trade-offs, the three ways the\n parenthetical sentence is wrong, and the many pitfalls we've run\n into trying to allow for such an exception. Very small\n documentation fix with a huge commit message.\n\n * Patch 6 fixes the breakage demonstrated in patch 5, but if I only\n submitted patches 5-7, then the testsuite wouldn't pass because\n this fix uncovered multiple other bugs. That's where patches 1-4\n came in. This fix is also kind of opinionated; it takes the stance\n that allowing the exceptions isn't worth it.\n\nElijah Newren (7):\n t6044: verify that merges expected to abort actually abort\n t6044: add a testcase for index matching head, when head doesn't match HEAD\n merge-recursive: make sure when we say we abort that we actually abort\n merge-recursive: fix assumption that head tree being merged is HEAD\n t6044: add more testcases with staged changes before a merge is invoked\n merge-recursive: enforce rule that index matches head before merging\n merge: fix misleading pre-merge check documentation\n\n Documentation/git-merge.txt | 6 +-\n builtin/am.c | 6 +-\n cache.h | 8 --\n merge-recursive.c | 14 +--\n merge.c | 10 +-\n t/t6044-merge-unrelated-index-changes.sh | 67 +++++++++++--\n t/t7504-commit-msg-hook.sh | 4 +-\n t/t7611-merge-abort.sh | 118 -----------------------\n tree.h | 8 ++\n 9 files changed, 87 insertions(+), 154 deletions(-)\n\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349113","messageId":"20180603065810.23841-7-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 6/7] merge-recursive: enforce rule that index matches head before merging","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:09Z","receivedAt":"2018-06-03T06:58:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"builtin/merge.c says that when we are about to perform a merge:\n\n ...the index must be in sync with the head commit. The strategies are\n responsible to ensure this.\n\nmerge-recursive has always relied on unpack_trees() to enforce this\nrequirement, except in the case of an \"Already up to date!\" merge.\nunpack-trees.c does not actually enforce this requirement, though. It\nallows for a pair of exceptions, in cases which it refers to as #14(ALT)\nand #2ALT. Documentation/technical/trivial-merge.txt can be consulted for\nthe precise meanings of the various case numbers and their meanings for\nunpack-trees.c, but we have a high-level description of the intent behind\nthese two exceptions in a combined and summarized form in\nDocumentation/git-merge.txt:\n\n ...[merge will] abort if there are any changes registered in the index\n relative to the `HEAD` commit. (One exception is when the changed index\n entries are in the state that would result from the merge already.)\n\nWhile this high-level description does describe conditions under which it\nwould be safe to allow the index to diverge from HEAD, it does not match\nwhat is actually implemented. In particular, unpack-trees.c has no\nknowledge of renames, and these two exceptions were written assuming that\nno renames take place. Once renames get into the mix, it is no longer\nsafe to allow the index to not match for #2ALT. We could modify\nunpack-trees to only allow #14(ALT) as an exception, but that would be\nmore strict than required for the resolve strategy (since the resolve\nstrategy doesn't handle renames at all). Therefore, unpack_trees.c seems\nlike the wrong place to fix this.\n\nFurther, if someone fixes the combination of break and rename detection\nand modifies merge-recursive to take advantage of the combination, then it\nwill also no longer be safe to allow the index to not match for #14(ALT)\nwhen the recursive strategy is in use. Therefore, leaving one of the\nexceptions in place with the recursive merge strategy feels like we are\njust leaving a latent bug in the code for folks in the future to stumble\nacross.\n\nIt may be possible to fix both unpack-trees and merge-recursive in a way\nthat implements the exception as stated in Documentation/git-merge.txt,\nbut it would be somewhat complex, possibly also buggy at first, and\nultimately, not all that valuable. Instead, just enforce the requirement\nstated in builtin/merge.c; error out if the index does not match the HEAD\ncommit, just like the 'ours' and 'octopus' strategies do.\n\nSome testcase fixups were in order:\n t6044: We no longer expect stray staged changes to sometimes result\n in the merge continuing. Also, fixes a case where a merge\n didn't abort but should have.\n t7504: had a few tests that had stray staged changes that were not\n actually part of the test under consideration\n t7611: had many tests designed to show that `git merge --abort` could\n\t not always restore the index and working tree to the state they\n\t were in before the merge started. The tests that were associated\n\t with having changes in the index before the merge started are no\n longer applicable, so they have been removed.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 14 +--\n t/t6044-merge-unrelated-index-changes.sh | 19 ++--\n t/t7504-commit-msg-hook.sh | 4 +-\n t/t7611-merge-abort.sh | 118 -----------------------\n 4 files changed, 18 insertions(+), 137 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 762aa087d0..4640b47a19 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3254,6 +3254,13 @@ int merge_trees(struct merge_options *o,\n \t\tstruct tree **result)\n {\n \tint code, clean;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!o->call_depth && index_has_changes(&sb, head)) {\n+\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n+\t\t sb.buf);\n+\t\treturn -1;\n+\t}\n \n \tif (o->subtree_shift) {\n \t\tmerge = shift_tree_object(head, merge, o->subtree_shift);\n@@ -3261,13 +3268,6 @@ int merge_trees(struct merge_options *o,\n \t}\n \n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n-\t\tif (!o->call_depth && index_has_changes(&sb, head)) {\n-\t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n-\t\t\t sb.buf);\n-\t\t\treturn -1;\n-\t\t}\n \t\toutput(o, 0, _(\"Already up to date!\"));\n \t\t*result = head;\n \t\treturn 1;\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex e609f14f87..6e0ecab9c0 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -137,7 +137,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \ttest_i18ngrep \"Already up to date\" out\n '\n \n-test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '\n+test_expect_success 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n@@ -145,14 +145,12 @@ test_expect_failure 'recursive, when file has staged changes not matching HEAD n\n \ttest_seq 1 10 >subdir/a &&\n \tgit add subdir/a &&\n \n-\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n-\t# Since subdir/a matches neither HEAD nor what the merge would write\n-\t# to that file, the merge should fail to avoid overwriting what is\n-\t# currently found in subdir/a\n-\ttest_must_fail git merge -s recursive E^0\n+\t# We have staged changes; merge should error out\n+\ttest_must_fail git merge -s recursive E^0 2>err &&\n+\ttest_i18ngrep \"changes to the following files would be overwritten\" err\n '\n \n-test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '\n+test_expect_success 'recursive, when file has staged changes matching what a merge would give' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n@@ -160,10 +158,9 @@ test_expect_failure 'recursive, when file has staged changes matching what a mer\n \ttest_seq 1 11 >subdir/a &&\n \tgit add subdir/a &&\n \n-\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n-\t# Since subdir/a matches what the merge would write to that file,\n-\t# the merge should be safe to proceed\n-\tgit merge -s recursive E^0\n+\t# We have staged changes; merge should error out\n+\ttest_must_fail git merge -s recursive E^0 2>err &&\n+\ttest_i18ngrep \"changes to the following files would be overwritten\" err\n '\n \n test_expect_success 'octopus, unrelated file touched' '\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 302a3a2082..31b9c6a2c1 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -157,6 +157,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '\n \ttest_when_finished \"git branch -D newbranch\" &&\n \ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout --orphan newbranch &&\n+\tgit rm -f file &&\n \t: >file2 &&\n \tgit add file2 &&\n \tgit commit --no-verify file2 -m in-side-branch &&\n@@ -168,7 +169,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '\n chmod -x \"$HOOK\"\n test_expect_success POSIXPERM 'with non-executable hook' '\n \n-\techo \"content\" >> file &&\n+\techo \"content\" >file &&\n \tgit add file &&\n \tgit commit -m \"content\"\n \n@@ -249,6 +250,7 @@ test_expect_success 'hook called in git-merge picks up commit message' '\n \ttest_when_finished \"git branch -D newbranch\" &&\n \ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout --orphan newbranch &&\n+\tgit rm -f file &&\n \t: >file2 &&\n \tgit add file2 &&\n \tgit commit --no-verify file2 -m in-side-branch &&\ndiff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh\nindex 7b4798e8e4..7c84a518aa 100755\n--- a/t/t7611-merge-abort.sh\n+++ b/t/t7611-merge-abort.sh\n@@ -187,31 +187,6 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'Abort clean merge with matching dirty index' '\n-\tgit add bar &&\n-\tgit diff --staged > expect &&\n-\tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t### When aborting the merge, git will discard all staged changes,\n-\t### including those that were staged pre-merge. In other words,\n-\t### --abort will LOSE any staged changes (the staged changes that\n-\t### are lost must match the merge result, or the merge would not\n-\t### have been allowed to start). Change expectations accordingly:\n-\trm expect &&\n-\ttouch expect &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff --staged > actual &&\n-\ttest_cmp expect actual &&\n-\ttest -z \"$(git diff)\"\n-'\n-\n-test_expect_success 'Reset worktree changes' '\n-\tgit reset --hard \"$pre_merge_head\"\n-'\n-\n test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \techo barf > bar &&\n \tgit diff > expect &&\n@@ -223,97 +198,4 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'Abort conflicting merge with matching dirty index' '\n-\tgit add bar &&\n-\tgit diff --staged > expect &&\n-\ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t### When aborting the merge, git will discard all staged changes,\n-\t### including those that were staged pre-merge. In other words,\n-\t### --abort will LOSE any staged changes (the staged changes that\n-\t### are lost must match the merge result, or the merge would not\n-\t### have been allowed to start). Change expectations accordingly:\n-\trm expect &&\n-\ttouch expect &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff --staged > actual &&\n-\ttest_cmp expect actual &&\n-\ttest -z \"$(git diff)\"\n-'\n-\n-test_expect_success 'Reset worktree changes' '\n-\tgit reset --hard \"$pre_merge_head\"\n-'\n-\n-test_expect_success 'Abort merge with pre- and post-merge worktree changes' '\n-\t# Pre-merge worktree changes\n-\techo xyzzy > foo &&\n-\techo barf > bar &&\n-\tgit add bar &&\n-\tgit diff > expect &&\n-\tgit diff --staged > expect-staged &&\n-\t# Perform merge\n-\ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t# Post-merge worktree changes\n-\techo yzxxz > foo &&\n-\techo blech > baz &&\n-\t### When aborting the merge, git will discard staged changes (bar)\n-\t### and unmerged changes (baz). Other changes that are neither\n-\t### staged nor marked as unmerged (foo), will be preserved. For\n-\t### these changed, git cannot tell pre-merge changes apart from\n-\t### post-merge changes, so the post-merge changes will be\n-\t### preserved. Change expectations accordingly:\n-\tgit diff -- foo > expect &&\n-\trm expect-staged &&\n-\ttouch expect-staged &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff > actual &&\n-\ttest_cmp expect actual &&\n-\tgit diff --staged > actual-staged &&\n-\ttest_cmp expect-staged actual-staged\n-'\n-\n-test_expect_success 'Reset worktree changes' '\n-\tgit reset --hard \"$pre_merge_head\"\n-'\n-\n-test_expect_success 'Abort merge with pre- and post-merge index changes' '\n-\t# Pre-merge worktree changes\n-\techo xyzzy > foo &&\n-\techo barf > bar &&\n-\tgit add bar &&\n-\tgit diff > expect &&\n-\tgit diff --staged > expect-staged &&\n-\t# Perform merge\n-\ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t# Post-merge worktree changes\n-\techo yzxxz > foo &&\n-\techo blech > baz &&\n-\tgit add foo bar &&\n-\t### When aborting the merge, git will discard all staged changes\n-\t### (foo, bar and baz), and no changes will be preserved. Whether\n-\t### the changes were staged pre- or post-merge does not matter\n-\t### (except for not preventing starting the merge).\n-\t### Change expectations accordingly:\n-\trm expect expect-staged &&\n-\ttouch expect &&\n-\ttouch expect-staged &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff > actual &&\n-\ttest_cmp expect actual &&\n-\tgit diff --staged > actual-staged &&\n-\ttest_cmp expect-staged actual-staged\n-'\n-\n test_done\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349114","messageId":"20180603065810.23841-8-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 7/7] merge: fix misleading pre-merge check documentation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:10Z","receivedAt":"2018-06-03T06:58:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"builtin/merge.c contains this important requirement for merge strategies:\n\n ...the index must be in sync with the head commit. The strategies are\n responsible to ensure this.\n\nHowever, Documentation/git-merge.txt says:\n\n ...[merge will] abort if there are any changes registered in the index\n relative to the `HEAD` commit. (One exception is when the changed\n index entries are in the state that would result from the merge\n already.)\n\nInterestingly, prior to commit c0be8aa06b85 (\"Documentation/git-merge.txt:\nPartial rewrite of How Merge Works\", 2008-07-19),\nDocumentation/git-merge.txt said much more:\n\n ...the index file must match the tree of `HEAD` commit...\n [NOTE]\n This is a bit of a lite. In certain special cases [explained\n in detail]...\n Otherwise, merge will refuse to do any harm to your repository\n (that is...your working tree...and index are left intact).\n\nSo, this suggests that the exceptions existed because there were special\ncases where it would case no harm, and potentially be slightly more\nconvenient for the user. While the current text in git-merge.txt does\nlist a condition under which it would be safe to proceed despite the index\nnot matching HEAD, it does not match what is actually implemented, in\nthree different ways:\n\n * The exception is written to describe what unpack-trees allows. Not\n all merge strategies allow such an exception, though, making this\n description misleading. 'ours' and 'octopus' merges have strictly\n enforced index==HEAD for a while, and the commit previous to this\n one made 'recursive' do so as well.\n\n * If someone did a three-way content merge on a specific file using\n versions from the relevant commits and staged it prior to running\n merge, then that path would technically satisfy the exception listed\n in git-merge.txt. unpack-trees.c would still error out on the path,\n though, because it defers the three-way content merge logic to other\n parts of the code (resolve, octopus, or recursive) and has no way of\n checking whether the index entry from before the merge will match\n the end result of the merge.\n\n * The exception as implemented in unpack-trees actually only checked\n that the index matched the MERGE_HEAD version of the file and that\n HEAD matched the merge base. Assuming no renames, that would indeed\n provide cases where the index matches the end result we'd get from a\n merge. But renames means unpack-trees is checking that it instead\n matches something other than what the final result will be, risking\n either erroring out when we shouldn't need to, or not erroring out\n when we should and overwriting the user's staged changes.\n\nIn addition to the wording behind this exception being misleading, it is\nalso somewhat surprising to see how many times the code for the special\ncases were wrong or the check to make sure the index matched head was\nforgotten altogether:\n\n* Prior to commit ee6566e8d70d (\"[PATCH] Rewrite read-tree\", 2005-09-05),\n there were many cases where an unclean index entry was allowed (look for\n merged_entry_allow_dirty()); it appears that in those cases, the merge\n would have simply overwritten staged changes with the result of the\n merge. Thus, the merge result would have been correct, but the user's\n uncommitted changes could be thrown away without warning.\n\n* Prior to commit 160252f81626 (\"git-merge-ours: make sure our index\n matches HEAD\", 2005-11-03), the 'ours' merge strategy did not check\n whether the index matched HEAD. If it didn't, the resulting merge\n would include all the staged changes, and thus wasn't really an 'ours'\n strategy.\n\n* Prior to commit 3ec62ad9ffba (\"merge-octopus: abort if index does not\n match HEAD\", 2016-04-09), 'octopus' merges did not check whether the\n index matched HEAD, also resulting in any staged changes from before\n the commit silently being folded into the resulting merge. commit\n a6ee883b8eb5 (\"t6044: new merge testcases for when index doesn't match\n HEAD\", 2016-04-09) was also added at the same time to try to test to\n make sure all strategies did the necessary checking for the requirement\n that the index match HEAD. Sadly, it didn't catch all the cases, as\n evidenced by the remainder of this list...\n\n* Prior to commit 65170c07d466 (\"merge-recursive: avoid incorporating\n uncommitted changes in a merge\", 2017-12-21), merge-recursive simply\n relied on unpack_trees() to do the necessary check, but in one special\n case it avoided calling unpack_trees() entirely and accidentally ended\n up silently including any staged changes from before the merge in the\n resulting merge commit.\n\n* The commit immediately before this one in this series noted that the\n exceptions were written in a way that assumed no renames, making it\n unsafe for merge-recursive to use. merge-recursive was modified to\n use its own check to enforce that index==HEAD.\n\nThis history makes it very tempting to go into builtin/merge.c and replace\nthe comment that strategies must enforce that index matches HEAD with code\nthat just enforces it. At this point, that would only affect the\n'resolve' strategy; all other strategies have each been modified to\nmanually enforce it. (However, note that index==HEAD is not strictly\nenforced for fast-forward merges, as those are not considered a merge\nstrategy and they trigger in builtin/merge.c before the section in the\ncode where the relevant comment is found.)\n\nBut, even if we don't take the step of just fixing these problems by\nenforcing index==HEAD for all strategies, we at least need to update this\nmisleading documentation in git-merge.txt. For now, just modify the claim\nin Documentation/git-merge.txt to fix the error. The precise details\naround combination of merges strategies and special cases probably is not\nrelevant to most users, so simply state that exceptions may exist but are\nnarrow and vary depending upon which merge strategy is in use.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-merge.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex d5dfd8430f..141bd72284 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -122,9 +122,9 @@ merge' may need to update.\n \n To avoid recording unrelated changes in the merge commit,\n 'git pull' and 'git merge' will also abort if there are any changes\n-registered in the index relative to the `HEAD` commit. (One\n-exception is when the changed index entries are in the state that\n-would result from the merge already.)\n+registered in the index relative to the `HEAD` commit. (Special\n+narrow exceptions to this rule may exist depending on which merge\n+strategy is in use, but generally, the index must match HEAD.)\n \n If all named commits are already ancestors of `HEAD`, 'git merge'\n will exit early with the message \"Already up to date.\"\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349115","messageId":"20180603065810.23841-5-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:07Z","receivedAt":"2018-06-03T06:58:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"`git merge-recursive` does a three-way merge between user-specified trees\nbase, head, and remote. Since the user is allowed to specify head, we can\nnot necesarily assume that head == HEAD.\n\nWe modify index_has_changes() to take an extra argument specifying the\ntree to compare the index to. If NULL, it will compare to HEAD. We then\nuse this from merge-recursive to make sure we compare to the\nuser-specified head.\n\nSigned-off-by: Elijah Newren \n---\n\nI'm really unsure where the index_has_changes() declaration should go;\nI stuck it in tree.h, but is there a better spot?\n\n builtin/am.c | 6 +++---\n cache.h | 8 --------\n merge-recursive.c | 2 +-\n merge.c | 10 ++++++----\n t/t6044-merge-unrelated-index-changes.sh | 2 +-\n tree.h | 8 ++++++++\n 6 files changed, 19 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 2fc2d1e82c..c5b76cd095 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1763,7 +1763,7 @@ static void am_run(struct am_state *state, int resume)\n \n \trefresh_and_write_cache();\n \n-\tif (index_has_changes(&sb)) {\n+\tif (index_has_changes(&sb, NULL)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n@@ -1820,7 +1820,7 @@ static void am_run(struct am_state *state, int resume)\n \t\t\t * Applying the patch to an earlier tree and merging\n \t\t\t * the result may have produced the same tree as ours.\n \t\t\t */\n-\t\t\tif (!apply_status && !index_has_changes(NULL)) {\n+\t\t\tif (!apply_status && !index_has_changes(NULL, NULL)) {\n \t\t\t\tsay(state, stdout, _(\"No changes -- Patch already applied.\"));\n \t\t\t\tgoto next;\n \t\t\t}\n@@ -1878,7 +1878,7 @@ static void am_resolve(struct am_state *state)\n \n \tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg), state->msg);\n \n-\tif (!index_has_changes(NULL)) {\n+\tif (!index_has_changes(NULL, NULL)) {\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n \t\t\t\"If there is nothing left to stage, chances are that something else\\n\"\n \t\t\t\"already introduced the same changes; you might want to skip this patch.\"));\ndiff --git a/cache.h b/cache.h\nindex 89a107a7f7..439b9d9f6e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -634,14 +634,6 @@ extern int discard_index(struct index_state *);\n extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n extern int unmerged_index(const struct index_state *);\n \n-/**\n- * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n- * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n- * strbuf is provided, the space-separated list of files that differ will be\n- * appended to it.\n- */\n-extern int index_has_changes(struct strbuf *sb);\n-\n extern int verify_path(const char *path, unsigned mode);\n extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);\n extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b3deb7b182..762aa087d0 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3263,7 +3263,7 @@ int merge_trees(struct merge_options *o,\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (!o->call_depth && index_has_changes(&sb)) {\n+\t\tif (!o->call_depth && index_has_changes(&sb, head)) {\n \t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n \t\t\t sb.buf);\n \t\t\treturn -1;\ndiff --git a/merge.c b/merge.c\nindex 0783858739..965d287646 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -14,19 +14,21 @@ static const char *merge_argument(struct commit *commit)\n \treturn oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);\n }\n \n-int index_has_changes(struct strbuf *sb)\n+int index_has_changes(struct strbuf *sb, struct tree *tree)\n {\n-\tstruct object_id head;\n+\tstruct object_id cmp;\n \tint i;\n \n-\tif (!get_oid_tree(\"HEAD\", &head)) {\n+\tif (tree)\n+\t\tcmp = tree->object.oid;\n+\tif (tree || !get_oid_tree(\"HEAD\", &cmp)) {\n \t\tstruct diff_options opt;\n \n \t\tdiff_setup(&opt);\n \t\topt.flags.exit_with_status = 1;\n \t\tif (!sb)\n \t\t\topt.flags.quick = 1;\n-\t\tdo_diff_cache(&head, &opt);\n+\t\tdo_diff_cache(&cmp, &opt);\n \t\tdiffcore_std(&opt);\n \t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n \t\t\tif (i)\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 3876cfa4fa..1d43712c52 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -126,7 +126,7 @@ test_expect_success 'recursive, when merge branch matches merge base' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n-test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n+test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \tgit reset --hard &&\n \tgit checkout C^0 &&\n \ndiff --git a/tree.h b/tree.h\nindex e2a80be4ef..2e1d8ea720 100644\n--- a/tree.h\n+++ b/tree.h\n@@ -37,4 +37,12 @@ extern int read_tree_recursive(struct tree *tree,\n extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,\n \t\t struct index_state *istate);\n \n+/**\n+ * Returns 1 if the index differs from tree, 0 otherwise. If tree is NULL,\n+ * compares to HEAD. If tree is NULL and on an unborn branch, returns 1 if\n+ * there are entries in the index, 0 otherwise. If an strbuf is provided,\n+ * the space-separated list of files that differ will be appended to it.\n+ */\n+extern int index_has_changes(struct strbuf *sb, struct tree *tree);\n+\n #endif /* TREE_H */\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349116","messageId":"20180603065810.23841-2-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[RFC PATCH 1/7] t6044: verify that merges expected to abort actually abort","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-03T06:58:04Z","receivedAt":"2018-06-03T06:58:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"t6044 has lots of tests for verifying that merge will abort as expected\nwhen there are changes staged before the merge starts. However, it only\nchecked for non-zero exit code, which could mean that the merge ran to\ncompletion with conflicts. Check that the merge was actually correctly\naborted, i.e. that .git/MERGE_HEAD is not present.\n\nThis changes one of the tests from expect_success to expect_failure.\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 32 ++++++++++++++++--------\n 1 file changed, 21 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 23b86fb977..f9c2f8179e 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -82,7 +82,8 @@ test_expect_success 'ff update, important file modified' '\n \ttouch subdir/e &&\n \tgit add subdir/e &&\n \n-\ttest_must_fail git merge E^0\n+\ttest_must_fail git merge E^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'resolve, trivial' '\n@@ -91,7 +92,8 @@ test_expect_success 'resolve, trivial' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s resolve C^0\n+\ttest_must_fail git merge -s resolve C^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'resolve, non-trivial' '\n@@ -100,7 +102,8 @@ test_expect_success 'resolve, non-trivial' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s resolve D^0\n+\ttest_must_fail git merge -s resolve D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'recursive' '\n@@ -109,16 +112,18 @@ test_expect_success 'recursive' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s recursive C^0\n+\ttest_must_fail git merge -s recursive C^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n-test_expect_success 'recursive, when merge branch matches merge base' '\n+test_expect_failure 'recursive, when merge branch matches merge base' '\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 F^0\n+\ttest_must_fail git merge -s recursive F^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'octopus, unrelated file touched' '\n@@ -127,7 +132,8 @@ test_expect_success 'octopus, unrelated file touched' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge C^0 D^0\n+\ttest_must_fail git merge C^0 D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'octopus, related file removed' '\n@@ -136,7 +142,8 @@ test_expect_success 'octopus, related file removed' '\n \n \tgit rm b &&\n \n-\ttest_must_fail git merge C^0 D^0\n+\ttest_must_fail git merge C^0 D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'octopus, related file modified' '\n@@ -145,7 +152,8 @@ test_expect_success 'octopus, related file modified' '\n \n \techo 12 >>a && git add a &&\n \n-\ttest_must_fail git merge C^0 D^0\n+\ttest_must_fail git merge C^0 D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'ours' '\n@@ -154,7 +162,8 @@ test_expect_success 'ours' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s ours C^0\n+\ttest_must_fail git merge -s ours C^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'subtree' '\n@@ -163,7 +172,8 @@ test_expect_success 'subtree' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s subtree E^0\n+\ttest_must_fail git merge -s subtree E^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_done\n-- \n2.18.0.rc0.49.g3c08dc0fef\n\n"},{"id":"349143","messageId":"d8f9f7a4-16c7-8258-1210-ad3d32f3d57b@ramsayjones.plus.com","threadId":"48632","inReplyTo":"20180603065810.23841-5-newren@gmail.com","subject":"Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-03T13:52:12Z","receivedAt":"2018-06-03T13:52:17Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/06/18 07:58, Elijah Newren wrote:\n> `git merge-recursive` does a three-way merge between user-specified trees\n> base, head, and remote. Since the user is allowed to specify head, we can\n> not necesarily assume that head == HEAD.\n> \n> We modify index_has_changes() to take an extra argument specifying the\n> tree to compare the index to. If NULL, it will compare to HEAD. We then\n> use this from merge-recursive to make sure we compare to the\n> user-specified head.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> \n> I'm really unsure where the index_has_changes() declaration should go;\n> I stuck it in tree.h, but is there a better spot?\n\nErr, leave it where it is and '#include \"tree.h\"' ? :-D\n\nATB,\nRamsay Jones\n\n\n"},{"id":"349198","messageId":"20180603233747.GB288937@genre.crustytoothpaste.net","threadId":"48632","inReplyTo":"d8f9f7a4-16c7-8258-1210-ad3d32f3d57b@ramsayjones.plus.com","subject":"Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-06-03T23:37:47Z","receivedAt":"2018-06-03T23:37:55Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jun 03, 2018 at 02:52:12PM +0100, Ramsay Jones wrote:\n> On 03/06/18 07:58, Elijah Newren wrote:\n> > I'm really unsure where the index_has_changes() declaration should go;\n> > I stuck it in tree.h, but is there a better spot?\n> \n> Err, leave it where it is and '#include \"tree.h\"' ? :-D\n\nOr leave it where it is and use a forward structure declaration?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"349202","messageId":"00bc05ba-da77-716f-94f5-c414d6020608@ramsayjones.plus.com","threadId":"48632","inReplyTo":"20180603233747.GB288937@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-04T02:26:49Z","receivedAt":"2018-06-04T02:26:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/06/18 00:37, brian m. carlson wrote:\n> On Sun, Jun 03, 2018 at 02:52:12PM +0100, Ramsay Jones wrote:\n>> On 03/06/18 07:58, Elijah Newren wrote:\n>>> I'm really unsure where the index_has_changes() declaration should go;\n>>> I stuck it in tree.h, but is there a better spot?\n>>\n>> Err, leave it where it is and '#include \"tree.h\"' ? :-D\n> \n> Or leave it where it is and use a forward structure declaration?\n\nIndeed, I had intended to mention that possibility as well.\n\n[Note: the \"merge-recursive.h\" header file references several\n'struct tree *' parameters, but does not itself include a\ndeclaration/definition from any source. So, in all of the six\nfiles that #include it, it relies on a previous #include to\nprovide such a declaration/definition. I haven't checked, but\nI think that it is usually provided by the \"commit.h\" header (even\non the single occasion that \"tree.h\" was included!).]\n\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"349206","messageId":"xmqqvaaz5jcv.fsf@gitster-ct.c.googlers.com","threadId":"48632","inReplyTo":"20180603065810.23841-5-newren@gmail.com","subject":"Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T03:19:28Z","receivedAt":"2018-06-04T03:19:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> `git merge-recursive` does a three-way merge between user-specified trees\n> base, head, and remote. Since the user is allowed to specify head, we can\n> not necesarily assume that head == HEAD.\n>\n> We modify index_has_changes() to take an extra argument specifying the\n> tree to compare the index to. If NULL, it will compare to HEAD. We then\n> use this from merge-recursive to make sure we compare to the\n> user-specified head.\n>\n> Signed-off-by: Elijah Newren \n> ---\n>\n> I'm really unsure where the index_has_changes() declaration should go;\n> I stuck it in tree.h, but is there a better spot?\n\nI think I saw you tried to lift an assumption that we're always\nworking on the_index in a separate patch recently. Should that\nlogic apply also to this part of the codebase? IOW, shouldn't\nindex_has_changes() take a pointer to istate (as opposed to a\nfunction that uses the implicit the_index that should be named as\n\"cache_has_changes()\" or something?)\n\nI tend to think this function as part of the larger read-cache.c\nfamily whose definitions are in cache.h and accompanied by macros\nthat are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we\nwere to move it elsewhere, I'd keep the header part as-is and\nimplementation to read-cache.c to keep it together with the family,\nbut I do not see a huge issue with the current placement, either.\n\n> diff --git a/cache.h b/cache.h\n> index 89a107a7f7..439b9d9f6e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -634,14 +634,6 @@ extern int discard_index(struct index_state *);\n> extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n> extern int unmerged_index(const struct index_state *);\n> \n> -/**\n> - * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n> - * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n> - * strbuf is provided, the space-separated list of files that differ will be\n> - * appended to it.\n> - */\n> -extern int index_has_changes(struct strbuf *sb);\n> -\n> extern int verify_path(const char *path, unsigned mode);\n> extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);\n> extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index b3deb7b182..762aa087d0 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3263,7 +3263,7 @@ int merge_trees(struct merge_options *o,\n> \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \n> -\t\tif (!o->call_depth && index_has_changes(&sb)) {\n> +\t\tif (!o->call_depth && index_has_changes(&sb, head)) {\n> \t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n> \t\t\t sb.buf);\n> \t\t\treturn -1;\n> diff --git a/merge.c b/merge.c\n> index 0783858739..965d287646 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -14,19 +14,21 @@ static const char *merge_argument(struct commit *commit)\n> \treturn oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);\n> }\n> \n> -int index_has_changes(struct strbuf *sb)\n> +int index_has_changes(struct strbuf *sb, struct tree *tree)\n> {\n> -\tstruct object_id head;\n> +\tstruct object_id cmp;\n> \tint i;\n> \n> -\tif (!get_oid_tree(\"HEAD\", &head)) {\n> +\tif (tree)\n> +\t\tcmp = tree->object.oid;\n> +\tif (tree || !get_oid_tree(\"HEAD\", &cmp)) {\n> \t\tstruct diff_options opt;\n> \n> \t\tdiff_setup(&opt);\n> \t\topt.flags.exit_with_status = 1;\n> \t\tif (!sb)\n> \t\t\topt.flags.quick = 1;\n> -\t\tdo_diff_cache(&head, &opt);\n> +\t\tdo_diff_cache(&cmp, &opt);\n> \t\tdiffcore_std(&opt);\n> \t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n> \t\t\tif (i)\n> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\n> index 3876cfa4fa..1d43712c52 100755\n> --- a/t/t6044-merge-unrelated-index-changes.sh\n> +++ b/t/t6044-merge-unrelated-index-changes.sh\n> @@ -126,7 +126,7 @@ test_expect_success 'recursive, when merge branch matches merge base' '\n> \ttest_path_is_missing .git/MERGE_HEAD\n> '\n> \n> -test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n> +test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n> \tgit reset --hard &&\n> \tgit checkout C^0 &&\n> \n> diff --git a/tree.h b/tree.h\n> index e2a80be4ef..2e1d8ea720 100644\n> --- a/tree.h\n> +++ b/tree.h\n> @@ -37,4 +37,12 @@ extern int read_tree_recursive(struct tree *tree,\n> extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,\n> \t\t struct index_state *istate);\n> \n> +/**\n> + * Returns 1 if the index differs from tree, 0 otherwise. If tree is NULL,\n> + * compares to HEAD. If tree is NULL and on an unborn branch, returns 1 if\n> + * there are entries in the index, 0 otherwise. If an strbuf is provided,\n> + * the space-separated list of files that differ will be appended to it.\n> + */\n> +extern int index_has_changes(struct strbuf *sb, struct tree *tree);\n> +\n> #endif /* TREE_H */\n"},{"id":"349318","messageId":"CABPp-BERW3VtKxwKZ=-K6=pdVZydHRRKKGce2S=sttqfcDeRDA@mail.gmail.com","threadId":"48632","inReplyTo":"xmqqvaaz5jcv.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-05T07:14:13Z","receivedAt":"2018-06-05T07:14:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 3, 2018 at 8:19 PM, Junio C Hamano wrote:\n> Elijah Newren writes:\n>\n>> `git merge-recursive` does a three-way merge between user-specified trees\n>> base, head, and remote. Since the user is allowed to specify head, we can\n>> not necesarily assume that head == HEAD.\n>>\n>> We modify index_has_changes() to take an extra argument specifying the\n>> tree to compare the index to. If NULL, it will compare to HEAD. We then\n>> use this from merge-recursive to make sure we compare to the\n>> user-specified head.\n>>\n>> Signed-off-by: Elijah Newren \n>> ---\n>>\n>> I'm really unsure where the index_has_changes() declaration should go;\n>> I stuck it in tree.h, but is there a better spot?\n>\n> I think I saw you tried to lift an assumption that we're always\n> working on the_index in a separate patch recently. Should that\n> logic apply also to this part of the codebase? IOW, shouldn't\n> index_has_changes() take a pointer to istate (as opposed to a\n> function that uses the implicit the_index that should be named as\n> \"cache_has_changes()\" or something?)\n>\n> I tend to think this function as part of the larger read-cache.c\n> family whose definitions are in cache.h and accompanied by macros\n> that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we\n> were to move it elsewhere, I'd keep the header part as-is and\n> implementation to read-cache.c to keep it together with the family,\n> but I do not see a huge issue with the current placement, either.\n\nThat's good point; the goal to lift assumptions on the_index should\nprobably also apply here. I'll make the change.\n(And it was actually Duy's patch that I was reviewing, but close\nenough.) I'll take a look at moving it to read-cache.c as well.\n"},{"id":"349599","messageId":"CABPp-BGZiM=9bXHw0_90x43k51rCNRisM9ccGVRVwQxWkntiBA@mail.gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-8-newren@gmail.com","subject":"Re: [RFC PATCH 7/7] merge: fix misleading pre-merge check documentation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-07T05:27:31Z","receivedAt":"2018-06-07T05:27:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 2, 2018 at 11:58 PM, Elijah Newren wrote:\n> builtin/merge.c contains this important requirement for merge strategies:\n>\n> ...the index must be in sync with the head commit. The strategies are\n> responsible to ensure this.\n>\n> However, Documentation/git-merge.txt says:\n>\n> ...[merge will] abort if there are any changes registered in the index\n> relative to the `HEAD` commit. (One exception is when the changed\n> index entries are in the state that would result from the merge\n> already.)\n>\n> Interestingly, prior to commit c0be8aa06b85 (\"Documentation/git-merge.txt:\n> Partial rewrite of How Merge Works\", 2008-07-19),\n> Documentation/git-merge.txt said much more:\n>\n> ...the index file must match the tree of `HEAD` commit...\n> [NOTE]\n> This is a bit of a lite. In certain special cases [explained\n> in detail]...\n> Otherwise, merge will refuse to do any harm to your repository\n> (that is...your working tree...and index are left intact).\n>\n> So, this suggests that the exceptions existed because there were special\n> cases where it would case no harm, and potentially be slightly more\n> convenient for the user. While the current text in git-merge.txt does\n> list a condition under which it would be safe to proceed despite the index\n> not matching HEAD, it does not match what is actually implemented, in\n> three different ways:\n>\n> * The exception is written to describe what unpack-trees allows. Not\n> all merge strategies allow such an exception, though, making this\n> description misleading. 'ours' and 'octopus' merges have strictly\n> enforced index==HEAD for a while, and the commit previous to this\n> one made 'recursive' do so as well.\n>\n> * If someone did a three-way content merge on a specific file using\n> versions from the relevant commits and staged it prior to running\n> merge, then that path would technically satisfy the exception listed\n> in git-merge.txt. unpack-trees.c would still error out on the path,\n> though, because it defers the three-way content merge logic to other\n> parts of the code (resolve, octopus, or recursive) and has no way of\n> checking whether the index entry from before the merge will match\n> the end result of the merge.\n>\n> * The exception as implemented in unpack-trees actually only checked\n> that the index matched the MERGE_HEAD version of the file and that\n> HEAD matched the merge base. Assuming no renames, that would indeed\n> provide cases where the index matches the end result we'd get from a\n> merge. But renames means unpack-trees is checking that it instead\n> matches something other than what the final result will be, risking\n> either erroring out when we shouldn't need to, or not erroring out\n> when we should and overwriting the user's staged changes.\n>\n> In addition to the wording behind this exception being misleading, it is\n> also somewhat surprising to see how many times the code for the special\n> cases were wrong or the check to make sure the index matched head was\n> forgotten altogether:\n>\n> * Prior to commit ee6566e8d70d (\"[PATCH] Rewrite read-tree\", 2005-09-05),\n> there were many cases where an unclean index entry was allowed (look for\n> merged_entry_allow_dirty()); it appears that in those cases, the merge\n> would have simply overwritten staged changes with the result of the\n> merge. Thus, the merge result would have been correct, but the user's\n> uncommitted changes could be thrown away without warning.\n>\n> * Prior to commit 160252f81626 (\"git-merge-ours: make sure our index\n> matches HEAD\", 2005-11-03), the 'ours' merge strategy did not check\n> whether the index matched HEAD. If it didn't, the resulting merge\n> would include all the staged changes, and thus wasn't really an 'ours'\n> strategy.\n>\n> * Prior to commit 3ec62ad9ffba (\"merge-octopus: abort if index does not\n> match HEAD\", 2016-04-09), 'octopus' merges did not check whether the\n> index matched HEAD, also resulting in any staged changes from before\n> the commit silently being folded into the resulting merge. commit\n> a6ee883b8eb5 (\"t6044: new merge testcases for when index doesn't match\n> HEAD\", 2016-04-09) was also added at the same time to try to test to\n> make sure all strategies did the necessary checking for the requirement\n> that the index match HEAD. Sadly, it didn't catch all the cases, as\n> evidenced by the remainder of this list...\n>\n> * Prior to commit 65170c07d466 (\"merge-recursive: avoid incorporating\n> uncommitted changes in a merge\", 2017-12-21), merge-recursive simply\n> relied on unpack_trees() to do the necessary check, but in one special\n> case it avoided calling unpack_trees() entirely and accidentally ended\n> up silently including any staged changes from before the merge in the\n> resulting merge commit.\n>\n> * The commit immediately before this one in this series noted that the\n> exceptions were written in a way that assumed no renames, making it\n> unsafe for merge-recursive to use. merge-recursive was modified to\n> use its own check to enforce that index==HEAD.\n>\n> This history makes it very tempting to go into builtin/merge.c and replace\n> the comment that strategies must enforce that index matches HEAD with code\n> that just enforces it. At this point, that would only affect the\n> 'resolve' strategy; all other strategies have each been modified to\n> manually enforce it.\n\nI'm curious if anyone has comments on this last paragraph above.\nWould anyone object to strictly enforcing index matches HEAD before\nall types of merges?\n"},{"id":"349910","messageId":"CABPp-BFhneCqGBRT3qtS_d2eUdjgj+5PWsyWJ7XGHBD6dxunqQ@mail.gmail.com","threadId":"48632","inReplyTo":"CABPp-BERW3VtKxwKZ=-K6=pdVZydHRRKKGce2S=sttqfcDeRDA@mail.gmail.com","subject":"Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-06-11T16:15:06Z","receivedAt":"2018-06-11T16:15:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jun 5, 2018 at 12:14 AM, Elijah Newren wrote:\n> On Sun, Jun 3, 2018 at 8:19 PM, Junio C Hamano wrote:\n>> Elijah Newren writes:\n>>\n>>> `git merge-recursive` does a three-way merge between user-specified trees\n>>> base, head, and remote. Since the user is allowed to specify head, we can\n>>> not necesarily assume that head == HEAD.\n>>>\n>>> We modify index_has_changes() to take an extra argument specifying the\n>>> tree to compare the index to. If NULL, it will compare to HEAD. We then\n>>> use this from merge-recursive to make sure we compare to the\n>>> user-specified head.\n>>>\n>>> Signed-off-by: Elijah Newren \n>>> ---\n>>>\n>>> I'm really unsure where the index_has_changes() declaration should go;\n>>> I stuck it in tree.h, but is there a better spot?\n>>\n>> I think I saw you tried to lift an assumption that we're always\n>> working on the_index in a separate patch recently. Should that\n>> logic apply also to this part of the codebase? IOW, shouldn't\n>> index_has_changes() take a pointer to istate (as opposed to a\n>> function that uses the implicit the_index that should be named as\n>> \"cache_has_changes()\" or something?)\n>>\n>> I tend to think this function as part of the larger read-cache.c\n>> family whose definitions are in cache.h and accompanied by macros\n>> that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we\n>> were to move it elsewhere, I'd keep the header part as-is and\n>> implementation to read-cache.c to keep it together with the family,\n>> but I do not see a huge issue with the current placement, either.\n>\n> That's good point; the goal to lift assumptions on the_index should\n> probably also apply here. I'll make the change.\n> (And it was actually Duy's patch that I was reviewing, but close\n> enough.) I'll take a look at moving it to read-cache.c as well.\n\nMaking it not depend on the_index will require changes to make\ndiff-lib.c not depend on the_index first, so this is going to have to\nwait for Duy's changes mentioned at\nhttps://public-inbox.org/git/CACsJy8Ba74iSPf4_zFxuV=_uNJgL6Z2QunOvAvi3qab-6EWi5g@mail.gmail.com/.\nI'll re-roll this series on top of Duy's when it comes out.\n"},{"id":"351455","messageId":"20180701012503.14382-1-newren@gmail.com","threadId":"48632","inReplyTo":"20180603065810.23841-1-newren@gmail.com","subject":"[PATCH v2 0/9] Fix merge issues with index not matching HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:24:54Z","receivedAt":"2018-07-01T01:25:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series exists to fix problems for merges when the index doesn't\nmatch HEAD. We've had an almost comical number of these types of\nproblems in our history, as thoroughly documented in the commit\nmessage for the final patch.\n\nv1 can be found here:\n https://public-inbox.org/git/20180603065810.23841-1-newren@gmail.com/\n\nChanges since v1 (full branch-diff below):\n * Minor wording tweaks to a few commit messages (fixing typos,\n rewrapping, etc.)\n \n * Move index_has_changes() to read-cache.c, and (partially) lift the\n assumption that we're always operating on the_index -- as\n suggested by Junio. A full lift of the assumption would conflict\n with and duplicate work Duy is doing, so there is a simple BUG()\n check in place for now.\n \n * Add two new patches to the _beginning_ of the series, to implement\n the last point. Reason: Since this series will probably conflict\n slightly with Duy's not-yet-submitted work to remove assumption of\n the_index throughout the codebase, I figured this would make it\n the clearest and easiest for him to fix up small conflicts.\n (Alternatively, if folks would rather that my series wait for his\n to go through, it should make it much easier for me to rebase my\n series on top of his work by having these placeholders at the\n beginning.)\n\nQuestions I'm particularly interested in reviewers addressing:\n\n * Do my two patches at the beginning make sense? In particular, is\n the BUG() a reasonable way to limit conflicts with Duy for now,\n while he works on more thoroughly stamping out assumed the_index\n usage?\n\n * Does the series flow well? I was curious if I should reorder the\n series when I submitted v1, but no one commented on that question.\n\n * The second to last patch points out that current git incorrectly\n implements what would be a safe exception to the index matching\n HEAD before a merge rule. Would it be more desireable to\n correctly implement the safe exception (even though it would be\n somewhat difficult), rather than just disallow the exception for\n merge-recursive as I did in that patch?\n\n * Given the large number of problems we've had in this area -- as\n documented in the final commit message -- should we be more\n defensive and disallow all merge strategies from having even\n so-called safe exceptions? We could do this in a single place,\n which would have prevented all the current and most if not all\n historical problems in this area, by just enforcing that the index\n match HEAD in builtin/merge.c. (See the second to last paragraph\n of the last commit message for more details.)\n\nElijah Newren (9):\n read-cache.c: move index_has_changes() from merge.c\n index_has_changes(): avoid assuming operating on the_index\n t6044: verify that merges expected to abort actually abort\n t6044: add a testcase for index matching head, when head doesn't match\n HEAD\n merge-recursive: make sure when we say we abort that we actually abort\n merge-recursive: fix assumption that head tree being merged is HEAD\n t6044: add more testcases with staged changes before a merge is\n invoked\n merge-recursive: enforce rule that index matches head before merging\n merge: fix misleading pre-merge check documentation\n\n Documentation/git-merge.txt | 6 +-\n builtin/am.c | 7 +-\n cache.h | 16 +--\n merge-recursive.c | 14 +--\n merge.c | 31 ------\n read-cache.c | 40 ++++++++\n t/t6044-merge-unrelated-index-changes.sh | 67 +++++++++++--\n t/t7504-commit-msg-hook.sh | 4 +-\n t/t7611-merge-abort.sh | 118 -----------------------\n 9 files changed, 124 insertions(+), 179 deletions(-)\n\n-: --------- > 1: ff2501ac4 read-cache.c: move index_has_changes() from merge.c\n-: --------- > 2: 5813ca722 index_has_changes(): avoid assuming operating on the_index\n1: 730e5e483 = 3: ca11503bd t6044: verify that merges expected to abort actually abort\n2: 36f7cc0a3 = 4: 386390899 t6044: add a testcase for index matching head, when head doesn't match HEAD\n3: 70899afa3 ! 5: 8a900d2ee merge-recursive: make sure when we say we abort that we actually abort\n @@ -19,7 +19,7 @@\n @@\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n - \t\tif (!o->call_depth && index_has_changes(&sb)) {\n + \t\tif (!o->call_depth && index_has_changes(&the_index, &sb)) {\n -\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n +\t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n \t\t\t sb.buf);\n4: eab2f36a4 ! 6: 2564b29e9 merge-recursive: fix assumption that head tree being merged is HEAD\n @@ -6,10 +6,9 @@\n base, head, and remote. Since the user is allowed to specify head, we can\n not necesarily assume that head == HEAD.\n \n - We modify index_has_changes() to take an extra argument specifying the\n - tree to compare the index to. If NULL, it will compare to HEAD. We then\n - use this from merge-recursive to make sure we compare to the\n - user-specified head.\n + Modify index_has_changes() to take an extra argument specifying the tree\n + to compare against. If NULL, it will compare to HEAD. We then use this\n + from merge-recursive to make sure we compare to the user-specified head.\n \n Signed-off-by: Elijah Newren \n \n @@ -20,8 +19,8 @@\n \n \trefresh_and_write_cache();\n \n --\tif (index_has_changes(&sb)) {\n -+\tif (index_has_changes(&sb, NULL)) {\n +-\tif (index_has_changes(&the_index, &sb)) {\n ++\tif (index_has_changes(&the_index, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n @@ -29,8 +28,9 @@\n \t\t\t * Applying the patch to an earlier tree and merging\n \t\t\t * the result may have produced the same tree as ours.\n \t\t\t */\n --\t\t\tif (!apply_status && !index_has_changes(NULL)) {\n -+\t\t\tif (!apply_status && !index_has_changes(NULL, NULL)) {\n +-\t\t\tif (!apply_status && !index_has_changes(&the_index, NULL)) {\n ++\t\t\tif (!apply_status &&\n ++\t\t\t !index_has_changes(&the_index, NULL, NULL)) {\n \t\t\t\tsay(state, stdout, _(\"No changes -- Patch already applied.\"));\n \t\t\t\tgoto next;\n \t\t\t}\n @@ -38,8 +38,8 @@\n \n \tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg), state->msg);\n \n --\tif (!index_has_changes(NULL)) {\n -+\tif (!index_has_changes(NULL, NULL)) {\n +-\tif (!index_has_changes(&the_index, NULL)) {\n ++\tif (!index_has_changes(&the_index, NULL, NULL)) {\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n \t\t\t\"If there is nothing left to stage, chances are that something else\\n\"\n \t\t\t\"already introduced the same changes; you might want to skip this patch.\"));\n @@ -48,20 +48,32 @@\n --- a/cache.h\n +++ b/cache.h\n @@\n - extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n + /* Forward structure decls */\n + struct pathspec;\n + struct child_process;\n ++struct tree;\n + \n + /*\n + * Copy the sha1 and stat state of a cache entry from one to\n +@@\n extern int unmerged_index(const struct index_state *);\n \n --/**\n -- * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n -- * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n -- * strbuf is provided, the space-separated list of files that differ will be\n -- * appended to it.\n -- */\n --extern int index_has_changes(struct strbuf *sb);\n --\n + /**\n +- * Returns 1 if istate differs from HEAD, 0 otherwise. When on an unborn\n +- * branch, returns 1 if there are entries in istate, 0 otherwise. If an\n +- * strbuf is provided, the space-separated list of files that differ will\n +- * be appended to it.\n ++ * Returns 1 if istate differs from tree, 0 otherwise. If tree is NULL,\n ++ * compares istate to HEAD. If tree is NULL and on an unborn branch,\n ++ * returns 1 if there are entries in istate, 0 otherwise. If an strbuf is\n ++ * provided, the space-separated list of files that differ will be appended\n ++ * to it.\n + */\n + extern int index_has_changes(const struct index_state *istate,\n ++\t\t\t struct tree *tree,\n + \t\t\t struct strbuf *sb);\n + \n extern int verify_path(const char *path, unsigned mode);\n - extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);\n - extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);\n \n diff --git a/merge-recursive.c b/merge-recursive.c\n --- a/merge-recursive.c\n @@ -70,26 +82,31 @@\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n --\t\tif (!o->call_depth && index_has_changes(&sb)) {\n -+\t\tif (!o->call_depth && index_has_changes(&sb, head)) {\n +-\t\tif (!o->call_depth && index_has_changes(&the_index, &sb)) {\n ++\t\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n \t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n \t\t\t sb.buf);\n \t\t\treturn -1;\n \n -diff --git a/merge.c b/merge.c\n ---- a/merge.c\n -+++ b/merge.c\n +diff --git a/read-cache.c b/read-cache.c\n +--- a/read-cache.c\n ++++ b/read-cache.c\n @@\n - \treturn oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);\n + \treturn 0;\n }\n \n --int index_has_changes(struct strbuf *sb)\n -+int index_has_changes(struct strbuf *sb, struct tree *tree)\n +-int index_has_changes(const struct index_state *istate, struct strbuf *sb)\n ++int index_has_changes(const struct index_state *istate,\n ++\t\t struct tree *tree,\n ++\t\t struct strbuf *sb)\n {\n -\tstruct object_id head;\n +\tstruct object_id cmp;\n \tint i;\n \n + \tif (istate != &the_index) {\n + \t\tBUG(\"index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.\");\n + \t}\n -\tif (!get_oid_tree(\"HEAD\", &head)) {\n +\tif (tree)\n +\t\tcmp = tree->object.oid;\n @@ -118,20 +135,3 @@\n \tgit reset --hard &&\n \tgit checkout C^0 &&\n \n -\n -diff --git a/tree.h b/tree.h\n ---- a/tree.h\n -+++ b/tree.h\n -@@\n - extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,\n - \t\t struct index_state *istate);\n - \n -+/**\n -+ * Returns 1 if the index differs from tree, 0 otherwise. If tree is NULL,\n -+ * compares to HEAD. If tree is NULL and on an unborn branch, returns 1 if\n -+ * there are entries in the index, 0 otherwise. If an strbuf is provided,\n -+ * the space-separated list of files that differ will be appended to it.\n -+ */\n -+extern int index_has_changes(struct strbuf *sb, struct tree *tree);\n -+\n - #endif /* TREE_H */\n5: 4aa0684c0 ! 7: 88a8e44a2 t6044: add more testcases with staged changes before a merge is invoked\n @@ -5,8 +5,9 @@\n According to Documentation/git-merge.txt,\n \n ...[merge will] abort if there are any changes registered in the index\n - relative to the `HEAD` commit. (One exception is when the changed index\n - entries are in the state that would result from the merge already.)\n + relative to the `HEAD` commit. (One exception is when the changed\n + index entries are in the state that would result from the merge\n + already.)\n \n Add some tests showing that this exception, while it does accurately state\n what would be a safe condition under which we could allow the merge to\n6: 905f2683f ! 8: c0049b788 merge-recursive: enforce rule that index matches head before merging\n @@ -48,16 +48,16 @@\n commit, just like the 'ours' and 'octopus' strategies do.\n \n Some testcase fixups were in order:\n - t6044: We no longer expect stray staged changes to sometimes result\n - in the merge continuing. Also, fixes a case where a merge\n - didn't abort but should have.\n - t7504: had a few tests that had stray staged changes that were not\n - actually part of the test under consideration\n t7611: had many tests designed to show that `git merge --abort` could\n not always restore the index and working tree to the state they\n were in before the merge started. The tests that were associated\n with having changes in the index before the merge started are no\n longer applicable, so they have been removed.\n + t7504: had a few tests that had stray staged changes that were not\n + actually part of the test under consideration\n + t6044: We no longer expect stray staged changes to sometimes result\n + in the merge continuing. Also, fix a case where a merge\n + didn't abort but should have.\n \n Signed-off-by: Elijah Newren \n \n @@ -70,7 +70,7 @@\n \tint code, clean;\n +\tstruct strbuf sb = STRBUF_INIT;\n +\n -+\tif (!o->call_depth && index_has_changes(&sb, head)) {\n ++\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n +\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n +\t\t sb.buf);\n +\t\treturn -1;\n @@ -84,7 +84,7 @@\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n -\t\tstruct strbuf sb = STRBUF_INIT;\n -\n --\t\tif (!o->call_depth && index_has_changes(&sb, head)) {\n +-\t\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n -\t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n -\t\t\t sb.buf);\n -\t\t\treturn -1;\n7: 69f6fe8b1 ! 9: 557c5d94c merge: fix misleading pre-merge check documentation\n @@ -20,7 +20,7 @@\n \n ...the index file must match the tree of `HEAD` commit...\n [NOTE]\n - This is a bit of a lite. In certain special cases [explained\n + This is a bit of a lie. In certain special cases [explained\n in detail]...\n Otherwise, merge will refuse to do any harm to your repository\n (that is...your working tree...and index are left intact).\n\n-- \n2.18.0.137.g2a11d05a6.dirty\n"},{"id":"351456","messageId":"20180701012503.14382-2-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 1/9] read-cache.c: move index_has_changes() from merge.c","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:24:55Z","receivedAt":"2018-07-01T01:25:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Since index_has_change() is an index-related function, move it to\nread-cache.c, only modifying it to avoid uses of the active_cache and\nactive_nr macros.\n\nSigned-off-by: Elijah Newren \n---\n merge.c | 31 -------------------------------\n read-cache.c | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 31 deletions(-)\n\ndiff --git a/merge.c b/merge.c\nindex 078385873..e30e03fb8 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -14,37 +14,6 @@ static const char *merge_argument(struct commit *commit)\n \treturn oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);\n }\n \n-int index_has_changes(struct strbuf *sb)\n-{\n-\tstruct object_id head;\n-\tint i;\n-\n-\tif (!get_oid_tree(\"HEAD\", &head)) {\n-\t\tstruct diff_options opt;\n-\n-\t\tdiff_setup(&opt);\n-\t\topt.flags.exit_with_status = 1;\n-\t\tif (!sb)\n-\t\t\topt.flags.quick = 1;\n-\t\tdo_diff_cache(&head, &opt);\n-\t\tdiffcore_std(&opt);\n-\t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n-\t\t\tif (i)\n-\t\t\t\tstrbuf_addch(sb, ' ');\n-\t\t\tstrbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n-\t\t}\n-\t\tdiff_flush(&opt);\n-\t\treturn opt.flags.has_changes != 0;\n-\t} else {\n-\t\tfor (i = 0; sb && i < active_nr; i++) {\n-\t\t\tif (i)\n-\t\t\t\tstrbuf_addch(sb, ' ');\n-\t\t\tstrbuf_addstr(sb, active_cache[i]->name);\n-\t\t}\n-\t\treturn !!active_nr;\n-\t}\n-}\n-\n int try_merge_command(const char *strategy, size_t xopts_nr,\n \t\t const char **xopts, struct commit_list *common,\n \t\t const char *head_arg, struct commit_list *remotes)\ndiff --git a/read-cache.c b/read-cache.c\nindex 372588260..5a9a93da3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -6,6 +6,8 @@\n #define NO_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"tempfile.h\"\n #include \"lockfile.h\"\n #include \"cache-tree.h\"\n@@ -1984,6 +1986,37 @@ int unmerged_index(const struct index_state *istate)\n \treturn 0;\n }\n \n+int index_has_changes(struct strbuf *sb)\n+{\n+\tstruct object_id head;\n+\tint i;\n+\n+\tif (!get_oid_tree(\"HEAD\", &head)) {\n+\t\tstruct diff_options opt;\n+\n+\t\tdiff_setup(&opt);\n+\t\topt.flags.exit_with_status = 1;\n+\t\tif (!sb)\n+\t\t\topt.flags.quick = 1;\n+\t\tdo_diff_cache(&head, &opt);\n+\t\tdiffcore_std(&opt);\n+\t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n+\t\t\tif (i)\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t\tstrbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n+\t\t}\n+\t\tdiff_flush(&opt);\n+\t\treturn opt.flags.has_changes != 0;\n+\t} else {\n+\t\tfor (i = 0; sb && i < the_index.cache_nr; i++) {\n+\t\t\tif (i)\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t\tstrbuf_addstr(sb, the_index.cache[i]->name);\n+\t\t}\n+\t\treturn !!the_index.cache_nr;\n+\t}\n+}\n+\n #define WRITE_BUFFER_SIZE 8192\n static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n static unsigned long write_buffer_len;\n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351457","messageId":"20180701012503.14382-3-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 2/9] index_has_changes(): avoid assuming operating on the_index","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:24:56Z","receivedAt":"2018-07-01T01:25:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Modify index_has_changes() to take a struct istate* instead of just\noperating on the_index. This is only a partial conversion, though,\nbecause we call do_diff_cache() which implicitly assumes work is to be\ndone on the_index. Ongoing work is being done elsewhere to do the\nremainder of the conversion, and thus is not duplicated here. Instead,\na simple check is put in place until that work is complete.\n\nSigned-off-by: Elijah Newren \n---\n builtin/am.c | 6 +++---\n cache.h | 11 ++++++-----\n merge-recursive.c | 2 +-\n read-cache.c | 11 +++++++----\n 4 files changed, 17 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6273ea519..24ad3e435 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1763,7 +1763,7 @@ static void am_run(struct am_state *state, int resume)\n \n \trefresh_and_write_cache();\n \n-\tif (index_has_changes(&sb)) {\n+\tif (index_has_changes(&the_index, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n@@ -1820,7 +1820,7 @@ static void am_run(struct am_state *state, int resume)\n \t\t\t * Applying the patch to an earlier tree and merging\n \t\t\t * the result may have produced the same tree as ours.\n \t\t\t */\n-\t\t\tif (!apply_status && !index_has_changes(NULL)) {\n+\t\t\tif (!apply_status && !index_has_changes(&the_index, NULL)) {\n \t\t\t\tsay(state, stdout, _(\"No changes -- Patch already applied.\"));\n \t\t\t\tgoto next;\n \t\t\t}\n@@ -1874,7 +1874,7 @@ static void am_resolve(struct am_state *state)\n \n \tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg), state->msg);\n \n-\tif (!index_has_changes(NULL)) {\n+\tif (!index_has_changes(&the_index, NULL)) {\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n \t\t\t\"If there is nothing left to stage, chances are that something else\\n\"\n \t\t\t\"already introduced the same changes; you might want to skip this patch.\"));\ndiff --git a/cache.h b/cache.h\nindex d49092d94..125f2767a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -635,12 +635,13 @@ extern void move_index_extensions(struct index_state *dst, struct index_state *s\n extern int unmerged_index(const struct index_state *);\n \n /**\n- * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n- * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n- * strbuf is provided, the space-separated list of files that differ will be\n- * appended to it.\n+ * Returns 1 if istate differs from HEAD, 0 otherwise. When on an unborn\n+ * branch, returns 1 if there are entries in istate, 0 otherwise. If an\n+ * strbuf is provided, the space-separated list of files that differ will\n+ * be appended to it.\n */\n-extern int index_has_changes(struct strbuf *sb);\n+extern int index_has_changes(const struct index_state *istate,\n+\t\t\t struct strbuf *sb);\n \n extern int verify_path(const char *path, unsigned mode);\n extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex bed4a5be0..f9384fabf 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3266,7 +3266,7 @@ int merge_trees(struct merge_options *o,\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (!o->call_depth && index_has_changes(&sb)) {\n+\t\tif (!o->call_depth && index_has_changes(&the_index, &sb)) {\n \t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n \t\t\t sb.buf);\n \t\t\treturn 0;\ndiff --git a/read-cache.c b/read-cache.c\nindex 5a9a93da3..ad3102083 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1986,11 +1986,14 @@ int unmerged_index(const struct index_state *istate)\n \treturn 0;\n }\n \n-int index_has_changes(struct strbuf *sb)\n+int index_has_changes(const struct index_state *istate, struct strbuf *sb)\n {\n \tstruct object_id head;\n \tint i;\n \n+\tif (istate != &the_index) {\n+\t\tBUG(\"index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.\");\n+\t}\n \tif (!get_oid_tree(\"HEAD\", &head)) {\n \t\tstruct diff_options opt;\n \n@@ -2008,12 +2011,12 @@ int index_has_changes(struct strbuf *sb)\n \t\tdiff_flush(&opt);\n \t\treturn opt.flags.has_changes != 0;\n \t} else {\n-\t\tfor (i = 0; sb && i < the_index.cache_nr; i++) {\n+\t\tfor (i = 0; sb && i < istate->cache_nr; i++) {\n \t\t\tif (i)\n \t\t\t\tstrbuf_addch(sb, ' ');\n-\t\t\tstrbuf_addstr(sb, the_index.cache[i]->name);\n+\t\t\tstrbuf_addstr(sb, istate->cache[i]->name);\n \t\t}\n-\t\treturn !!the_index.cache_nr;\n+\t\treturn !!istate->cache_nr;\n \t}\n }\n \n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351458","messageId":"20180701012503.14382-4-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 3/9] t6044: verify that merges expected to abort actually abort","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:24:57Z","receivedAt":"2018-07-01T01:25:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"t6044 has lots of tests for verifying that merge will abort as expected\nwhen there are changes staged before the merge starts. However, it only\nchecked for non-zero exit code, which could mean that the merge ran to\ncompletion with conflicts. Check that the merge was actually correctly\naborted, i.e. that .git/MERGE_HEAD is not present.\n\nThis changes one of the tests from expect_success to expect_failure.\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 32 ++++++++++++++++--------\n 1 file changed, 21 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 23b86fb97..f9c2f8179 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -82,7 +82,8 @@ test_expect_success 'ff update, important file modified' '\n \ttouch subdir/e &&\n \tgit add subdir/e &&\n \n-\ttest_must_fail git merge E^0\n+\ttest_must_fail git merge E^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'resolve, trivial' '\n@@ -91,7 +92,8 @@ test_expect_success 'resolve, trivial' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s resolve C^0\n+\ttest_must_fail git merge -s resolve C^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'resolve, non-trivial' '\n@@ -100,7 +102,8 @@ test_expect_success 'resolve, non-trivial' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s resolve D^0\n+\ttest_must_fail git merge -s resolve D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'recursive' '\n@@ -109,16 +112,18 @@ test_expect_success 'recursive' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s recursive C^0\n+\ttest_must_fail git merge -s recursive C^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n-test_expect_success 'recursive, when merge branch matches merge base' '\n+test_expect_failure 'recursive, when merge branch matches merge base' '\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 F^0\n+\ttest_must_fail git merge -s recursive F^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'octopus, unrelated file touched' '\n@@ -127,7 +132,8 @@ test_expect_success 'octopus, unrelated file touched' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge C^0 D^0\n+\ttest_must_fail git merge C^0 D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'octopus, related file removed' '\n@@ -136,7 +142,8 @@ test_expect_success 'octopus, related file removed' '\n \n \tgit rm b &&\n \n-\ttest_must_fail git merge C^0 D^0\n+\ttest_must_fail git merge C^0 D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'octopus, related file modified' '\n@@ -145,7 +152,8 @@ test_expect_success 'octopus, related file modified' '\n \n \techo 12 >>a && git add a &&\n \n-\ttest_must_fail git merge C^0 D^0\n+\ttest_must_fail git merge C^0 D^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'ours' '\n@@ -154,7 +162,8 @@ test_expect_success 'ours' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s ours C^0\n+\ttest_must_fail git merge -s ours C^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_expect_success 'subtree' '\n@@ -163,7 +172,8 @@ test_expect_success 'subtree' '\n \n \ttouch random_file && git add random_file &&\n \n-\ttest_must_fail git merge -s subtree E^0\n+\ttest_must_fail git merge -s subtree E^0 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n '\n \n test_done\n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351459","messageId":"20180701012503.14382-5-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 4/9] t6044: add a testcase for index matching head, when head doesn't match HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:24:58Z","receivedAt":"2018-07-01T01:25:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The `git merge-recursive` command allows the user to directly specify\nthree commits to merge -- base, head, and remote. (More than three can be\nspecified in the case of multiple merge bases.) Note that since the user\nis allowed to specify head, it need not match HEAD.\n\nVirtually every test and script in the current git.git codebase calls `git\nmerge-recursive` with head=HEAD, and likely external callers do as well,\nwhich is why this has gone unnoticed. There is one notable\ncounter-example: git-stash.sh. However, git-stash called `git\nmerge-recursive` with an index that matches the expected merge result,\nwhich happens to be a currently allowed exception to the \"index must match\nhead\" rule, so this never triggered an error previously.\n\nSince we would like to tighten up the \"index must match head\" rule, we\nneed to make sure we are comparing to the correct head. Add a testcase\nthat demonstrates the failure when we check the wrong HEAD.\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex f9c2f8179..92ec55255 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n+test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n+\tgit reset --hard &&\n+\tgit checkout C^0 &&\n+\n+\t# Make index match B\n+\tgit diff C B | git apply --cached &&\n+\t# Merge B & F, with B as \"head\"\n+\tgit merge-recursive A -- B F > out &&\n+\ttest_i18ngrep \"Already up to date\" out\n+'\n+\n test_expect_success 'octopus, unrelated file touched' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351460","messageId":"20180701012503.14382-9-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 8/9] merge-recursive: enforce rule that index matches head before merging","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:25:02Z","receivedAt":"2018-07-01T01:25:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"builtin/merge.c says that when we are about to perform a merge:\n\n ...the index must be in sync with the head commit. The strategies are\n responsible to ensure this.\n\nmerge-recursive has always relied on unpack_trees() to enforce this\nrequirement, except in the case of an \"Already up to date!\" merge.\nunpack-trees.c does not actually enforce this requirement, though. It\nallows for a pair of exceptions, in cases which it refers to as #14(ALT)\nand #2ALT. Documentation/technical/trivial-merge.txt can be consulted for\nthe precise meanings of the various case numbers and their meanings for\nunpack-trees.c, but we have a high-level description of the intent behind\nthese two exceptions in a combined and summarized form in\nDocumentation/git-merge.txt:\n\n ...[merge will] abort if there are any changes registered in the index\n relative to the `HEAD` commit. (One exception is when the changed index\n entries are in the state that would result from the merge already.)\n\nWhile this high-level description does describe conditions under which it\nwould be safe to allow the index to diverge from HEAD, it does not match\nwhat is actually implemented. In particular, unpack-trees.c has no\nknowledge of renames, and these two exceptions were written assuming that\nno renames take place. Once renames get into the mix, it is no longer\nsafe to allow the index to not match for #2ALT. We could modify\nunpack-trees to only allow #14(ALT) as an exception, but that would be\nmore strict than required for the resolve strategy (since the resolve\nstrategy doesn't handle renames at all). Therefore, unpack_trees.c seems\nlike the wrong place to fix this.\n\nFurther, if someone fixes the combination of break and rename detection\nand modifies merge-recursive to take advantage of the combination, then it\nwill also no longer be safe to allow the index to not match for #14(ALT)\nwhen the recursive strategy is in use. Therefore, leaving one of the\nexceptions in place with the recursive merge strategy feels like we are\njust leaving a latent bug in the code for folks in the future to stumble\nacross.\n\nIt may be possible to fix both unpack-trees and merge-recursive in a way\nthat implements the exception as stated in Documentation/git-merge.txt,\nbut it would be somewhat complex, possibly also buggy at first, and\nultimately, not all that valuable. Instead, just enforce the requirement\nstated in builtin/merge.c; error out if the index does not match the HEAD\ncommit, just like the 'ours' and 'octopus' strategies do.\n\nSome testcase fixups were in order:\n t7611: had many tests designed to show that `git merge --abort` could\n\t not always restore the index and working tree to the state they\n\t were in before the merge started. The tests that were associated\n\t with having changes in the index before the merge started are no\n longer applicable, so they have been removed.\n t7504: had a few tests that had stray staged changes that were not\n actually part of the test under consideration\n t6044: We no longer expect stray staged changes to sometimes result\n in the merge continuing. Also, fix a case where a merge\n didn't abort but should have.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 14 +--\n t/t6044-merge-unrelated-index-changes.sh | 19 ++--\n t/t7504-commit-msg-hook.sh | 4 +-\n t/t7611-merge-abort.sh | 118 -----------------------\n 4 files changed, 18 insertions(+), 137 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 855ca39ca..8f7a8e828 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3257,6 +3257,13 @@ int merge_trees(struct merge_options *o,\n \t\tstruct tree **result)\n {\n \tint code, clean;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n+\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n+\t\t sb.buf);\n+\t\treturn -1;\n+\t}\n \n \tif (o->subtree_shift) {\n \t\tmerge = shift_tree_object(head, merge, o->subtree_shift);\n@@ -3264,13 +3271,6 @@ int merge_trees(struct merge_options *o,\n \t}\n \n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n-\t\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n-\t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n-\t\t\t sb.buf);\n-\t\t\treturn -1;\n-\t\t}\n \t\toutput(o, 0, _(\"Already up to date!\"));\n \t\t*result = head;\n \t\treturn 1;\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex e609f14f8..6e0ecab9c 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -137,7 +137,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \ttest_i18ngrep \"Already up to date\" out\n '\n \n-test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '\n+test_expect_success 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n@@ -145,14 +145,12 @@ test_expect_failure 'recursive, when file has staged changes not matching HEAD n\n \ttest_seq 1 10 >subdir/a &&\n \tgit add subdir/a &&\n \n-\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n-\t# Since subdir/a matches neither HEAD nor what the merge would write\n-\t# to that file, the merge should fail to avoid overwriting what is\n-\t# currently found in subdir/a\n-\ttest_must_fail git merge -s recursive E^0\n+\t# We have staged changes; merge should error out\n+\ttest_must_fail git merge -s recursive E^0 2>err &&\n+\ttest_i18ngrep \"changes to the following files would be overwritten\" err\n '\n \n-test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '\n+test_expect_success 'recursive, when file has staged changes matching what a merge would give' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n@@ -160,10 +158,9 @@ test_expect_failure 'recursive, when file has staged changes matching what a mer\n \ttest_seq 1 11 >subdir/a &&\n \tgit add subdir/a &&\n \n-\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n-\t# Since subdir/a matches what the merge would write to that file,\n-\t# the merge should be safe to proceed\n-\tgit merge -s recursive E^0\n+\t# We have staged changes; merge should error out\n+\ttest_must_fail git merge -s recursive E^0 2>err &&\n+\ttest_i18ngrep \"changes to the following files would be overwritten\" err\n '\n \n test_expect_success 'octopus, unrelated file touched' '\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 302a3a208..31b9c6a2c 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -157,6 +157,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '\n \ttest_when_finished \"git branch -D newbranch\" &&\n \ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout --orphan newbranch &&\n+\tgit rm -f file &&\n \t: >file2 &&\n \tgit add file2 &&\n \tgit commit --no-verify file2 -m in-side-branch &&\n@@ -168,7 +169,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '\n chmod -x \"$HOOK\"\n test_expect_success POSIXPERM 'with non-executable hook' '\n \n-\techo \"content\" >> file &&\n+\techo \"content\" >file &&\n \tgit add file &&\n \tgit commit -m \"content\"\n \n@@ -249,6 +250,7 @@ test_expect_success 'hook called in git-merge picks up commit message' '\n \ttest_when_finished \"git branch -D newbranch\" &&\n \ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout --orphan newbranch &&\n+\tgit rm -f file &&\n \t: >file2 &&\n \tgit add file2 &&\n \tgit commit --no-verify file2 -m in-side-branch &&\ndiff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh\nindex 7b4798e8e..7c84a518a 100755\n--- a/t/t7611-merge-abort.sh\n+++ b/t/t7611-merge-abort.sh\n@@ -187,31 +187,6 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'Abort clean merge with matching dirty index' '\n-\tgit add bar &&\n-\tgit diff --staged > expect &&\n-\tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t### When aborting the merge, git will discard all staged changes,\n-\t### including those that were staged pre-merge. In other words,\n-\t### --abort will LOSE any staged changes (the staged changes that\n-\t### are lost must match the merge result, or the merge would not\n-\t### have been allowed to start). Change expectations accordingly:\n-\trm expect &&\n-\ttouch expect &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff --staged > actual &&\n-\ttest_cmp expect actual &&\n-\ttest -z \"$(git diff)\"\n-'\n-\n-test_expect_success 'Reset worktree changes' '\n-\tgit reset --hard \"$pre_merge_head\"\n-'\n-\n test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \techo barf > bar &&\n \tgit diff > expect &&\n@@ -223,97 +198,4 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'Abort conflicting merge with matching dirty index' '\n-\tgit add bar &&\n-\tgit diff --staged > expect &&\n-\ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t### When aborting the merge, git will discard all staged changes,\n-\t### including those that were staged pre-merge. In other words,\n-\t### --abort will LOSE any staged changes (the staged changes that\n-\t### are lost must match the merge result, or the merge would not\n-\t### have been allowed to start). Change expectations accordingly:\n-\trm expect &&\n-\ttouch expect &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff --staged > actual &&\n-\ttest_cmp expect actual &&\n-\ttest -z \"$(git diff)\"\n-'\n-\n-test_expect_success 'Reset worktree changes' '\n-\tgit reset --hard \"$pre_merge_head\"\n-'\n-\n-test_expect_success 'Abort merge with pre- and post-merge worktree changes' '\n-\t# Pre-merge worktree changes\n-\techo xyzzy > foo &&\n-\techo barf > bar &&\n-\tgit add bar &&\n-\tgit diff > expect &&\n-\tgit diff --staged > expect-staged &&\n-\t# Perform merge\n-\ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t# Post-merge worktree changes\n-\techo yzxxz > foo &&\n-\techo blech > baz &&\n-\t### When aborting the merge, git will discard staged changes (bar)\n-\t### and unmerged changes (baz). Other changes that are neither\n-\t### staged nor marked as unmerged (foo), will be preserved. For\n-\t### these changed, git cannot tell pre-merge changes apart from\n-\t### post-merge changes, so the post-merge changes will be\n-\t### preserved. Change expectations accordingly:\n-\tgit diff -- foo > expect &&\n-\trm expect-staged &&\n-\ttouch expect-staged &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff > actual &&\n-\ttest_cmp expect actual &&\n-\tgit diff --staged > actual-staged &&\n-\ttest_cmp expect-staged actual-staged\n-'\n-\n-test_expect_success 'Reset worktree changes' '\n-\tgit reset --hard \"$pre_merge_head\"\n-'\n-\n-test_expect_success 'Abort merge with pre- and post-merge index changes' '\n-\t# Pre-merge worktree changes\n-\techo xyzzy > foo &&\n-\techo barf > bar &&\n-\tgit add bar &&\n-\tgit diff > expect &&\n-\tgit diff --staged > expect-staged &&\n-\t# Perform merge\n-\ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n-\t# Post-merge worktree changes\n-\techo yzxxz > foo &&\n-\techo blech > baz &&\n-\tgit add foo bar &&\n-\t### When aborting the merge, git will discard all staged changes\n-\t### (foo, bar and baz), and no changes will be preserved. Whether\n-\t### the changes were staged pre- or post-merge does not matter\n-\t### (except for not preventing starting the merge).\n-\t### Change expectations accordingly:\n-\trm expect expect-staged &&\n-\ttouch expect &&\n-\ttouch expect-staged &&\n-\t# Abort merge\n-\tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n-\ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n-\tgit diff > actual &&\n-\ttest_cmp expect actual &&\n-\tgit diff --staged > actual-staged &&\n-\ttest_cmp expect-staged actual-staged\n-'\n-\n test_done\n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351461","messageId":"20180701012503.14382-6-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 5/9] merge-recursive: make sure when we say we abort that we actually abort","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:24:59Z","receivedAt":"2018-07-01T01:25:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 65170c07d4 (\"merge-recursive: avoid incorporating uncommitted\nchanges in a merge\", 2017-12-21), it was noted that there was a special\ncase when merge-recursive didn't rely on unpack_trees() to enforce the\nindex == HEAD requirement, and thus that it needed to do that enforcement\nitself. Unfortunately, it returned the wrong exit status, signalling that\nthe merge completed but had conflicts, rather than that it was aborted.\nFix the return code, and while we're at it, change the error message to\nmatch what unpack_trees() would have printed.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 4 ++--\n t/t6044-merge-unrelated-index-changes.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex f9384fabf..9c0699be5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3267,9 +3267,9 @@ int merge_trees(struct merge_options *o,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tif (!o->call_depth && index_has_changes(&the_index, &sb)) {\n-\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n+\t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n \t\t\t sb.buf);\n-\t\t\treturn 0;\n+\t\t\treturn -1;\n \t\t}\n \t\toutput(o, 0, _(\"Already up to date!\"));\n \t\t*result = head;\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 92ec55255..3876cfa4f 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -116,7 +116,7 @@ test_expect_success 'recursive' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n-test_expect_failure 'recursive, when merge branch matches merge base' '\n+test_expect_success 'recursive, when merge branch matches merge base' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351462","messageId":"20180701012503.14382-7-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 6/9] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:25:00Z","receivedAt":"2018-07-01T01:25:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"`git merge-recursive` does a three-way merge between user-specified trees\nbase, head, and remote. Since the user is allowed to specify head, we can\nnot necesarily assume that head == HEAD.\n\nModify index_has_changes() to take an extra argument specifying the tree\nto compare against. If NULL, it will compare to HEAD. We then use this\nfrom merge-recursive to make sure we compare to the user-specified head.\n\nSigned-off-by: Elijah Newren \n---\n builtin/am.c | 7 ++++---\n cache.h | 11 +++++++----\n merge-recursive.c | 2 +-\n read-cache.c | 12 ++++++++----\n t/t6044-merge-unrelated-index-changes.sh | 2 +-\n 5 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 24ad3e435..cb8e2b960 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1763,7 +1763,7 @@ static void am_run(struct am_state *state, int resume)\n \n \trefresh_and_write_cache();\n \n-\tif (index_has_changes(&the_index, &sb)) {\n+\tif (index_has_changes(&the_index, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n \t}\n@@ -1820,7 +1820,8 @@ static void am_run(struct am_state *state, int resume)\n \t\t\t * Applying the patch to an earlier tree and merging\n \t\t\t * the result may have produced the same tree as ours.\n \t\t\t */\n-\t\t\tif (!apply_status && !index_has_changes(&the_index, NULL)) {\n+\t\t\tif (!apply_status &&\n+\t\t\t !index_has_changes(&the_index, NULL, NULL)) {\n \t\t\t\tsay(state, stdout, _(\"No changes -- Patch already applied.\"));\n \t\t\t\tgoto next;\n \t\t\t}\n@@ -1874,7 +1875,7 @@ static void am_resolve(struct am_state *state)\n \n \tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg), state->msg);\n \n-\tif (!index_has_changes(&the_index, NULL)) {\n+\tif (!index_has_changes(&the_index, NULL, NULL)) {\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n \t\t\t\"If there is nothing left to stage, chances are that something else\\n\"\n \t\t\t\"already introduced the same changes; you might want to skip this patch.\"));\ndiff --git a/cache.h b/cache.h\nindex 125f2767a..b5a75806e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -218,6 +218,7 @@ struct cache_entry {\n /* Forward structure decls */\n struct pathspec;\n struct child_process;\n+struct tree;\n \n /*\n * Copy the sha1 and stat state of a cache entry from one to\n@@ -635,12 +636,14 @@ extern void move_index_extensions(struct index_state *dst, struct index_state *s\n extern int unmerged_index(const struct index_state *);\n \n /**\n- * Returns 1 if istate differs from HEAD, 0 otherwise. When on an unborn\n- * branch, returns 1 if there are entries in istate, 0 otherwise. If an\n- * strbuf is provided, the space-separated list of files that differ will\n- * be appended to it.\n+ * Returns 1 if istate differs from tree, 0 otherwise. If tree is NULL,\n+ * compares istate to HEAD. If tree is NULL and on an unborn branch,\n+ * returns 1 if there are entries in istate, 0 otherwise. If an strbuf is\n+ * provided, the space-separated list of files that differ will be appended\n+ * to it.\n */\n extern int index_has_changes(const struct index_state *istate,\n+\t\t\t struct tree *tree,\n \t\t\t struct strbuf *sb);\n \n extern int verify_path(const char *path, unsigned mode);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9c0699be5..855ca39ca 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3266,7 +3266,7 @@ int merge_trees(struct merge_options *o,\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (!o->call_depth && index_has_changes(&the_index, &sb)) {\n+\t\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n \t\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n \t\t\t sb.buf);\n \t\t\treturn -1;\ndiff --git a/read-cache.c b/read-cache.c\nindex ad3102083..acc8c390a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1986,22 +1986,26 @@ int unmerged_index(const struct index_state *istate)\n \treturn 0;\n }\n \n-int index_has_changes(const struct index_state *istate, struct strbuf *sb)\n+int index_has_changes(const struct index_state *istate,\n+\t\t struct tree *tree,\n+\t\t struct strbuf *sb)\n {\n-\tstruct object_id head;\n+\tstruct object_id cmp;\n \tint i;\n \n \tif (istate != &the_index) {\n \t\tBUG(\"index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.\");\n \t}\n-\tif (!get_oid_tree(\"HEAD\", &head)) {\n+\tif (tree)\n+\t\tcmp = tree->object.oid;\n+\tif (tree || !get_oid_tree(\"HEAD\", &cmp)) {\n \t\tstruct diff_options opt;\n \n \t\tdiff_setup(&opt);\n \t\topt.flags.exit_with_status = 1;\n \t\tif (!sb)\n \t\t\topt.flags.quick = 1;\n-\t\tdo_diff_cache(&head, &opt);\n+\t\tdo_diff_cache(&cmp, &opt);\n \t\tdiffcore_std(&opt);\n \t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n \t\t\tif (i)\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 3876cfa4f..1d43712c5 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -126,7 +126,7 @@ test_expect_success 'recursive, when merge branch matches merge base' '\n \ttest_path_is_missing .git/MERGE_HEAD\n '\n \n-test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n+test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \tgit reset --hard &&\n \tgit checkout C^0 &&\n \n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351463","messageId":"20180701012503.14382-8-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 7/9] t6044: add more testcases with staged changes before a merge is invoked","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:25:01Z","receivedAt":"2018-07-01T01:25:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"According to Documentation/git-merge.txt,\n\n ...[merge will] abort if there are any changes registered in the index\n relative to the `HEAD` commit. (One exception is when the changed\n index entries are in the state that would result from the merge\n already.)\n\nAdd some tests showing that this exception, while it does accurately state\nwhat would be a safe condition under which we could allow the merge to\nproceed, is not what is actually implemented.\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 29 ++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 1d43712c5..e609f14f8 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -137,6 +137,35 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \ttest_i18ngrep \"Already up to date\" out\n '\n \n+test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\tmkdir subdir &&\n+\ttest_seq 1 10 >subdir/a &&\n+\tgit add subdir/a &&\n+\n+\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n+\t# Since subdir/a matches neither HEAD nor what the merge would write\n+\t# to that file, the merge should fail to avoid overwriting what is\n+\t# currently found in subdir/a\n+\ttest_must_fail git merge -s recursive E^0\n+'\n+\n+test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\tmkdir subdir &&\n+\ttest_seq 1 11 >subdir/a &&\n+\tgit add subdir/a &&\n+\n+\t# HEAD has no subdir/a; merge would write 1..11 to subdir/a;\n+\t# Since subdir/a matches what the merge would write to that file,\n+\t# the merge should be safe to proceed\n+\tgit merge -s recursive E^0\n+'\n+\n test_expect_success 'octopus, unrelated file touched' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351464","messageId":"20180701012503.14382-10-newren@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-1-newren@gmail.com","subject":"[PATCH v2 9/9] merge: fix misleading pre-merge check documentation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-01T01:25:03Z","receivedAt":"2018-07-01T01:25:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"builtin/merge.c contains this important requirement for merge strategies:\n\n ...the index must be in sync with the head commit. The strategies are\n responsible to ensure this.\n\nHowever, Documentation/git-merge.txt says:\n\n ...[merge will] abort if there are any changes registered in the index\n relative to the `HEAD` commit. (One exception is when the changed\n index entries are in the state that would result from the merge\n already.)\n\nInterestingly, prior to commit c0be8aa06b85 (\"Documentation/git-merge.txt:\nPartial rewrite of How Merge Works\", 2008-07-19),\nDocumentation/git-merge.txt said much more:\n\n ...the index file must match the tree of `HEAD` commit...\n [NOTE]\n This is a bit of a lie. In certain special cases [explained\n in detail]...\n Otherwise, merge will refuse to do any harm to your repository\n (that is...your working tree...and index are left intact).\n\nSo, this suggests that the exceptions existed because there were special\ncases where it would case no harm, and potentially be slightly more\nconvenient for the user. While the current text in git-merge.txt does\nlist a condition under which it would be safe to proceed despite the index\nnot matching HEAD, it does not match what is actually implemented, in\nthree different ways:\n\n * The exception is written to describe what unpack-trees allows. Not\n all merge strategies allow such an exception, though, making this\n description misleading. 'ours' and 'octopus' merges have strictly\n enforced index==HEAD for a while, and the commit previous to this\n one made 'recursive' do so as well.\n\n * If someone did a three-way content merge on a specific file using\n versions from the relevant commits and staged it prior to running\n merge, then that path would technically satisfy the exception listed\n in git-merge.txt. unpack-trees.c would still error out on the path,\n though, because it defers the three-way content merge logic to other\n parts of the code (resolve, octopus, or recursive) and has no way of\n checking whether the index entry from before the merge will match\n the end result of the merge.\n\n * The exception as implemented in unpack-trees actually only checked\n that the index matched the MERGE_HEAD version of the file and that\n HEAD matched the merge base. Assuming no renames, that would indeed\n provide cases where the index matches the end result we'd get from a\n merge. But renames means unpack-trees is checking that it instead\n matches something other than what the final result will be, risking\n either erroring out when we shouldn't need to, or not erroring out\n when we should and overwriting the user's staged changes.\n\nIn addition to the wording behind this exception being misleading, it is\nalso somewhat surprising to see how many times the code for the special\ncases were wrong or the check to make sure the index matched head was\nforgotten altogether:\n\n* Prior to commit ee6566e8d70d (\"[PATCH] Rewrite read-tree\", 2005-09-05),\n there were many cases where an unclean index entry was allowed (look for\n merged_entry_allow_dirty()); it appears that in those cases, the merge\n would have simply overwritten staged changes with the result of the\n merge. Thus, the merge result would have been correct, but the user's\n uncommitted changes could be thrown away without warning.\n\n* Prior to commit 160252f81626 (\"git-merge-ours: make sure our index\n matches HEAD\", 2005-11-03), the 'ours' merge strategy did not check\n whether the index matched HEAD. If it didn't, the resulting merge\n would include all the staged changes, and thus wasn't really an 'ours'\n strategy.\n\n* Prior to commit 3ec62ad9ffba (\"merge-octopus: abort if index does not\n match HEAD\", 2016-04-09), 'octopus' merges did not check whether the\n index matched HEAD, also resulting in any staged changes from before\n the commit silently being folded into the resulting merge. commit\n a6ee883b8eb5 (\"t6044: new merge testcases for when index doesn't match\n HEAD\", 2016-04-09) was also added at the same time to try to test to\n make sure all strategies did the necessary checking for the requirement\n that the index match HEAD. Sadly, it didn't catch all the cases, as\n evidenced by the remainder of this list...\n\n* Prior to commit 65170c07d466 (\"merge-recursive: avoid incorporating\n uncommitted changes in a merge\", 2017-12-21), merge-recursive simply\n relied on unpack_trees() to do the necessary check, but in one special\n case it avoided calling unpack_trees() entirely and accidentally ended\n up silently including any staged changes from before the merge in the\n resulting merge commit.\n\n* The commit immediately before this one in this series noted that the\n exceptions were written in a way that assumed no renames, making it\n unsafe for merge-recursive to use. merge-recursive was modified to\n use its own check to enforce that index==HEAD.\n\nThis history makes it very tempting to go into builtin/merge.c and replace\nthe comment that strategies must enforce that index matches HEAD with code\nthat just enforces it. At this point, that would only affect the\n'resolve' strategy; all other strategies have each been modified to\nmanually enforce it. (However, note that index==HEAD is not strictly\nenforced for fast-forward merges, as those are not considered a merge\nstrategy and they trigger in builtin/merge.c before the section in the\ncode where the relevant comment is found.)\n\nBut, even if we don't take the step of just fixing these problems by\nenforcing index==HEAD for all strategies, we at least need to update this\nmisleading documentation in git-merge.txt. For now, just modify the claim\nin Documentation/git-merge.txt to fix the error. The precise details\naround combination of merges strategies and special cases probably is not\nrelevant to most users, so simply state that exceptions may exist but are\nnarrow and vary depending upon which merge strategy is in use.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-merge.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 6a5c00e2c..b050c8c2e 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -122,9 +122,9 @@ merge' may need to update.\n \n To avoid recording unrelated changes in the merge commit,\n 'git pull' and 'git merge' will also abort if there are any changes\n-registered in the index relative to the `HEAD` commit. (One\n-exception is when the changed index entries are in the state that\n-would result from the merge already.)\n+registered in the index relative to the `HEAD` commit. (Special\n+narrow exceptions to this rule may exist depending on which merge\n+strategy is in use, but generally, the index must match HEAD.)\n \n If all named commits are already ancestors of `HEAD`, 'git merge'\n will exit early with the message \"Already up to date.\"\n-- \n2.18.0.137.g2a11d05a6.dirty\n\n"},{"id":"351644","messageId":"xmqq601wqf3u.fsf@gitster-ct.c.googlers.com","threadId":"48632","inReplyTo":"20180701012503.14382-3-newren@gmail.com","subject":"Re: [PATCH v2 2/9] index_has_changes(): avoid assuming operating on the_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T19:44:05Z","receivedAt":"2018-07-03T19:44:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Modify index_has_changes() to take a struct istate* instead of just\n> operating on the_index. This is only a partial conversion, though,\n> because we call do_diff_cache() which implicitly assumes work is to be\n> done on the_index. Ongoing work is being done elsewhere to do the\n> remainder of the conversion, and thus is not duplicated here. Instead,\n> a simple check is put in place until that work is complete.\n\nYeah, that is an unfortunate but necessary compromise until we\ncreate do_diff_index() that can take an istate, and optionally turn\ndo_diff_cache() into\n\n #define do_diff_cache(...) do_diff_index(&the_index, ...)\n\nif there are many callers that want to work on the default in-core\nistate.\n"},{"id":"351645","messageId":"xmqq1sckqeh2.fsf@gitster-ct.c.googlers.com","threadId":"48632","inReplyTo":"20180701012503.14382-7-newren@gmail.com","subject":"Re: [PATCH v2 6/9] merge-recursive: fix assumption that head tree being merged is HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T19:57:45Z","receivedAt":"2018-07-03T19:57:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> `git merge-recursive` does a three-way merge between user-specified trees\n> base, head, and remote. Since the user is allowed to specify head, we can\n> not necesarily assume that head == HEAD.\n\nMakes sense.\n"},{"id":"351646","messageId":"xmqqwoucozje.fsf@gitster-ct.c.googlers.com","threadId":"48632","inReplyTo":"20180701012503.14382-9-newren@gmail.com","subject":"Re: [PATCH v2 8/9] merge-recursive: enforce rule that index matches head before merging","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T20:05:41Z","receivedAt":"2018-07-03T20:07:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> ...[merge will] abort if there are any changes registered in the index\n> relative to the `HEAD` commit. (One exception is when the changed index\n> entries are in the state that would result from the merge already.)\n>\n> While this high-level description does describe conditions under which it\n> would be safe to allow the index to diverge from HEAD, it does not match\n> what is actually implemented. In particular, unpack-trees.c has no\n> knowledge of renames, and these two exceptions were written assuming that\n> no renames take place.\n\nI think the above analysis is quite correct (I even suspect that the\nrule was outlined long before any renaming merge was designed).\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 855ca39ca..8f7a8e828 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3257,6 +3257,13 @@ int merge_trees(struct merge_options *o,\n> \t\tstruct tree **result)\n> {\n> \tint code, clean;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (!o->call_depth && index_has_changes(&the_index, head, &sb)) {\n> +\t\terr(o, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\n> +\t\t sb.buf);\n> +\t\treturn -1;\n> +\t}\n\nThis change to ensure the index is pristine upfront makes sense,\ntoo.\n"},{"id":"352179","messageId":"20180710203922.26367-1-szeder.dev@gmail.com","threadId":"48632","inReplyTo":"20180701012503.14382-5-newren@gmail.com","subject":"Re: [PATCH v2 4/9] t6044: add a testcase for index matching head, when head doesn't match HEAD","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-10T20:39:22Z","receivedAt":"2018-07-10T20:39:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\n> index f9c2f8179..92ec55255 100755\n> --- a/t/t6044-merge-unrelated-index-changes.sh\n> +++ b/t/t6044-merge-unrelated-index-changes.sh\n> @@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '\n> \ttest_path_is_missing .git/MERGE_HEAD\n> '\n> \n> +test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n> +\tgit reset --hard &&\n> +\tgit checkout C^0 &&\n> +\n> +\t# Make index match B\n> +\tgit diff C B | git apply --cached &&\n\nBranch 'C' contains the file 'c', which creates ambiguity on\ncase-insensitive file systems and makes the test fail:\n\n ++git reset --hard\n HEAD is now at ed5d5a6 B\n ++git checkout 'C^0'\n Previous HEAD position was ed5d5a6 B\n HEAD is now at 4fa59ce C\n ++git diff C B\n ++git apply --cached\n fatal: ambiguous argument 'C': both revision and filename\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n error: unrecognized input\n error: last command exited with $?=128\n not ok 8 - merge-recursive, when index==head but head!=HEAD\n\n> +\t# Merge B & F, with B as \"head\"\n> +\tgit merge-recursive A -- B F > out &&\n> +\ttest_i18ngrep \"Already up to date\" out\n> +'\n> +\n> test_expect_success 'octopus, unrelated file touched' '\n> \tgit reset --hard &&\n> \tgit checkout B^0 &&\n> -- \n> 2.18.0.137.g2a11d05a6.dirty\n> \n> \n"},{"id":"352192","messageId":"20180711030923.26499-1-newren@gmail.com","threadId":"48632","inReplyTo":"20180710203922.26367-1-szeder.dev@gmail.com","subject":"Re: [RFC PATCH 2/7] t6044: add a testcase for index matching head, when head doesn't match HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-11T03:09:23Z","receivedAt":"2018-07-11T03:11:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jul 10, 2018 at 1:39 PM, SZEDER Gábor wrote:\n>> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\n>> index f9c2f8179..92ec55255 100755\n>> --- a/t/t6044-merge-unrelated-index-changes.sh\n>> +++ b/t/t6044-merge-unrelated-index-changes.sh\n>> @@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '\n>> test_path_is_missing .git/MERGE_HEAD\n>> '\n>>\n>> +test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '\n>> + git reset --hard &&\n>> + git checkout C^0 &&\n>> +\n>> + # Make index match B\n>> + git diff C B | git apply --cached &&\n>\n> Branch 'C' contains the file 'c', which creates ambiguity on\n> case-insensitive file systems and makes the test fail:\n>\n> ++git reset --hard\n> HEAD is now at ed5d5a6 B\n> ++git checkout 'C^0'\n> Previous HEAD position was ed5d5a6 B\n> HEAD is now at 4fa59ce C\n> ++git diff C B\n> ++git apply --cached\n> fatal: ambiguous argument 'C': both revision and filename\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> error: unrecognized input\n> error: last command exited with $?=128\n> not ok 8 - merge-recursive, when index==head but head!=HEAD\n>\n\nGood catch; thanks for pointing it out. Here's a fix:\n\n-- 8< --\n\nSubject: [PATCH] fixup! t6044: add a testcase for index matching head, when\n head doesn't match HEAD\n\nSigned-off-by: Elijah Newren \n---\n t/t6044-merge-unrelated-index-changes.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 6e0ecab9c..5e3779ebc 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -131,7 +131,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '\n \tgit checkout C^0 &&\n \n \t# Make index match B\n-\tgit diff C B | git apply --cached &&\n+\tgit diff C B -- | git apply --cached &&\n \t# Merge B & F, with B as \"head\"\n \tgit merge-recursive A -- B F > out &&\n \ttest_i18ngrep \"Already up to date\" out\n-- \n2.18.0.132.g6e63b23f4\n\n"}]}