{"thread":{"id":"23514","subject":"[PATCH v2 1/3] stash bug: stash can lose data in a file removed from the index","startedAt":"2010-04-18T18:27:49Z","lastAt":"2010-04-19T07:45:20Z","messageCount":3,"participants":["Charles Bailey","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"139827","messageId":"20100418182749.GA29329@hashpling.org","threadId":"23514","inReplyTo":null,"subject":"[PATCH v2 1/3] stash bug: stash can lose data in a file removed from the index","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-04-18T18:27:49Z","receivedAt":"2010-04-18T18:27:49Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"If a file is removed from the index and then modified in the working\ntree then stash will discard the working tree file with no way to\nrecover the changes.\n\nThis can might be done in one of a number of ways.\n\ngit rm file\nvi file              # edit a new version\ngit stash\n\nor with git mv\n\ngit mv file newfile\nvi file              # make a new file with the old name\ngit stash\n\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n\nSince the last version of this series I've added a number of new cases.\nSome worked before but were broken by my last patch. Others highlighted\nbreakages that I was only in development.\n\nThe last two indicate problems that we have when trying to stash changes\nwhere either a path changed from being a tracked directory to a file in\nthe work tree or was a tracked file and became a directory in the work\ntree.\n\nIn both circumstances we can still permanently lose user data although\nthis patch series reduces the number of situations where this can\nhappen.\n\n t/t3903-stash.sh |  144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 144 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 476e5ec..bb026aa 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -228,4 +228,148 @@ test_expect_success 'stash --invalid-option' '\n \ttest bar,bar2 = $(cat file),$(cat file2)\n '\n \n+test_expect_success 'stash rm then recreate' '\n+\tgit reset --hard &&\n+\tgit rm file &&\n+\techo bar7 > file &&\n+\tgit stash save \"rm then recreate\" &&\n+\ttest bar = $(cat file) &&\n+\tgit stash apply &&\n+\ttest bar7 = $(cat file)\n+'\n+\n+test_expect_success 'stash rm and ignore' '\n+\tgit reset --hard &&\n+\tgit rm file &&\n+\techo file > .gitignore &&\n+\tgit stash save \"rm and ignore\" &&\n+\ttest bar = \"$(cat file)\" &&\n+\ttest file = \"$(cat .gitignore)\"\n+\tgit stash apply &&\n+\t! test -r file &&\n+\ttest file = \"$(cat .gitignore)\"\n+'\n+\n+test_expect_success 'stash rm and ignore (stage .gitignore)' '\n+\tgit reset --hard &&\n+\tgit rm file &&\n+\techo file > .gitignore &&\n+\tgit add .gitignore &&\n+\tgit stash save \"rm and ignore (stage .gitignore)\" &&\n+\ttest bar = \"$(cat file)\" &&\n+\t! test -r .gitignore\n+\tgit stash apply &&\n+\t! test -r file &&\n+\ttest file = \"$(cat .gitignore)\"\n+'\n+\n+test_expect_success 'stash file to symlink' '\n+\tgit reset --hard &&\n+\trm file &&\n+\tln -s file2 file &&\n+\tgit stash save \"file to symlink\" &&\n+\ttest -f file &&\n+\ttest bar = \"$(cat file)\" &&\n+\tgit stash apply &&\n+\ttest -f file &&\n+\ttest file2 = \"$(readlink file)\"\n+'\n+\n+test_expect_success 'stash file to symlink (stage rm)' '\n+\tgit reset --hard &&\n+\tgit rm file &&\n+\tln -s file2 file &&\n+\tgit stash save \"file to symlink (stage rm)\" &&\n+\ttest -f file &&\n+\ttest bar = \"$(cat file)\" &&\n+\tgit stash apply &&\n+\ttest -f file &&\n+\ttest file2 = \"$(readlink file)\"\n+'\n+\n+test_expect_success 'stash file to symlink (full stage)' '\n+\tgit reset --hard &&\n+\trm file &&\n+\tln -s file2 file &&\n+\tgit add file &&\n+\tgit stash save \"file to symlink (full stage)\" &&\n+\ttest -f file &&\n+\ttest bar = \"$(cat file)\" &&\n+\tgit stash apply &&\n+\ttest -f file &&\n+\ttest file2 = \"$(readlink file)\"\n+'\n+\n+# This test creates a commit with a symlink used for the following tests\n+\n+test_expect_success 'stash symlink to file' '\n+\tgit reset --hard &&\n+\tln -s file filelink &&\n+\tgit add filelink &&\n+\tgit commit -m \"Add symlink\" &&\n+\trm filelink &&\n+\tcp file filelink &&\n+\tgit stash save \"symlink to file\" &&\n+\ttest -h filelink &&\n+\ttest file = \"$(readlink filelink)\" &&\n+\tgit stash apply &&\n+\t! test -h filelink &&\n+\ttest bar = \"$(cat file)\"\n+'\n+\n+test_expect_success 'stash symlink to file (stage rm)' '\n+\tgit reset --hard &&\n+\tgit rm filelink &&\n+\tcp file filelink &&\n+\tgit stash save \"symlink to file (stage rm)\" &&\n+\ttest -h filelink &&\n+\ttest file = \"$(readlink filelink)\" &&\n+\tgit stash apply &&\n+\t! test -h filelink &&\n+\ttest bar = \"$(cat file)\"\n+'\n+\n+test_expect_success 'stash symlink to file (full stage)' '\n+\tgit reset --hard &&\n+\trm filelink &&\n+\tcp file filelink &&\n+\tgit add filelink &&\n+\tgit stash save \"symlink to file (full stage)\" &&\n+\ttest -h filelink &&\n+\ttest file = \"$(readlink filelink)\" &&\n+\tgit stash apply &&\n+\t! test -h filelink &&\n+\ttest bar = \"$(cat file)\"\n+'\n+\n+test_expect_failure 'stash directory to file' '\n+\tgit reset --hard &&\n+\tmkdir dir &&\n+\techo foo >dir/file &&\n+\tgit add dir/file &&\n+\tgit commit -m \"Add file in dir\" &&\n+\trm dir/file &&\n+\trmdir dir &&\n+\techo bar >dir &&\n+\tgit stash save \"directory to file\" &&\n+\ttest -d dir &&\n+\ttest foo = \"$(cat dir/file)\" &&\n+\ttest_must_fail git stash apply &&\n+\ttest bar = \"$(cat dir)\" &&\n+\tgit reset --soft HEAD^\n+'\n+\n+test_expect_failure 'stash file to directory' '\n+\tgit reset --hard &&\n+\trm file &&\n+\tmkdir file &&\n+\techo foo >file/file &&\n+\tgit stash save \"file to directory\" &&\n+\ttest -f file &&\n+\ttest bar = \"$(cat file)\" &&\n+\tgit stash apply &&\n+\ttest -f file/file &&\n+\ttest foo = \"$(cat file/file)\"\n+'\n+\n test_done\n-- \n1.7.1.rc1.241.g4e72f\n"},{"id":"139856","messageId":"7vvdbotkz0.fsf@alter.siamese.dyndns.org","threadId":"23514","inReplyTo":"20100418182749.GA29329@hashpling.org","subject":"Re: [PATCH v2 1/3] stash bug: stash can lose data in a file removed from the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-18T23:11:15Z","receivedAt":"2010-04-18T23:11:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> +test_expect_success 'stash file to symlink' '\n> +\tgit reset --hard &&\n> +\trm file &&\n> +\tln -s file2 file &&\n> +\tgit stash save \"file to symlink\" &&\n> +\ttest -f file &&\n> +\ttest bar = \"$(cat file)\" &&\n> +\tgit stash apply &&\n> +\ttest -f file &&\n> +\ttest file2 = \"$(readlink file)\"\n> +'\n\nIt is likely that this needs to be protected with SYMLINKS prerequisite.\nAlso I am a bit unhappy about the use of \"readlink\" which is not even in\nPOSIX.1 here.  We already have one use of it in the tests but that only\nhappens while doing valgrind.  Traditionally this has been more portably\ndone by reading from \"ls -l file\", like so:\n\n\tcase \"$(ls -l file)\" in *\" file -> file2\") :;; *) false;; esac\n\nAlso, whether \"readlink file\" or \"ls -l file\" is used to check the result,\nthe \"test -f file\" is redundant.\n\n> +test_expect_failure 'stash directory to file' '\n> +\tgit reset --hard &&\n> +\tmkdir dir &&\n> +\techo foo >dir/file &&\n> +\tgit add dir/file &&\n> +\tgit commit -m \"Add file in dir\" &&\n> +\trm dir/file &&\n> +\trmdir dir &&\n> +\techo bar >dir &&\n> +\tgit stash save \"directory to file\" &&\n> +\ttest -d dir &&\n> +\ttest foo = \"$(cat dir/file)\" &&\n> +\ttest_must_fail git stash apply &&\n> +\ttest bar = \"$(cat dir)\" &&\n> +\tgit reset --soft HEAD^\n> +'\n\nI have a feeling that this test is being a bit unfair.\n\nWhat should a successful invocation of \"stash apply\" leave in the working\ntree in this case, especially when you consider that in a real life use\ncase you may have other files in \"dir\" directory or changes to \"dir/file\"?\n"},{"id":"139874","messageId":"20100419074520.GA5513@hashpling.org","threadId":"23514","inReplyTo":"7vvdbotkz0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3] stash bug: stash can lose data in a file removed from the index","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-04-19T07:45:20Z","receivedAt":"2010-04-19T07:45:20Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Apr 18, 2010 at 04:11:15PM -0700, Junio C Hamano wrote:\n> Charles Bailey <charles@hashpling.org> writes:\n> \n> It is likely that this needs to be protected with SYMLINKS prerequisite.\n> Also I am a bit unhappy about the use of \"readlink\" which is not even in\n> POSIX.1 here.  We already have one use of it in the tests but that only\n> happens while doing valgrind.  Traditionally this has been more portably\n> done by reading from \"ls -l file\", like so:\n> \n> \tcase \"$(ls -l file)\" in *\" file -> file2\") :;; *) false;; esac\n\nI don't have access to many flavours of unix at the moment and I\nfailed to look up the POSIXness of readlink. I'll re-roll with greater\nportability.\n\n> Also, whether \"readlink file\" or \"ls -l file\" is used to check the result,\n> the \"test -f file\" is redundant.\n\nYes, of course. Not sure what I was thinking.\n\nWhat about \"test -h\"? Is this sufficiently portable\nfor use in our tests? I understand that it's supposed to be POSIX and\navailable on Solaris sh.\n\n> \n> > +test_expect_failure 'stash directory to file' '\n> > +\tgit reset --hard &&\n> > +\tmkdir dir &&\n> > +\techo foo >dir/file &&\n> > +\tgit add dir/file &&\n> > +\tgit commit -m \"Add file in dir\" &&\n> > +\trm dir/file &&\n> > +\trmdir dir &&\n> > +\techo bar >dir &&\n> > +\tgit stash save \"directory to file\" &&\n> > +\ttest -d dir &&\n> > +\ttest foo = \"$(cat dir/file)\" &&\n> > +\ttest_must_fail git stash apply &&\n> > +\ttest bar = \"$(cat dir)\" &&\n> > +\tgit reset --soft HEAD^\n> > +'\n> \n> I have a feeling that this test is being a bit unfair.\n> \n> What should a successful invocation of \"stash apply\" leave in the working\n> tree in this case, especially when you consider that in a real life use\n> case you may have other files in \"dir\" directory or changes to \"dir/file\"?\n\nActually I now think that this is completely wrong. There's no reason\nthat stash apply shouldn't succeed. If we managed to save the new file\nwhere the directory was in the stash then why shouldn't apply be able\nto at least attempt to remove the tracked files in the directory that\nwere originally removed and replace them with the stashed file?\n\nEven if we decide that it can't or shouldn't, we should expect a\nfailing stash apply to leave the tree as it currently is. That does\nleave the question of how the user is supposed to get stuff out of his\nstash. After all, he's trying to apply the stash on exactly the state\nthat stash left him in.\n\nIs it sensible to be guided by these two principles: git stash should\nbe safe, i.e. it should never remove content that it doesn't save in\nthe database. git stash && git stash apply should leave the working\ntree exactly as it was before the git stash invocation (if the stash\nsucceeds it may be equivalent to a git reset)?\n\nIf so we definitely need to fix the behaviour where git stash\nvaporizes local changes when there's a file <-> directory change\nin the working tree. Even if we cop out and make git stash fail if it\ndetermines that it wouldn't restore the changes.\n\nCharles.\n"}]}