Re: [PATCH] git stash: Avoid data loss when saving a stash

[Junio C Hamano <gitster@pobox.com>]

Petr Baudis <pasky@ucw.cz> writes:

> Signed-off-by: Petr Baudis <pasky@ucw.cz>
> ---
>
> Please Cc me, I'm currently not subscribed on the list.

Heh, long time no see.

> @@ -71,6 +72,13 @@ linkgit:git-add[1] to learn how to operate the `--patch` mode.
>  +
>  The `--patch` option implies `--keep-index`.  You can use
>  `--no-keep-index` to override this.
> ++
> +In some cases, saving a stash could mean irretrievably removing some
> +data - if a directory with untracked files replaces a tracked file of
> +the same name, the new untracked files are not saved (except in case
> +of `--include-untracked`) but the original tracked file shall be restored.
> +Normally, stash save will abort; `--force` will make it remove the
> +untracked files.

It _might_ look obvious from the context after somebody spends
enough time thinking about this case (and only this case) that
"in such a case" is implied in "Normally, ... will abort", but for a
casual reader who encounters this paragraph for the first time, I do
not think it is obvious.  I'd rephrase to:

    By default, `stash save` will abort in such a case; `--force` will
    allow it to remove the untracked files.

> @@ -258,6 +262,12 @@ save_stash () {
>  		say "$(gettext "No local changes to save")"
>  		exit 0
>  	fi
> +	if test -z "$untracked$force" -a -n "$(git ls-files --killed | head -n 1)"; then

Split the line at the semicolon in "; then".  Also "git grep" will
tell us that we tend to avoid "-a" in "test".

        if test -z "$untracked$force" &&
           test -n "$(git ls-files --killed | head -n 1)"
        then
		...

> +		say "$(gettext "The following untracked files would NOT be saved but need to be removed by stash save:")"
> +		test -n "$GIT_QUIET" || git ls-files --killed | sed 's/^/\t/'
> +		say "$(gettext "Abording. Consider using either the --force or --include-untracked switches.")" >&2

s/Abord/Abort/;

> +		exit 1
> +	fi
>  	test -f "$GIT_DIR/logs/$ref_stash" ||
>  		clear_stash || die "$(gettext "Cannot initialize stash")"
>  
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index debda7a..4ac4ebe 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -673,4 +673,19 @@ test_expect_success 'store updates stash ref and reflog' '
>  	grep quux bazzy
>  '
>  
> +test_expect_success SYMLINKS 'stash symlink to non-empty directory' '
> +	git reset --hard &&
> +	ln -s file2 linkdir &&
> +	git add linkdir &&
> +	git commit -m"+linkdir as symlink" &&
> +	rm linkdir && mkdir linkdir && touch linkdir/file &&

Use ">linkdir/file" instead for clarity, when you are not interested
in modifying the timestamp of an existing file.

> +	! git stash save "symlink to non-empty directory" &&

Use test_must_fail

> +	[ -e linkdir/file ]

"test -f linkdir/file"  You not only want to see it exists, you know
it must be a regular file.

> +'
> +
> +test_expect_success SYMLINKS 'stash symlink to non-empty directory (forced)' '
> +	git stash save --force "symlink to non-empty directory (forced)" &&
> +	[ ! -e linkdir/file ] && [ -L linkdir ]
> +'

"git grep" will tell us that "test -h" is preferred over "test -L"
in our codebase.

> +
>  test_done

Also I do not think you need to limit the tests on symlink-capable
platforms.  You can create the "linkdir" as a regular file and
commit, and make a local change to turn it into a directory, and try
to "stash save" to recover that original regular file.

Thanks.  I'll queue it with a pair of fix-up commits on top, so that
they can later be squashed in.

The result of squashing the fix-ups would look like this.

-- >8 --
From: Petr Baudis <pasky@ucw.cz>
Date: Fri, 28 Jun 2013 17:05:32 +0200
Subject: [PATCH] git stash: avoid data loss when "git stash save" kills a directory

"stash save" is about saving the local change to the working tree,
but also about restoring the state of the last commit to the working
tree.  When a local change is to turn a non-directory to a directory,
in order to restore the non-directory, everything in the directory
needs to be removed.

Which is fine when running "git stash save --include-untracked",
but without that option, untracked, newly created files in the
directory will have to be discarded.

Introduce a safety valve to fail the operation in such case, using
the "ls-files --killed" which was designed for this exact purpose.

The "stash save" is stopped when untracked files need to be
discarded because their leading path ceased to be a directory, and
the user is required to pass --force to really have the data
removed.

Signed-off-by: Petr Baudis <pasky@ucw.cz>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-stash.txt | 12 ++++++++++--
 git-stash.sh                | 12 ++++++++++++
 t/t3903-stash.sh            | 18 ++++++++++++++++++
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index db7e803..7c8b648 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -14,7 +14,8 @@ SYNOPSIS
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-	     [-u|--include-untracked] [-a|--all] [<message>]]
+	     [-u|--include-untracked] [-a|--all] [-f|--force]
+	     [<message>]]
 'git stash' clear
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -44,7 +45,7 @@ is also possible).
 OPTIONS
 -------
 
-save [-p|--patch] [--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+save [-p|--patch] [--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-f|--force] [<message>]::
 
 	Save your local modifications to a new 'stash', and run `git reset
 	--hard` to revert them.  The <message> part is optional and gives
@@ -71,6 +72,13 @@ linkgit:git-add[1] to learn how to operate the `--patch` mode.
 +
 The `--patch` option implies `--keep-index`.  You can use
 `--no-keep-index` to override this.
++
+In some cases, saving a stash could mean irretrievably removing some
+data - if a directory with untracked files replaces a tracked file of
+the same name, the new untracked files are not saved (except in case
+of `--include-untracked`) but the original tracked file shall be restored.
+By default, `stash save` will abort in such a case; `--force` will allow
+it to remove the untracked files.
 
 list [<options>]::
 
diff --git a/git-stash.sh b/git-stash.sh
index 1e541a2..85c9e2c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -195,6 +195,7 @@ save_stash () {
 	keep_index=
 	patch_mode=
 	untracked=
+	force=
 	while test $# != 0
 	do
 		case "$1" in
@@ -215,6 +216,9 @@ save_stash () {
 		-u|--include-untracked)
 			untracked=untracked
 			;;
+		-f|--force)
+			force=t
+			;;
 		-a|--all)
 			untracked=all
 			;;
@@ -258,6 +262,14 @@ save_stash () {
 		say "$(gettext "No local changes to save")"
 		exit 0
 	fi
+	if test -z "$untracked$force" &&
+	   test -n "$(git ls-files --killed | head -n 1)"
+	then
+		say "$(gettext "The following untracked files would NOT be saved but need to be removed by stash save:")"
+		test -n "$GIT_QUIET" || git ls-files --killed | sed 's/^/\t/'
+		say "$(gettext "Aborting. Consider using either the --force or --include-untracked option.")" >&2
+		exit 1
+	fi
 	test -f "$GIT_DIR/logs/$ref_stash" ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index debda7a..5d22f17 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -673,4 +673,22 @@ test_expect_success 'store updates stash ref and reflog' '
 	grep quux bazzy
 '
 
+test_expect_success 'stash a change to turn a non-directory to a directory' '
+	git reset --hard &&
+	>testfile &&
+	git add testfile &&
+	git commit -m "add testfile as a regular file" &&
+	rm testfile &&
+	mkdir testfile &&
+	>testfile/file &&
+	test_must_fail git stash save "recover regular file" &&
+	test -f testfile/file
+'
+
+test_expect_success 'stash a change to turn a non-directory to a directory (forced)' '
+	git stash save --force "recover regular file (forced)" &&
+	! test -f testfile/file &&
+	test -f testfile
+'
+
 test_done
-- 
1.8.3.1-814-gbbacdaa