{"thread":{"id":"40648","subject":"[PATCH 0/3] detecting delete/modechange conflicts","startedAt":"2015-10-26T21:35:02Z","lastAt":"2015-10-26T21:54:06Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"272271","messageId":"20151026213502.GA17244@sigill.intra.peff.net","threadId":"40648","inReplyTo":null,"subject":"[PATCH 0/3] detecting delete/modechange conflicts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-26T21:35:02Z","receivedAt":"2015-10-26T21:35:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was surprised to find that:\n\n  # base commit\n  echo base >file &&\n  git add file &&\n  git commit -m base &&\n\n  # one side changes mode\n  chmod +x file &&\n  git commit -am executable &&\n\n  # the other deletes the file\n  git checkout -b other HEAD^ &&\n  git rm file &&\n  git commit -am removed &&\n\n  git merge master\n\nsilently completes the merge, dropping the mode change. We detect\ndelete/modify conflicts, but not delete/modechange conflicts.  The\ntrivial index merge does catch it, but both the resolve and recursive\nstrategies resolve it silently in favor of the deletion.\n\nAfter looking through the history and the list archive, I don't _think_\nthis was intentional, and we simply missed the case in both places. But\nmaybe somebody else knows something I don't. It seems like we should be\npunting to the user under the general principle of stupid and safe\nmerges.\n\n  [1/3]: t6031: move triple-rename test to t3030\n  [2/3]: t6031: generalize for recursive and resolve strategies\n  [3/3]: merge: detect delete/modechange conflict\n\n-Peff\n"},{"id":"272272","messageId":"20151026213618.GA17373@sigill.intra.peff.net","threadId":"40648","inReplyTo":"20151026213502.GA17244@sigill.intra.peff.net","subject":"[PATCH 1/3] t6031: move triple-rename test to t3030","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-26T21:36:18Z","receivedAt":"2015-10-26T21:36:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The t6031 test was introduced to check filemode handling of\nmerge-recursive. Much later, an unrelated test was tacked on\nto look at renames and d/f conflicts. This test does not\ndepend on anything that happened before (it actually blows\naway any existing content in the test repo). Let's move it\nto t3030, where there are more related tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI didn't actually look all that closely at what it does and what t3030\ndoes to see if there is overlap, and we could simply get rid of this.\nBut it _definitely_ doesn't belong in t6031, so this is at least a step\nforward.\n\n t/t3030-merge-recursive.sh | 30 ++++++++++++++++++++++++++++++\n t/t6031-merge-recursive.sh | 31 -------------------------------\n 2 files changed, 30 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 82e1854..6224187 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -629,5 +629,35 @@ test_expect_failure 'merge-recursive rename vs. rename/symlink' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'merging with triple rename across D/F conflict' '\n+\tgit reset --hard HEAD &&\n+\tgit checkout -b main &&\n+\tgit rm -rf . &&\n+\n+\techo \"just a file\" >sub1 &&\n+\tmkdir -p sub2 &&\n+\techo content1 >sub2/file1 &&\n+\techo content2 >sub2/file2 &&\n+\techo content3 >sub2/file3 &&\n+\tmkdir simple &&\n+\techo base >simple/bar &&\n+\tgit add -A &&\n+\ttest_tick &&\n+\tgit commit -m base &&\n+\n+\tgit checkout -b other &&\n+\techo more >>simple/bar &&\n+\ttest_tick &&\n+\tgit commit -a -m changesimplefile &&\n+\n+\tgit checkout main &&\n+\tgit rm sub1 &&\n+\tgit mv sub2 sub1 &&\n+\ttest_tick &&\n+\tgit commit -m changefiletodir &&\n+\n+\ttest_tick &&\n+\tgit merge other\n+'\n \n test_done\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nindex 6464a16..4053bd9 100755\n--- a/t/t6031-merge-recursive.sh\n+++ b/t/t6031-merge-recursive.sh\n@@ -53,35 +53,4 @@ test_expect_success FILEMODE 'verify executable bit on file' '\n \ttest -x file2\n '\n \n-test_expect_success 'merging with triple rename across D/F conflict' '\n-\tgit reset --hard HEAD &&\n-\tgit checkout -b main &&\n-\tgit rm -rf . &&\n-\n-\techo \"just a file\" >sub1 &&\n-\tmkdir -p sub2 &&\n-\techo content1 >sub2/file1 &&\n-\techo content2 >sub2/file2 &&\n-\techo content3 >sub2/file3 &&\n-\tmkdir simple &&\n-\techo base >simple/bar &&\n-\tgit add -A &&\n-\ttest_tick &&\n-\tgit commit -m base &&\n-\n-\tgit checkout -b other &&\n-\techo more >>simple/bar &&\n-\ttest_tick &&\n-\tgit commit -a -m changesimplefile &&\n-\n-\tgit checkout main &&\n-\tgit rm sub1 &&\n-\tgit mv sub2 sub1 &&\n-\ttest_tick &&\n-\tgit commit -m changefiletodir &&\n-\n-\ttest_tick &&\n-\tgit merge other\n-'\n-\n test_done\n-- \n2.6.2.481.g6ca35c3\n"},{"id":"272273","messageId":"20151026213717.GB17373@sigill.intra.peff.net","threadId":"40648","inReplyTo":"20151026213502.GA17244@sigill.intra.peff.net","subject":"[PATCH 2/3] t6031: generalize for recursive and resolve strategies","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-26T21:37:18Z","receivedAt":"2015-10-26T21:37:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This script tests the filemode handling of merge-recursive,\nbut we do not test the same thing for merge-resolve. Let's\ngeneralize the script a little:\n\n  1. Break out the setup steps for each test into a separate\n     snippet.\n\n  2. For each test, run it twice; once with \"-s recursive\"\n     and once with \"-s resolve\". We can avoid repeating\n     ourselves by adding a function.\n\n  3. Since we have a nice abstracted function, we can make\n     our tests more thorough by testing both directions\n     (change on \"ours\" versus \"theirs\").\n\nThis improves our test coverage, and will make this the\nplace to add more tests related to merging mode changes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSadly I changed too much for this to count as a rename (mostly due to\nthe new indentation). Oh well, the rename diff was not especially\nreadable, and it is probably just as well to read the new content as a\nsingle blob.\n\n t/t6031-merge-filemode.sh  | 77 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t6031-merge-recursive.sh | 56 ---------------------------------\n 2 files changed, 77 insertions(+), 56 deletions(-)\n create mode 100755 t/t6031-merge-filemode.sh\n delete mode 100755 t/t6031-merge-recursive.sh\n\ndiff --git a/t/t6031-merge-filemode.sh b/t/t6031-merge-filemode.sh\nnew file mode 100755\nindex 0000000..c6896e6\n--- /dev/null\n+++ b/t/t6031-merge-filemode.sh\n@@ -0,0 +1,77 @@\n+#!/bin/sh\n+\n+test_description='merge: handle file mode'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up mode change in one branch' '\n+\t: >file1 &&\n+\tgit add file1 &&\n+\tgit commit -m initial &&\n+\tgit checkout -b a1 master &&\n+\t: >dummy &&\n+\tgit add dummy &&\n+\tgit commit -m a &&\n+\tgit checkout -b b1 master &&\n+\ttest_chmod +x file1 &&\n+\tgit add file1 &&\n+\tgit commit -m b1\n+'\n+\n+do_one_mode () {\n+\tstrategy=$1\n+\tus=$2\n+\tthem=$3\n+\ttest_expect_success \"resolve single mode change ($strategy, $us)\" '\n+\t\tgit checkout -f $us &&\n+\t\tgit merge -s $strategy $them &&\n+\t\tgit ls-files -s file1 | grep ^100755\n+\t'\n+\n+\ttest_expect_success FILEMODE \"verify executable bit on file ($strategy, $us)\" '\n+\t\ttest -x file1\n+\t'\n+}\n+\n+do_one_mode recursive a1 b1\n+do_one_mode recursive b1 a1\n+do_one_mode resolve a1 b1\n+do_one_mode resolve b1 a1\n+\n+test_expect_success 'set up mode change in both branches' '\n+\tgit reset --hard HEAD &&\n+\tgit checkout -b a2 master &&\n+\t: >file2 &&\n+\tH=$(git hash-object file2) &&\n+\ttest_chmod +x file2 &&\n+\tgit commit -m a2 &&\n+\tgit checkout -b b2 master &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit -m b2 &&\n+\t{\n+\t\techo \"100755 $H 2\tfile2\"\n+\t\techo \"100644 $H 3\tfile2\"\n+\t} >expect\n+'\n+\n+do_both_modes () {\n+\tstrategy=$1\n+\ttest_expect_success \"detect conflict on double mode change ($strategy)\" '\n+\t\tgit reset --hard &&\n+\t\tgit checkout -f a2 &&\n+\t\ttest_must_fail git merge -s $strategy b2 &&\n+\t\tgit ls-files -u >actual &&\n+\t\ttest_cmp actual expect &&\n+\t\tgit ls-files -s file2 | grep ^100755\n+\t'\n+\n+\ttest_expect_success FILEMODE \"verify executable bit on file ($strategy)\" '\n+\t\ttest -x file2\n+\t'\n+}\n+\n+# both sides are equivalent, so no need to run both ways\n+do_both_modes recursive\n+do_both_modes resolve\n+\n+test_done\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\ndeleted file mode 100755\nindex 4053bd9..0000000\n--- a/t/t6031-merge-recursive.sh\n+++ /dev/null\n@@ -1,56 +0,0 @@\n-#!/bin/sh\n-\n-test_description='merge-recursive: handle file mode'\n-. ./test-lib.sh\n-\n-test_expect_success 'mode change in one branch: keep changed version' '\n-\t: >file1 &&\n-\tgit add file1 &&\n-\tgit commit -m initial &&\n-\tgit checkout -b a1 master &&\n-\t: >dummy &&\n-\tgit add dummy &&\n-\tgit commit -m a &&\n-\tgit checkout -b b1 master &&\n-\ttest_chmod +x file1 &&\n-\tgit add file1 &&\n-\tgit commit -m b1 &&\n-\tgit checkout a1 &&\n-\tgit merge-recursive master -- a1 b1 &&\n-\tgit ls-files -s file1 | grep ^100755\n-'\n-\n-test_expect_success FILEMODE 'verify executable bit on file' '\n-\ttest -x file1\n-'\n-\n-test_expect_success 'mode change in both branches: expect conflict' '\n-\tgit reset --hard HEAD &&\n-\tgit checkout -b a2 master &&\n-\t: >file2 &&\n-\tH=$(git hash-object file2) &&\n-\ttest_chmod +x file2 &&\n-\tgit commit -m a2 &&\n-\tgit checkout -b b2 master &&\n-\t: >file2 &&\n-\tgit add file2 &&\n-\tgit commit -m b2 &&\n-\tgit checkout a2 &&\n-\t(\n-\t\tgit merge-recursive master -- a2 b2\n-\t\ttest $? = 1\n-\t) &&\n-\tgit ls-files -u >actual &&\n-\t(\n-\t\techo \"100755 $H 2\tfile2\"\n-\t\techo \"100644 $H 3\tfile2\"\n-\t) >expect &&\n-\ttest_cmp actual expect &&\n-\tgit ls-files -s file2 | grep ^100755\n-'\n-\n-test_expect_success FILEMODE 'verify executable bit on file' '\n-\ttest -x file2\n-'\n-\n-test_done\n-- \n2.6.2.481.g6ca35c3\n"},{"id":"272274","messageId":"20151026213939.GC17373@sigill.intra.peff.net","threadId":"40648","inReplyTo":"20151026213502.GA17244@sigill.intra.peff.net","subject":"[PATCH 3/3] merge: detect delete/modechange conflict","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-26T21:39:39Z","receivedAt":"2015-10-26T21:39:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If one side deletes a file and the other changes its\ncontent, we notice and report a conflict. However, if\ninstead of changing the content, we change only the mode,\nthe merge does not notice (and the mode change is silently\ndropped).\n\nThe trivial index merge notices the problem and correctly\nleaves the conflict in the index, but both merge-recursive\nand merge-one-file will silently resolve this in favor of\nthe deletion.  In many cases that is a sane resolution, but\nwe should be punting to the user whenever there is any\nquestion. So let's detect and treat this as a conflict (in\nboth strategies).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe text for merge-one-file looks really gross and unlike\nour usual error messages, but it matches the similar add/add\ncase lower in the script. I don't know if its worth cleaning\nthose messages up, as I doubt many people use \"-s resolve\"\nthese days. But even if we do, that should be a separate\nseries.\n\n git-merge-one-file.sh     |  8 ++++++++\n merge-recursive.c         |  8 ++++++--\n t/t6031-merge-filemode.sh | 23 +++++++++++++++++++++++\n 3 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 07dfeb8..cdc02af 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -38,6 +38,14 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n # Deleted in both or deleted in one and unchanged in the other\n #\n \"$1..\" | \"$1.$1\" | \"$1$1.\")\n+\tif { test -z \"$6\" && test \"$5\" != \"$7\"; } ||\n+\t   { test -z \"$7\" && test \"$5\" != \"$6\"; }\n+\tthen\n+\t\techo \"ERROR: File $4 deleted on one branch but had its\" >&2\n+\t\techo \"ERROR: permissions changed on the other.\" >&2\n+\t\texit 1\n+\tfi\n+\n \tif test -n \"$2\"\n \tthen\n \t\techo \"Removing $4\"\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a5e74d8..21e680a 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1530,13 +1530,17 @@ static int read_sha1_strbuf(const unsigned char *sha1, struct strbuf *dst)\n }\n \n static int blob_unchanged(const unsigned char *o_sha,\n+\t\t\t  unsigned o_mode,\n \t\t\t  const unsigned char *a_sha,\n+\t\t\t  unsigned a_mode,\n \t\t\t  int renormalize, const char *path)\n {\n \tstruct strbuf o = STRBUF_INIT;\n \tstruct strbuf a = STRBUF_INIT;\n \tint ret = 0; /* assume changed for safety */\n \n+\tif (a_mode != o_mode)\n+\t\treturn 0;\n \tif (sha_eq(o_sha, a_sha))\n \t\treturn 1;\n \tif (!renormalize)\n@@ -1722,8 +1726,8 @@ static int process_entry(struct merge_options *o,\n \t} else if (o_sha && (!a_sha || !b_sha)) {\n \t\t/* Case A: Deleted in one */\n \t\tif ((!a_sha && !b_sha) ||\n-\t\t    (!b_sha && blob_unchanged(o_sha, a_sha, normalize, path)) ||\n-\t\t    (!a_sha && blob_unchanged(o_sha, b_sha, normalize, path))) {\n+\t\t    (!b_sha && blob_unchanged(o_sha, o_mode, a_sha, a_mode, normalize, path)) ||\n+\t\t    (!a_sha && blob_unchanged(o_sha, o_mode, b_sha, b_mode, normalize, path))) {\n \t\t\t/* Deleted in both or deleted in one and\n \t\t\t * unchanged in the other */\n \t\t\tif (a_sha)\ndiff --git a/t/t6031-merge-filemode.sh b/t/t6031-merge-filemode.sh\nindex c6896e6..7d06461 100755\n--- a/t/t6031-merge-filemode.sh\n+++ b/t/t6031-merge-filemode.sh\n@@ -74,4 +74,27 @@ do_both_modes () {\n do_both_modes recursive\n do_both_modes resolve\n \n+test_expect_success 'set up delete/modechange scenario' '\n+\tgit reset --hard &&\n+\tgit checkout -b deletion master &&\n+\tgit rm file1 &&\n+\tgit commit -m deletion\n+'\n+\n+do_delete_modechange () {\n+\tstrategy=$1\n+\tus=$2\n+\tthem=$3\n+\ttest_expect_success \"detect delete/modechange conflict ($strategy, $us)\" '\n+\t\tgit reset --hard &&\n+\t\tgit checkout $us &&\n+\t\ttest_must_fail git merge -s $strategy $them\n+\t'\n+}\n+\n+do_delete_modechange recursive b1 deletion\n+do_delete_modechange recursive deletion b1\n+do_delete_modechange resolve b1 deletion\n+do_delete_modechange resolve deletion b1\n+\n test_done\n-- \n2.6.2.481.g6ca35c3\n"},{"id":"272275","messageId":"xmqq8u6pfgl9.fsf@gitster.mtv.corp.google.com","threadId":"40648","inReplyTo":"20151026213502.GA17244@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] detecting delete/modechange conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-26T21:46:42Z","receivedAt":"2015-10-26T21:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> After looking through the history and the list archive, I don't _think_\n> this was intentional, and we simply missed the case in both places. But\n> maybe somebody else knows something I don't. It seems like we should be\n> punting to the user under the general principle of stupid and safe\n> merges.\n\nYes, I do not recall ever discussing and agreeing with Linus that we\nshould resolve to deletion over mode change, and I agree that it\nwould be very likely that this never came up in practice simply\nbecause in real life removal is already rare, mode change is rarer,\nand these happening to the same path in the same timeperiod to\nmatter in merges is even more rare.\n\nWe should definitely signal a conflict.\n\n>   [1/3]: t6031: move triple-rename test to t3030\n>   [2/3]: t6031: generalize for recursive and resolve strategies\n>   [3/3]: merge: detect delete/modechange conflict\n>\n> -Peff\n"},{"id":"272277","messageId":"20151026215406.GB17419@sigill.intra.peff.net","threadId":"40648","inReplyTo":"xmqq8u6pfgl9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] detecting delete/modechange conflicts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-26T21:54:06Z","receivedAt":"2015-10-26T21:54:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2015 at 02:46:42PM -0700, Junio C Hamano wrote:\n\n> > After looking through the history and the list archive, I don't _think_\n> > this was intentional, and we simply missed the case in both places. But\n> > maybe somebody else knows something I don't. It seems like we should be\n> > punting to the user under the general principle of stupid and safe\n> > merges.\n> \n> Yes, I do not recall ever discussing and agreeing with Linus that we\n> should resolve to deletion over mode change, and I agree that it\n> would be very likely that this never came up in practice simply\n> because in real life removal is already rare, mode change is rarer,\n> and these happening to the same path in the same timeperiod to\n> matter in merges is even more rare.\n> \n> We should definitely signal a conflict.\n\nThanks, that matches my thinking exactly.\n\nBTW, this came up because libgit2 does signal the conflict, and we are\nregression-testing a switch from merge-resolve over to libgit2 to power\nGitHub's \"merge\" button. Run it on enough test cases and you will find\none of everything. :)\n\n-Peff\n"}]}