git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 28, 2013, 18:39 UTC
Message-ID
<7vobaqnkq3.fsf@alter.siamese.dyndns.org>
In-Reply-To
<20130628150532.GD12252@machine.or.cz>
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.
Show 11 quoted lines
> @@ -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.
Show 5 quoted lines
> @@ -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/;
Show 19 quoted lines
> +		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.

Show 6 quoted lines
> +'
> +
> +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
Previous: Petr BaudisNext: Junio C Hamano
Message 2 of 10 in “git stash: Avoid data loss when saving a stash”
  1. git stash: Avoid data loss when saving a stashPetr Baudis, Jun 28, 2013
  2. Junio C HamanoJun 28, 2013
  3. Junio C HamanoJun 28, 2013
  4. Junio C HamanoJun 28, 2013
  5. Petr BaudisJun 30, 2013
  6. Junio C HamanoJun 30, 2013
  7. Petr BaudisJul 6, 2013
  8. 0/2 Safety for "stash save"Junio C Hamano, Jul 1, 2013
  9. 1/2 treat_directory(): do not declare submodules to be untrackedJunio C Hamano, Jul 1, 2013
  10. 2/2 git stash: avoid data loss when "git stash save" kills a directoryJunio C Hamano, Jul 1, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.