Petr Baudis <pasky@xxxxxx> writes:
> Signed-off-by: Petr Baudis <pasky@xxxxxx>
> ---
>
> 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@xxxxxx>
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@xxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html