Re: [PATCH v2 1/3] stash bug: stash can lose data in a file removed from the index

Previous thread: none

Next thread: [PATCH v2 2/3] stash: Don't overwrite files that have gone from the index by Charles Bailey on Sunday, April 18, 2010 - 11:28 am. (2 messages)
From: Charles Bailey
Date: Sunday, April 18, 2010 - 11:27 am

If a file is removed from the index and then modified in the working
tree then stash will discard the working tree file with no way to
recover the changes.

This can might be done in one of a number of ways.

git rm file
vi file              # edit a new version
git stash

or with git mv

git mv file newfile
vi file              # make a new file with the old name
git stash

Signed-off-by: Charles Bailey <charles@hashpling.org>
---

Since the last version of this series I've added a number of new cases.
Some worked before but were broken by my last patch. Others highlighted
breakages that I was only in development.

The last two indicate problems that we have when trying to stash changes
where either a path changed from being a tracked directory to a file in
the work tree or was a tracked file and became a directory in the work
tree.

In both circumstances we can still permanently lose user data although
this patch series reduces the number of situations where this can
happen.

 t/t3903-stash.sh |  144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 144 insertions(+), 0 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 476e5ec..bb026aa 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -228,4 +228,148 @@ test_expect_success 'stash --invalid-option' '
 	test bar,bar2 = $(cat file),$(cat file2)
 '
 
+test_expect_success 'stash rm then recreate' '
+	git reset --hard &&
+	git rm file &&
+	echo bar7 > file &&
+	git stash save "rm then recreate" &&
+	test bar = $(cat file) &&
+	git stash apply &&
+	test bar7 = $(cat file)
+'
+
+test_expect_success 'stash rm and ignore' '
+	git reset --hard &&
+	git rm file &&
+	echo file > .gitignore &&
+	git stash save "rm and ignore" &&
+	test bar = "$(cat file)" &&
+	test file = "$(cat .gitignore)"
+	git stash apply &&
+	! test -r file &&
+	test file = "$(cat .gitignore)"
+'
+
+test_expect_success 'stash rm and ignore (stage .gitignore)' '
+	git reset --hard ...
From: Junio C Hamano
Date: Sunday, April 18, 2010 - 4:11 pm

It is likely that this needs to be protected with SYMLINKS prerequisite.
Also I am a bit unhappy about the use of "readlink" which is not even in
POSIX.1 here.  We already have one use of it in the tests but that only
happens while doing valgrind.  Traditionally this has been more portably
done by reading from "ls -l file", like so:

	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac

Also, whether "readlink file" or "ls -l file" is used to check the result,

I have a feeling that this test is being a bit unfair.

What should a successful invocation of "stash apply" leave in the working
tree in this case, especially when you consider that in a real life use
case you may have other files in "dir" directory or changes to "dir/file"?
--

From: Charles Bailey
Date: Monday, April 19, 2010 - 12:45 am

I don't have access to many flavours of unix at the moment and I
failed to look up the POSIXness of readlink. I'll re-roll with greater

Yes, of course. Not sure what I was thinking.

What about "test -h"? Is this sufficiently portable
for use in our tests? I understand that it's supposed to be POSIX and

Actually I now think that this is completely wrong. There's no reason
that stash apply shouldn't succeed. If we managed to save the new file
where the directory was in the stash then why shouldn't apply be able
to at least attempt to remove the tracked files in the directory that
were originally removed and replace them with the stashed file?

Even if we decide that it can't or shouldn't, we should expect a
failing stash apply to leave the tree as it currently is. That does
leave the question of how the user is supposed to get stuff out of his
stash. After all, he's trying to apply the stash on exactly the state
that stash left him in.

Is it sensible to be guided by these two principles: git stash should
be safe, i.e. it should never remove content that it doesn't save in
the database. git stash && git stash apply should leave the working
tree exactly as it was before the git stash invocation (if the stash
succeeds it may be equivalent to a git reset)?

If so we definitely need to fix the behaviour where git stash
vaporizes local changes when there's a file <-> directory change
in the working tree. Even if we cop out and make git stash fail if it
determines that it wouldn't restore the changes.

Charles.
--

Previous thread: none

Next thread: [PATCH v2 2/3] stash: Don't overwrite files that have gone from the index by Charles Bailey on Sunday, April 18, 2010 - 11:28 am. (2 messages)