threads / patch / 23514

v2, 3 partsstash bug: stash can lose data in a file removed from the index

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

## tl;dr

3 messages between Apr 18, 2010 and Apr 19, 2010. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Charles Bailey· Apr 18, 2010, 18:27 UTC · lore

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(-)
Show changes to t/t3903-stash.sh +144 −0
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 &&
+	git rm file &&
+	echo file > .gitignore &&
+	git add .gitignore &&
+	git stash save "rm and ignore (stage .gitignore)" &&
+	test bar = "$(cat file)" &&
+	! test -r .gitignore
+	git stash apply &&
+	! test -r file &&
+	test file = "$(cat .gitignore)"
+'
+
+test_expect_success 'stash file to symlink' '
+	git reset --hard &&
+	rm file &&
+	ln -s file2 file &&
+	git stash save "file to symlink" &&
+	test -f file &&
+	test bar = "$(cat file)" &&
+	git stash apply &&
+	test -f file &&
+	test file2 = "$(readlink file)"
+'
+
+test_expect_success 'stash file to symlink (stage rm)' '
+	git reset --hard &&
+	git rm file &&
+	ln -s file2 file &&
+	git stash save "file to symlink (stage rm)" &&
+	test -f file &&
+	test bar = "$(cat file)" &&
+	git stash apply &&
+	test -f file &&
+	test file2 = "$(readlink file)"
+'
+
+test_expect_success 'stash file to symlink (full stage)' '
+	git reset --hard &&
+	rm file &&
+	ln -s file2 file &&
+	git add file &&
+	git stash save "file to symlink (full stage)" &&
+	test -f file &&
+	test bar = "$(cat file)" &&
+	git stash apply &&
+	test -f file &&
+	test file2 = "$(readlink file)"
+'
+
+# This test creates a commit with a symlink used for the following tests
+
+test_expect_success 'stash symlink to file' '
+	git reset --hard &&
+	ln -s file filelink &&
+	git add filelink &&
+	git commit -m "Add symlink" &&
+	rm filelink &&
+	cp file filelink &&
+	git stash save "symlink to file" &&
+	test -h filelink &&
+	test file = "$(readlink filelink)" &&
+	git stash apply &&
+	! test -h filelink &&
+	test bar = "$(cat file)"
+'
+
+test_expect_success 'stash symlink to file (stage rm)' '
+	git reset --hard &&
+	git rm filelink &&
+	cp file filelink &&
+	git stash save "symlink to file (stage rm)" &&
+	test -h filelink &&
+	test file = "$(readlink filelink)" &&
+	git stash apply &&
+	! test -h filelink &&
+	test bar = "$(cat file)"
+'
+
+test_expect_success 'stash symlink to file (full stage)' '
+	git reset --hard &&
+	rm filelink &&
+	cp file filelink &&
+	git add filelink &&
+	git stash save "symlink to file (full stage)" &&
+	test -h filelink &&
+	test file = "$(readlink filelink)" &&
+	git stash apply &&
+	! test -h filelink &&
+	test bar = "$(cat file)"
+'
+
+test_expect_failure 'stash directory to file' '
+	git reset --hard &&
+	mkdir dir &&
+	echo foo >dir/file &&
+	git add dir/file &&
+	git commit -m "Add file in dir" &&
+	rm dir/file &&
+	rmdir dir &&
+	echo bar >dir &&
+	git stash save "directory to file" &&
+	test -d dir &&
+	test foo = "$(cat dir/file)" &&
+	test_must_fail git stash apply &&
+	test bar = "$(cat dir)" &&
+	git reset --soft HEAD^
+'
+
+test_expect_failure 'stash file to directory' '
+	git reset --hard &&
+	rm file &&
+	mkdir file &&
+	echo foo >file/file &&
+	git stash save "file to directory" &&
+	test -f file &&
+	test bar = "$(cat file)" &&
+	git stash apply &&
+	test -f file/file &&
+	test foo = "$(cat file/file)"
+'
+
 test_done

-- 
1.7.1.rc1.241.g4e72f
Junio C Hamano· Apr 18, 2010, 23:11 UTC · re: Charles Bailey · lore

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

Charles Bailey <charles@hashpling.org> writes:
Show 11 quoted lines
> +test_expect_success 'stash file to symlink' '
> +	git reset --hard &&
> +	rm file &&
> +	ln -s file2 file &&
> +	git stash save "file to symlink" &&
> +	test -f file &&
> +	test bar = "$(cat file)" &&
> +	git stash apply &&
> +	test -f file &&
> +	test file2 = "$(readlink file)"
> +'

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, the "test -f file" is redundant.

Show 16 quoted lines
> +test_expect_failure 'stash directory to file' '
> +	git reset --hard &&
> +	mkdir dir &&
> +	echo foo >dir/file &&
> +	git add dir/file &&
> +	git commit -m "Add file in dir" &&
> +	rm dir/file &&
> +	rmdir dir &&
> +	echo bar >dir &&
> +	git stash save "directory to file" &&
> +	test -d dir &&
> +	test foo = "$(cat dir/file)" &&
> +	test_must_fail git stash apply &&
> +	test bar = "$(cat dir)" &&
> +	git reset --soft HEAD^
> +'
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"?

Charles Bailey· Apr 19, 2010, 07:45 UTC · re: Junio C Hamano · lore

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

On Sun, Apr 18, 2010 at 04:11:15PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> Charles Bailey <charles@hashpling.org> writes:
> 
> 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

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 portability.

> Also, whether "readlink file" or "ls -l file" is used to check the result,
> the "test -f file" is redundant.
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 available on Solaris sh.

Show 23 quoted lines
> 
> > +test_expect_failure 'stash directory to file' '
> > +	git reset --hard &&
> > +	mkdir dir &&
> > +	echo foo >dir/file &&
> > +	git add dir/file &&
> > +	git commit -m "Add file in dir" &&
> > +	rm dir/file &&
> > +	rmdir dir &&
> > +	echo bar >dir &&
> > +	git stash save "directory to file" &&
> > +	test -d dir &&
> > +	test foo = "$(cat dir/file)" &&
> > +	test_must_fail git stash apply &&
> > +	test bar = "$(cat dir)" &&
> > +	git reset --soft HEAD^
> > +'
> 
> 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"?

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.

← back to recent threads