{"thread":{"id":"20266","subject":"[PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.","startedAt":"2009-07-28T22:13:16Z","lastAt":"2009-07-30T06:05:06Z","messageCount":18,"participants":["James Pickens","Michael J Gruber","Junio C Hamano","Pickens, James E","Linus Torvalds","Kjetil Barvik"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"118997","messageId":"1248819198-13921-1-git-send-email-james.e.pickens@intel.com","threadId":"20266","inReplyTo":null,"subject":"More symlink/directory troubles","fromName":"James Pickens","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-28T22:13:16Z","receivedAt":"2009-07-28T22:13:16Z","isPatch":false,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"This is a follow up to the original thread and patch at\nhttp://article.gmane.org/gmane.comp.version-control.git/122297. There was\na bug report about problems when a directory is replaced with a symlink. I\nsaid that the patch fixed the bug for me, but I didn't test thoroughly\nenough, because it turns out there are 3 bugs, and the patch only fixed one\nof them.\n\nI am sending some test scripts to demonstrate the bugs in hopes of spurring\nsome activity here. I think the convention is to use test_expect_failure\nfor this sort of test, so that's what I did.\n\nUnfortunately I have very little time in the next 2 weeks, so I probably\nwon't be able to do much more than send the tests, and fix them up if\nnecessary. In 2 weeks I can take a look at the C code, if it isn't already\nfixed by then.\n\nJames\n"},{"id":"118996","messageId":"1248819198-13921-2-git-send-email-james.e.pickens@intel.com","threadId":"20266","inReplyTo":"1248819198-13921-1-git-send-email-james.e.pickens@intel.com","subject":"[PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.","fromName":"James Pickens","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-28T22:13:17Z","receivedAt":"2009-07-28T22:13:17Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"This test creates two directories, a/b and a/b-2, then replaces a/b with\na symlink to a/b-2, then merges that change into another branch that\ncontains an unrelated change.\n\nThere are two bugs:\n1. 'git checkout' wrongly deletes work tree file a/b-2/d.\n2. 'git merge' wrongly deletes work tree file a/b-2/d.\n\nSigned-off-by: James Pickens \n---\n t/t6035-merge-dir-to-symlink.sh | 32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n create mode 100755 t/t6035-merge-dir-to-symlink.sh\n\ndiff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\nnew file mode 100755\nindex 0000000..926c8ed\n--- /dev/null\n+++ b/t/t6035-merge-dir-to-symlink.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='merging when a directory was replaced with a symlink'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tmkdir -p a/b/c a/b-2/c &&\n+\t> a/b/c/d &&\n+\t> a/b-2/c/d &&\n+\t> a/x &&\n+\tgit add -A &&\n+\tgit commit -m base &&\n+\trm -rf a/b &&\n+\tln -s b-2 a/b &&\n+\tgit add -A &&\n+\tgit commit -m \"dir to symlink\"\n+'\n+\n+test_expect_failure 'checkout should not delete a/b-2/c/d' '\n+\tgit checkout -b temp HEAD^ &&\n+\ttest -f a/b-2/c/d\n+'\n+\n+test_expect_failure 'merge should not delete a/b-2/c/d' '\n+\techo x > a/x &&\n+\tgit add a/x &&\n+\tgit commit -m x &&\n+\tgit merge master &&\n+\ttest -f a/b-2/c/d\n+'\n+\n+test_done\n-- \n1.6.2.5.1\n"},{"id":"118995","messageId":"1248819198-13921-3-git-send-email-james.e.pickens@intel.com","threadId":"20266","inReplyTo":"1248819198-13921-2-git-send-email-james.e.pickens@intel.com","subject":"[PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.","fromName":"James Pickens","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-28T22:13:18Z","receivedAt":"2009-07-28T22:13:18Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"This test creates two directories, a/b and a/b-2, replaces a/b-2 with a\nsymlink to a/b, then merges that change into another branch that\ncontains unrelated changes. Since the changes are unrelated, the merge\nshould be free of conflicts, but 'git merge' gives a file/directory\nconflict.\n\nNote that this test is almost identical to t6035, except that instead of\nreplacing a/b with a symlink, it replaces a/b-2 with a symlink. This\ntest results in a merge conflict, whereas t6035 does not.\n\nSigned-off-by: James Pickens \n---\n t/t6036-merge-dir-to-symlink.sh | 30 ++++++++++++++++++++++++++++++\n 1 files changed, 30 insertions(+), 0 deletions(-)\n create mode 100755 t/t6036-merge-dir-to-symlink.sh\n\ndiff --git a/t/t6036-merge-dir-to-symlink.sh b/t/t6036-merge-dir-to-symlink.sh\nnew file mode 100755\nindex 0000000..020db7c\n--- /dev/null\n+++ b/t/t6036-merge-dir-to-symlink.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='merging when a directory was replaced with a symlink'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tmkdir -p a/b/c a/b-2/c &&\n+\t> a/b/c/d &&\n+\t> a/b-2/c/d &&\n+\t> a/x &&\n+\tgit add -A &&\n+\tgit commit -m base &&\n+\trm -rf a/b-2 &&\n+\tln -s b a/b-2 &&\n+\tgit add -A &&\n+\tgit commit -m \"dir to symlink\"\n+'\n+\n+test_expect_failure 'checkout should not delete a/b/c/d' '\n+\tgit checkout -b temp HEAD^ &&\n+\ttest -f a/b/c/d\n+'\n+\n+test_expect_failure 'merge should not have conflicts' '\n+\techo x > a/x &&\n+\tgit add a/x &&\n+\tgit commit -m x &&\n+\tgit merge master'\n+\n+test_done\n-- \n1.6.2.5.1\n"},{"id":"119029","messageId":"4A70062A.4040008@drmicha.warpmail.net","threadId":"20266","inReplyTo":"1248819198-13921-2-git-send-email-james.e.pickens@intel.com","subject":"Re: [PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-07-29T08:19:54Z","receivedAt":"2009-07-29T08:19:54Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"James Pickens venit, vidit, dixit 29.07.2009 00:13:\n> This test creates two directories, a/b and a/b-2, then replaces a/b with\n> a symlink to a/b-2, then merges that change into another branch that\n> contains an unrelated change.\n> \n> There are two bugs:\n> 1. 'git checkout' wrongly deletes work tree file a/b-2/d.\n> 2. 'git merge' wrongly deletes work tree file a/b-2/d.\n> \n> Signed-off-by: James Pickens \n> ---\n> t/t6035-merge-dir-to-symlink.sh | 32 ++++++++++++++++++++++++++++++++\n> 1 files changed, 32 insertions(+), 0 deletions(-)\n> create mode 100755 t/t6035-merge-dir-to-symlink.sh\n> \n> diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\n> new file mode 100755\n> index 0000000..926c8ed\n> --- /dev/null\n> +++ b/t/t6035-merge-dir-to-symlink.sh\n> @@ -0,0 +1,32 @@\n> +#!/bin/sh\n> +\n> +test_description='merging when a directory was replaced with a symlink'\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\tmkdir -p a/b/c a/b-2/c &&\n> +\t> a/b/c/d &&\n> +\t> a/b-2/c/d &&\n> +\t> a/x &&\n> +\tgit add -A &&\n> +\tgit commit -m base &&\n> +\trm -rf a/b &&\n> +\tln -s b-2 a/b &&\n> +\tgit add -A &&\n> +\tgit commit -m \"dir to symlink\"\n> +'\n> +\n> +test_expect_failure 'checkout should not delete a/b-2/c/d' '\n> +\tgit checkout -b temp HEAD^ &&\n> +\ttest -f a/b-2/c/d\n> +'\n> +\n> +test_expect_failure 'merge should not delete a/b-2/c/d' '\n> +\techo x > a/x &&\n> +\tgit add a/x &&\n> +\tgit commit -m x &&\n> +\tgit merge master &&\n> +\ttest -f a/b-2/c/d\n> +'\n> +\n> +test_done\n\nIsn't the failure of the second test caused by that of the first one?\na/b-2/c/d is gone from the worktree, and master does not touch it, so\nthe merge leaves the worktree version (non-existent) as is.\n\nMichael\n"},{"id":"119032","messageId":"4A70086C.9070408@drmicha.warpmail.net","threadId":"20266","inReplyTo":"1248819198-13921-3-git-send-email-james.e.pickens@intel.com","subject":"Re: [PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-07-29T08:29:32Z","receivedAt":"2009-07-29T08:29:32Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"James Pickens venit, vidit, dixit 29.07.2009 00:13:\n> This test creates two directories, a/b and a/b-2, replaces a/b-2 with a\n> symlink to a/b, then merges that change into another branch that\n> contains unrelated changes. Since the changes are unrelated, the merge\n> should be free of conflicts, but 'git merge' gives a file/directory\n> conflict.\n> \n> Note that this test is almost identical to t6035, except that instead of\n> replacing a/b with a symlink, it replaces a/b-2 with a symlink. This\n> test results in a merge conflict, whereas t6035 does not.\n\nIn fact they are identical: Exchange b for b-2 and vice versa everywhere\nand you get the same test, except for the fact that in 1/2 you \"test -f\"\nin the last step. But I'm sure that test fails at the merge step already\n(because of a dirty worktree), doesn't it? You should see this when\nrunning the test with -d/-v. (I'm guessing, I haven't run your test.)\n\n> \n> Signed-off-by: James Pickens \n> ---\n> t/t6036-merge-dir-to-symlink.sh | 30 ++++++++++++++++++++++++++++++\n> 1 files changed, 30 insertions(+), 0 deletions(-)\n> create mode 100755 t/t6036-merge-dir-to-symlink.sh\n> \n> diff --git a/t/t6036-merge-dir-to-symlink.sh b/t/t6036-merge-dir-to-symlink.sh\n> new file mode 100755\n> index 0000000..020db7c\n> --- /dev/null\n> +++ b/t/t6036-merge-dir-to-symlink.sh\n> @@ -0,0 +1,30 @@\n> +#!/bin/sh\n> +\n> +test_description='merging when a directory was replaced with a symlink'\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\tmkdir -p a/b/c a/b-2/c &&\n> +\t> a/b/c/d &&\n> +\t> a/b-2/c/d &&\n> +\t> a/x &&\n> +\tgit add -A &&\n> +\tgit commit -m base &&\n> +\trm -rf a/b-2 &&\n> +\tln -s b a/b-2 &&\n> +\tgit add -A &&\n> +\tgit commit -m \"dir to symlink\"\n> +'\n> +\n> +test_expect_failure 'checkout should not delete a/b/c/d' '\n> +\tgit checkout -b temp HEAD^ &&\n> +\ttest -f a/b/c/d\n> +'\n> +\n> +test_expect_failure 'merge should not have conflicts' '\n> +\techo x > a/x &&\n> +\tgit add a/x &&\n> +\tgit commit -m x &&\n> +\tgit merge master'\n> +\n> +test_done\n\nAs in 1/2, I think the first expect_failure leaves a dirty/unexpected\nworktree (d missing) which causes the merge failure in the last step.\n\nMichael\n"},{"id":"119033","messageId":"7v4osvyjl2.fsf@alter.siamese.dyndns.org","threadId":"20266","inReplyTo":"4A70062A.4040008@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-29T08:33:29Z","receivedAt":"2009-07-29T08:33:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> +test_expect_failure 'checkout should not delete a/b-2/c/d' '\n>> +\tgit checkout -b temp HEAD^ &&\n>> +\ttest -f a/b-2/c/d\n>> +'\n>> +\n>> +test_expect_failure 'merge should not delete a/b-2/c/d' '\n>> +\techo x > a/x &&\n>> +\tgit add a/x &&\n>> +\tgit commit -m x &&\n>> +\tgit merge master &&\n>> +\ttest -f a/b-2/c/d\n>> +'\n>> +\n>> +test_done\n>\n> Isn't the failure of the second test caused by that of the first one?\n> a/b-2/c/d is gone from the worktree, and master does not touch it, so\n> the merge leaves the worktree version (non-existent) as is.\n\nTo avoid that impression the second test should probably have been written\nto start from a clean slate, using \"reset --hard\" or something.\n\nKjetil's patch actually fixes the first one, but the second one will still\nshow breakage.\n\nI wonder if the breakage is in recursive merge or in the generic read-tree\nthree-way merge code. I highly suspect that using \"git merge -s resolve\"\nwould make the test pass. Historically recursive merge is known to be\ncareless in many corner cases.\n"},{"id":"119078","messageId":"3BA20DF9B35F384F8B7395B001EC3FB3424029A4@azsmsx507.amr.corp.intel.com","threadId":"20266","inReplyTo":"4A70086C.9070408@drmicha.warpmail.net","subject":"RE: [PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-29T16:39:52Z","receivedAt":"2009-07-29T16:39:52Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"On Wed, Jul 29, 2009, Michael J Gruber wrote:\n> In fact they are identical: Exchange b for b-2 and vice versa everywhere\n> and you get the same test, except for the fact that in 1/2 you \"test -f\"\n> in the last step. But I'm sure that test fails at the merge step already\n> (because of a dirty worktree), doesn't it? You should see this when\n> running the test with -d/-v. (I'm guessing, I haven't run your test.)\n\n\n\n> As in 1/2, I think the first expect_failure leaves a dirty/unexpected\n> worktree (d missing) which causes the merge failure in the last step.\n\nThat's true, but this test still fails even after applying Kjetil's patch\nthat fixes the first problem (so the work tree is clean before 'git merge'\nis run).\n\nJames\n"},{"id":"119082","messageId":"3BA20DF9B35F384F8B7395B001EC3FB342402A13@azsmsx507.amr.corp.intel.com","threadId":"20266","inReplyTo":"7v4osvyjl2.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-29T16:57:36Z","receivedAt":"2009-07-29T16:57:36Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"On Wed, Jul 29, 2009, Junio C Hamano wrote:\n> Michael J Gruber writes:\n>> Isn't the failure of the second test caused by that of the first one?\n>> a/b-2/c/d is gone from the worktree, and master does not touch it, so\n>> the merge leaves the worktree version (non-existent) as is.\n>\n> To avoid that impression the second test should probably have been written\n> to start from a clean slate, using \"reset --hard\" or something.\n\nI'll send a new patch shortly that combines the two tests into one and\nincludes the \"reset --hard\".\n\n> Kjetil's patch actually fixes the first one, but the second one will still\n> show breakage.\n>\n> I wonder if the breakage is in recursive merge or in the generic read-tree\n> three-way merge code. I highly suspect that using \"git merge -s resolve\"\n> would make the test pass. Historically recursive merge is known to be\n> careless in many corner cases.\n\nYou're right, using the resolve strategy does make the test pass.\n\nJames\n"},{"id":"119085","messageId":"3BA20DF9B35F384F8B7395B001EC3FB342402AD9@azsmsx507.amr.corp.intel.com","threadId":"20266","inReplyTo":"7v4osvyjl2.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-29T17:48:11Z","receivedAt":"2009-07-29T17:48:11Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"This test creates two directories, a/b and a/b-2, then replaces a/b with\na symlink to a/b-2, then merges that change into another branch that\ncontains an unrelated change.\n\nThere are two bugs:\n1. 'git checkout' wrongly deletes work tree file a/b-2/d.\n2. 'git merge' wrongly deletes work tree file a/b-2/d.\n\nThe test goes on to create another branch in which a/b-2 is replaced\nwith a symlink to a/b (i.e., the reverse of what was done the first\ntime), and merge it with the \"unrelated changes\" branch.\n\nThere's another bug:\n3. Since the changes are unrelated, the merge should be clean, but git\n reports a conflict.\n\nNote that using the resolve strategy instead of recursive makes the\nsecond bug go away, but not the third one.\n\nSigned-off-by: James Pickens \n---\nThis version combines the two tests into one, and cleans up between steps\nso that the early failures don't affect the later tests.\n\nThis time I include the bare minimum commands inside the\ntest_expect_failure calls, which seems like the right thing to do, since\nthe other commands are expected to \"succeed\" (exit code of 0).\n\nBTW I'm sending this patch using 'git format-patch' + Outlook instead of\n'git send-email'; apologies if it gets botched.\n\n t/t6035-merge-dir-to-symlink.sh | 49 +++++++++++++++++++++++++++++++++++++++\n 1 files changed, 49 insertions(+), 0 deletions(-)\n create mode 100755 t/t6035-merge-dir-to-symlink.sh\n\ndiff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\nnew file mode 100755\nindex 0000000..94a9f32\n--- /dev/null\n+++ b/t/t6035-merge-dir-to-symlink.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='merging when a directory was replaced with a symlink'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a merge where dir a/b changed to symlink' '\n+ mkdir -p a/b/c a/b-2/c &&\n+ > a/b/c/d &&\n+ > a/b-2/c/d &&\n+ > a/x &&\n+ git add -A &&\n+ git commit -m base &&\n+ rm -rf a/b &&\n+ ln -s b-2 a/b &&\n+ git add -A &&\n+ git commit -m \"dir to symlink\" &&\n+ git checkout -b temp HEAD^\n+'\n+\n+test_expect_failure 'checkout should not have deleted a/b-2/c/d' '\n+ test -f a/b-2/c/d\n+'\n+\n+test_expect_success 'clean the work tree and do the merge' '\n+ git reset --hard &&\n+ test -f a/b-2/c/d &&\n+ echo x > a/x &&\n+ git add a/x &&\n+ git commit -m x &&\n+ git merge master\n+'\n+\n+test_expect_failure 'merge should not have deleted a/b-2/c/d' '\n+ test -f a/b-2/c/d\n+'\n+\n+test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '\n+ git checkout -f -b temp2 master^ &&\n+ rm -rf a/b-2 &&\n+ ln -s b a/b-2 &&\n+ git add -A &&\n+ git commit -m \"dir a/b-2 to symlink\"\n+'\n+\n+test_expect_failure 'merge should not have conflicts' '\n+ git merge temp\n+'\n+\n+test_done\n--\n1.6.2.5\n"},{"id":"119087","messageId":"7v63dbuyru.fsf@alter.siamese.dyndns.org","threadId":"20266","inReplyTo":"3BA20DF9B35F384F8B7395B001EC3FB342402AD9@azsmsx507.amr.corp.intel.com","subject":"Re: [PATCH v2] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-29T18:31:17Z","receivedAt":"2009-07-29T18:31:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Pickens, James E\" writes:\n\n> This test creates two directories, a/b and a/b-2, then replaces a/b with\n> a symlink to a/b-2, then merges that change into another branch that\n> contains an unrelated change.\n\nThanks.\n\n> Note that using the resolve strategy instead of recursive makes the\n> second bug go away, but not the third one.\n\nIt is better to have separate tests for documentation purposes to help\npeople who track down the breakage in such a case.\n\n> +test_expect_failure 'checkout should not have deleted a/b-2/c/d' '\n> + test -f a/b-2/c/d\n> +'\n> +\n> +test_expect_success 'clean the work tree and do the merge' '\n> + git reset --hard &&\n> + test -f a/b-2/c/d &&\n> + echo x > a/x &&\n> + git add a/x &&\n> + git commit -m x &&\n> + git merge master\n> +'\n> +\n> +test_expect_failure 'merge should not have deleted a/b-2/c/d' '\n> + test -f a/b-2/c/d\n> +'\n\nSo...\n\n\ttest_expect_success 'setup for merge test' '\n \t...\n git commit -m x &&\n git tag baseline\n\t'\n\n\ttest_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '\n\t\tgit reset --hard &&\n \tgit checkout baseline^0 &&\n git merge -s resolve master\n\t'\n\n\ttest_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '\n\t\tgit reset --hard &&\n \tgit checkout baseline^0 &&\n git merge -s recursive master\n\t'\n\nLikewise for the other one.\n"},{"id":"119099","messageId":"3BA20DF9B35F384F8B7395B001EC3FB342402D3C@azsmsx507.amr.corp.intel.com","threadId":"20266","inReplyTo":"7v63dbuyru.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2009-07-29T21:02:39Z","receivedAt":"2009-07-29T21:02:39Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"This test creates two directories, a/b and a/b-2, then replaces a/b with\na symlink to a/b-2, then merges that change into the 'baseline' commit,\nwhich contains an unrelated change.\n\nThere are two bugs:\n1. 'git checkout' incorrectly deletes work tree file a/b-2/d.\n2. 'git merge' incorrectly deletes work tree file a/b-2/d.\n\nThe test goes on to create another branch in which a/b-2 is replaced\nwith a symlink to a/b (i.e., the reverse of what was done the first\ntime), and merge it into the 'baseline' commit.\n\nThere is a different bug:\n3. The merge should be clean, but git reports a conflict.\n\nSigned-off-by: James Pickens \n---\n\nOk, one more try. I added Junio's latest feedback, and also added more checks\nfor correct merge results after each merge. For the merges that incorrectly\nreport conflicts, those checks won't be executed since the conflict stops the\ntest. If/when the bug causing the merge conflict is fixed, it will become\nimportant to check the merge results, so those checks might as well be there\nfrom the beginning.\n\n t/t6035-merge-dir-to-symlink.sh | 76 +++++++++++++++++++++++++++++++++++++++\n 1 files changed, 76 insertions(+), 0 deletions(-)\n create mode 100755 t/t6035-merge-dir-to-symlink.sh\n\ndiff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh\nnew file mode 100755\nindex 0000000..89e8e6a\n--- /dev/null\n+++ b/t/t6035-merge-dir-to-symlink.sh\n@@ -0,0 +1,76 @@\n+#!/bin/sh\n+\n+test_description='merging when a directory was replaced with a symlink'\n+. ./test-lib.sh\n+\n+test_expect_success 'create a commit where dir a/b changed to symlink' '\n+ mkdir -p a/b/c a/b-2/c &&\n+ > a/b/c/d &&\n+ > a/b-2/c/d &&\n+ > a/x &&\n+ git add -A &&\n+ git commit -m base &&\n+ git tag start &&\n+ rm -rf a/b &&\n+ ln -s b-2 a/b &&\n+ git add -A &&\n+ git commit -m \"dir to symlink\" &&\n+ git checkout start^0\n+'\n+\n+test_expect_failure 'checkout should not have deleted a/b-2/c/d' '\n+ test -f a/b-2/c/d\n+'\n+\n+test_expect_success 'setup for merge test' '\n+ git reset --hard &&\n+ test -f a/b-2/c/d &&\n+ echo x > a/x &&\n+ git add a/x &&\n+ git commit -m x &&\n+ git tag baseline\n+'\n+\n+test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '\n+ git reset --hard &&\n+ git checkout baseline^0 &&\n+ git merge -s resolve master &&\n+ test -h a/b &&\n+ test -f a/b-2/c/d\n+'\n+\n+test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '\n+ git reset --hard &&\n+ git checkout baseline^0 &&\n+ git merge -s recursive master &&\n+ test -h a/b &&\n+ test -f a/b-2/c/d\n+'\n+\n+test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '\n+ git reset --hard &&\n+ git checkout start^0 &&\n+ rm -rf a/b-2 &&\n+ ln -s b a/b-2 &&\n+ git add -A &&\n+ git commit -m \"dir a/b-2 to symlink\" &&\n+ git tag test2\n+'\n+\n+test_expect_failure 'merge should not have conflicts (resolve)' '\n+ git reset --hard &&\n+ git checkout baseline^0 &&\n+ git merge -s resolve test2 &&\n+ test -h a/b-2 &&\n+ test -f a/b/c/d\n+'\n+\n+test_expect_failure 'merge should not have conflicts (recursive)' '\n+ git reset --hard &&\n+ git checkout baseline^0 &&\n+ git merge -s recursive test2 &&\n+ test -h a/b-2 &&\n+ test -f a/b/c/d\n+'\n+\n+test_done\n--\n1.6.2.5\n"},{"id":"119106","messageId":"alpine.LFD.2.01.0907291440480.3161@localhost.localdomain","threadId":"20266","inReplyTo":"3BA20DF9B35F384F8B7395B001EC3FB342402D3C@azsmsx507.amr.corp.intel.com","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-29T22:08:12Z","receivedAt":"2009-07-29T22:08:12Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 29 Jul 2009, Pickens, James E wrote:\n>\n> This test creates two directories, a/b and a/b-2, then replaces a/b with\n> a symlink to a/b-2, then merges that change into the 'baseline' commit,\n> which contains an unrelated change.\n\nGreat tests.\n\nThis patch should fix the 'checkout' issue.\n\nI made it use a new generic helper function (\"check_path()\"), since there \nare other cases like this that use just 'lstat()', and I bet we want to \nchange that.\n\nThe 'merge' issue is different, though: it's not due to a blind 'lstat()', \nbut due to a blind 'unlink()' done by 'remove_path()'. I think \n'remove_path()' should be taught to look for symlinks, and remove just the \nsymlink - but that's a bit more work, especially since the symlink cache \ndoesn't seem to expose any way to get the \"what is the first symlink path\" \ninformation.\n\nKjetil, can you look at that? \n\n\t\tLinus\n\n---\n cache.h | 3 +++\n entry.c | 15 ++++++++++++++-\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e6c7f33..9222774 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -468,6 +468,9 @@ extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_obje\n extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);\n extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n \n+/* \"careful lstat()\" */\n+extern int check_path(const char *path, int len, struct stat *st);\n+\n #define REFRESH_REALLY\t\t0x0001\t/* ignore_valid */\n #define REFRESH_UNMERGED\t0x0002\t/* allow unmerged */\n #define REFRESH_QUIET\t\t0x0004\t/* be quiet about it */\ndiff --git a/entry.c b/entry.c\nindex d3e86c7..f276cf3 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -175,6 +175,19 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \treturn 0;\n }\n \n+/*\n+ * This is like 'lstat()', except it refuses to follow symlinks\n+ * in the path.\n+ */\n+int check_path(const char *path, int len, struct stat *st)\n+{\n+\tif (has_symlink_leading_path(path, len)) {\n+\t\terrno = ENOENT;\n+\t\treturn -1;\n+\t}\n+\treturn lstat(path, st);\n+}\n+\n int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)\n {\n \tstatic char path[PATH_MAX + 1];\n@@ -188,7 +201,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \tstrcpy(path + len, ce->name);\n \tlen += ce_namelen(ce);\n \n-\tif (!lstat(path, &st)) {\n+\tif (!check_path(path, len, &st)) {\n \t\tunsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);\n \t\tif (!changed)\n \t\t\treturn 0;\n"},{"id":"119111","messageId":"7vprbjp0ra.fsf@alter.siamese.dyndns.org","threadId":"20266","inReplyTo":"alpine.LFD.2.01.0907291440480.3161@localhost.localdomain","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-29T22:44:57Z","receivedAt":"2009-07-29T22:44:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Wed, 29 Jul 2009, Pickens, James E wrote:\n>>\n>> This test creates two directories, a/b and a/b-2, then replaces a/b with\n>> a symlink to a/b-2, then merges that change into the 'baseline' commit,\n>> which contains an unrelated change.\n>\n> Great tests.\n>\n> This patch should fix the 'checkout' issue.\n\nThanks.\n\nI've queued v2 with local fixes and Kjetil's earlier fix in 'pu' that\nupdates has_symlink_leading_path() breakage. Will take a look at your\npatch (and v3 test) later.\n"},{"id":"119117","messageId":"86prbjm6uj.fsf@broadpark.no","threadId":"20266","inReplyTo":"alpine.LFD.2.01.0907291440480.3161@localhost.localdomain","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-07-29T23:01:40Z","receivedAt":"2009-07-29T23:01:40Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Linus Torvalds writes:\n\n> On Wed, 29 Jul 2009, Pickens, James E wrote:\n>>\n>> This test creates two directories, a/b and a/b-2, then replaces a/b with\n>> a symlink to a/b-2, then merges that change into the 'baseline' commit,\n>> which contains an unrelated change.\n>\n> Great tests.\n>\n> This patch should fix the 'checkout' issue.\n>\n> I made it use a new generic helper function (\"check_path()\"), since there \n> are other cases like this that use just 'lstat()', and I bet we want to \n> change that.\n>\n> The 'merge' issue is different, though: it's not due to a blind 'lstat()', \n> but due to a blind 'unlink()' done by 'remove_path()'. I think \n> 'remove_path()' should be taught to look for symlinks, and remove just the \n> symlink - but that's a bit more work, especially since the symlink cache \n> doesn't seem to expose any way to get the \"what is the first symlink path\" \n> information.\n>\n> Kjetil, can you look at that? \n\n Yes, I will take a look. Also, on all the other mails CC'ed to me\n today. Give me a cople of days.\n\n Sorry, I do not work at \"full normal speed\" for the moment. But, I\n will try to my best.\n\n -- kjetil\n"},{"id":"119121","messageId":"alpine.LFD.2.01.0907291656420.3161@localhost.localdomain","threadId":"20266","inReplyTo":"alpine.LFD.2.01.0907291440480.3161@localhost.localdomain","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-29T23:58:53Z","receivedAt":"2009-07-29T23:58:53Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 29 Jul 2009, Linus Torvalds wrote:\n>\n> The 'merge' issue is different, though: it's not due to a blind 'lstat()', \n> but due to a blind 'unlink()' done by 'remove_path()'. I think \n> 'remove_path()' should be taught to look for symlinks, and remove just the \n> symlink - but that's a bit more work, especially since the symlink cache \n> doesn't seem to expose any way to get the \"what is the first symlink path\" \n> information.\n> \n> Kjetil, can you look at that? \n\nHmm... This looks like it should do it.\n\nIt doesn't make the test _pass_ (we don't seem to be creating a/b-2/c/d \nproperly - I haven't checked why yet, but I suspect it is becasue we think \nit already exists due to the symlinked version lstat'ing fine), but it \nseems to do the right thing.\n\n\t\tLinus\n\n---\n dir.c | 20 --------------------\n symlinks.c | 26 ++++++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 20 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex e05b850..2204826 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -911,23 +911,3 @@ void setup_standard_excludes(struct dir_struct *dir)\n \tif (excludes_file && !access(excludes_file, R_OK))\n \t\tadd_excludes_from_file(dir, excludes_file);\n }\n-\n-int remove_path(const char *name)\n-{\n-\tchar *slash;\n-\n-\tif (unlink(name) && errno != ENOENT)\n-\t\treturn -1;\n-\n-\tslash = strrchr(name, '/');\n-\tif (slash) {\n-\t\tchar *dirs = xstrdup(name);\n-\t\tslash = dirs + (slash - name);\n-\t\tdo {\n-\t\t\t*slash = '\\0';\n-\t\t} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));\n-\t\tfree(dirs);\n-\t}\n-\treturn 0;\n-}\n-\ndiff --git a/symlinks.c b/symlinks.c\nindex 4bdded3..349c8d5 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -306,3 +306,29 @@ void remove_scheduled_dirs(void)\n {\n \tdo_remove_scheduled_dirs(0);\n }\n+\n+int remove_path(const char *name)\n+{\n+\tchar *slash;\n+\n+\t/*\n+\t * If we have a leading symlink, we remove\n+\t * just the symlink!\n+\t */\n+\tif (has_symlink_leading_path(name, strlen(name)))\n+\t\tname = default_cache.path;\n+\n+\tif (unlink(name) && errno != ENOENT)\n+\t\treturn -1;\n+\n+\tslash = strrchr(name, '/');\n+\tif (slash) {\n+\t\tchar *dirs = xstrdup(name);\n+\t\tslash = dirs + (slash - name);\n+\t\tdo {\n+\t\t\t*slash = '\\0';\n+\t\t} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));\n+\t\tfree(dirs);\n+\t}\n+\treturn 0;\n+}\n"},{"id":"119141","messageId":"alpine.LFD.2.01.0907291758030.3161@localhost.localdomain","threadId":"20266","inReplyTo":"alpine.LFD.2.01.0907291656420.3161@localhost.localdomain","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-30T01:06:39Z","receivedAt":"2009-07-30T01:06:39Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 29 Jul 2009, Linus Torvalds wrote:\n> \n> Hmm... This looks like it should do it.\n> \n> It doesn't make the test _pass_ (we don't seem to be creating a/b-2/c/d \n> properly - I haven't checked why yet, but I suspect it is becasue we think \n> it already exists due to the symlinked version lstat'ing fine), but it \n> seems to do the right thing.\n\nNever mind. The patch does what I set out to do, but it's not relevant for \nthe problem.\n\nWhat happens is:\n\n - we remove a/b/c/d to make room for the a/b symlink:\n\n\tmerge_trees ->\n\t git_merge_trees ->\n\t check_updates ->\n\t checkout_entry ->\n\t remove_subtree(\"a/b\") ->\n\t recursive rm\n\n This is correct\n\n - then we create the a/b symlink to a/b2\n\n\tmerge_trees ->\n\t git_merge_trees ->\n\t check_updates ->\n\t checkout_entry ->\n\t write_entry ->\n\t symlink\n\n This is correct\n\n - Then we remove 'a/b/c/d' again for the 'unmerged_cache()' case:\n\n\tmerge_trees ->\n\t process_entry ->\n\t remove_file\n\n because th eprocess_entry() will decide that the original tree had that \n \"a/b/c/d\" file (true) that needs to be removed (false - we already \n did that as part of creating \"a/b\").\n\nAnnoying.\n\n\t\tLinus\n"},{"id":"119147","messageId":"7vbpn2n958.fsf@alter.siamese.dyndns.org","threadId":"20266","inReplyTo":"alpine.LFD.2.01.0907291440480.3161@localhost.localdomain","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-30T03:26:43Z","receivedAt":"2009-07-30T03:26:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> This patch should fix the 'checkout' issue.\n>\n> I made it use a new generic helper function (\"check_path()\"), since there \n> are other cases like this that use just 'lstat()', and I bet we want to \n> change that.\n>\n> The 'merge' issue is different, though: it's not due to a blind 'lstat()', \n> but due to a blind 'unlink()' done by 'remove_path()'. I think \n> 'remove_path()' should be taught to look for symlinks, and remove just the \n> symlink - but that's a bit more work, especially since the symlink cache \n> doesn't seem to expose any way to get the \"what is the first symlink path\" \n> information.\n\nThis is a good thing to do, but the James's \"checkout\" test fails for an\nunrelated reason.\n\nThe tree has\n\n 120000 blob a36b773\ta/b\t\t-> b-2\n 100644 blob e69de29\ta/b-2/c/d\n 100644 blob e69de29\ta/x\n\nchecked out, and wants to switch to \n\n 100644 blob e69de29\ta/b-2/c/d\n 100644 blob e69de29\ta/b/c/d\n 100644 blob e69de29\ta/x\n\ncheckout_entry() is called to check out \"a/b/c/d\". If \"a/b\" symlink\nwere still there, the lstat() you fixed will be fooled.\n\nBut in James's test, because the symlink \"a/b\" is tracked in the\nswitched-from commit and is being obliterated by switching to a tree that\nhas a directory there, we (should) have called deleted_entry() on a/b to\nmark it for removal, and inside check_updates() before going into the loop\nto call checkout_entry(), we would have already removed the symlink \"a/b\"\nthat is going away inside unlink_entry().\n\nThe problem is that has_symlink_or_noent_leading_path() called from there\nlies, without Kjetil's fix c52dc70 (lstat_cache: guard against full match\nof length of 'name' parameter, 2009-06-14) that is in 'pu'.\n\nIf the original tree in the test did not have \"a/b\" tracked, but has an\nuntracked symlink \"a/b\" that points at b-2, then \"a/b\" will stay in the\nwork tree when the codepath your patch touches is reached, and the problem\nwill be demonstrated. Your patch will fix that issue.\n\nSo both fixes are necessary, and we need a separate test to illustrate\nwhat your patch fixes.\n\nI'll push out some updates to do so.\n"},{"id":"119151","messageId":"7vfxceln8t.fsf@alter.siamese.dyndns.org","threadId":"20266","inReplyTo":"alpine.LFD.2.01.0907291440480.3161@localhost.localdomain","subject":"Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-30T06:05:06Z","receivedAt":"2009-07-30T06:05:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> This patch should fix the 'checkout' issue.\n>\n> I made it use a new generic helper function (\"check_path()\"), since there \n> are other cases like this that use just 'lstat()', and I bet we want to \n> change that.\n>\n> The 'merge' issue is different, though: it's not due to a blind 'lstat()', \n> but due to a blind 'unlink()' done by 'remove_path()'. I think \n> 'remove_path()' should be taught to look for symlinks, and remove just the \n> symlink - but that's a bit more work, especially since the symlink cache \n> doesn't seem to expose any way to get the \"what is the first symlink path\" \n> information.\n\nI think this is a good thing to do regardless, but the James's \"checkout\"\ntest fails for an unrelated reason.\n\nThe tree has\n\n 120000 blob a36b773\ta/b\t\t-> b-2\n 100644 blob e69de29\ta/b-2/c/d\n 100644 blob e69de29\ta/x\n\nchecked out, and wants to switch to \n\n 100644 blob e69de29\ta/b-2/c/d\n 100644 blob e69de29\ta/b/c/d\n 100644 blob e69de29\ta/x\n\ncheckout_entry() is called to check out \"a/b/c/d\". If \"a/b\" symlink\nwere still there, the lstat() you fixed will be fooled.\n\nBut in James's test, because the symlink \"a/b\" is tracked in the\nswitched-from commit and is being obliterated by switching to a tree that\nhas a directory there, we (should) have called deleted_entry() on a/b to\nmark it for removal, and inside check_updates() before going into the loop\nto call checkout_entry(), we would have already removed the symlink \"a/b\"\nthat is going away inside unlink_entry().\n\nThe problem is that has_symlink_or_noent_leading_path() called from there\nlies, without Kjetil's fix c52dc70 (lstat_cache: guard against full match\nof length of 'name' parameter, 2009-06-14) that is in 'pu'.\n\nIf the original tree in the test did not have \"a/b\" tracked, but has an\nuntracked symlink \"a/b\" that points at b-2, then \"a/b\" will stay in the\nwork tree when the codepath your patch touches is reached, and the problem\nwill be demonstrated. Your patch will fix that issue.\n\nSo both fixes are necessary, and we need a separate test to illustrate\nwhat your patch fixes.\n\nI'll push out some updates.\n"}]}