threads / patch / 34299

patchgit stash: Avoid data loss when saving a stash

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

## tl;dr

10 messages between Jun 28, 2013 and Jul 6, 2013. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Petr Baudis· Jun 28, 2013, 15:05 UTC · lore

I have been recently again bitten by git stash versus an uncommitted file-to-directory change that happily threw away few gigabytes of my data. This has been recently discussed in the past, e.g.

	http://thread.gmane.org/gmane.comp.version-control.git/202332/

and I implemented Junio's suggestion in this patch - if ls-files --killed produced any output, the stash save is stopped and the user is required to pass --force to really have the data removed.

A test case is included. I think that the (currently failing) tests 'stash file to directory' and 'stash directory to file' are related to this, but I consider their expectation to be bogus - I would personally be surprised by `git stash` suddenly importing the complete never-meant-to-be-tracked contents of my directory to Git during a stash save, and I think the approach of aborting in this situation is more reasonable.

Other parts of Git also behave in a troublesome way in case of tracked file being replaced by an untracked directory - I wouldn't expect `git reset --hard` alone to remove the directory (i.e. touch untracked files) either. However, this matter is much more complicated since `git reset --hard` is assumed to "never fail in ordinary circumstances" (see e.g. git-stash code ;-) and I'm unable to devote sufficient effort to seeing such a change through.

Signed-off-by: Petr Baudis <pasky@ucw.cz>
---
Please Cc me, I'm currently not subscribed on the list.
 Documentation/git-stash.txt |   12 ++++++++++--
 git-stash.sh                |   10 ++++++++++
 t/t3903-stash.sh            |   16 ++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)
Show changes to 3 files +35 −2

Documentation/git-stash.txt, git-stash.sh, t/t3903-stash.sh

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index db7e803..52d4def 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.
+Normally, stash save will abort; `--force` will make it remove the
+untracked files.
 
 list [<options>]::
 
diff --git a/git-stash.sh b/git-stash.sh
index 1e541a2..3cb9b05 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,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
+		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
+		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 &&
+	! git stash save "symlink to non-empty directory" &&
+	[ -e linkdir/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 ]
+'
+
 test_done
-- 
1.7.10.4
Junio C Hamano· Jun 28, 2013, 18:39 UTC · re: Petr Baudis · lore

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

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(-)
Show changes to 3 files +40 −2

Documentation/git-stash.txt, git-stash.sh, t/t3903-stash.sh

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
Junio C Hamano· Jun 28, 2013, 19:37 UTC · re: Petr Baudis · lore

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

Petr Baudis <pasky@ucw.cz> writes:
Show 10 quoted lines
> diff --git a/git-stash.sh b/git-stash.sh
> index 1e541a2..3cb9b05 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -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
> +		say "$(gettext "The following untracked files would NOT be saved but need to be removed by stash save:")"

I think "ls-files --killed" was not adjusted for the new world order when submodules were introduced. With this change, you see t7402 break, even though "git status" would say

# Changes not staged for commit: # (use "git add <file>..." to update what will be committed) # (use "git checkout -- <file>..." to discard changes in working # directory) # # modified: file # modified: submodule (new commits)

The path "submodule" in HEAD and the index is already submodule, so "stash save" that reverts to the original state will _not_ have to kill it, but the new check triggers it.

Exactly the same breakage this patch introduces triggers in t7610, too.

I think another patch to teach "ls-files --killed" what to do with submodules is needed as a preliminary step before this patch.

Junio C Hamano· Jun 28, 2013, 21:30 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Petr Baudis <pasky@ucw.cz> writes:
>
>> diff --git a/git-stash.sh b/git-stash.sh
>> index 1e541a2..3cb9b05 100755
>> --- a/git-stash.sh
>> +++ b/git-stash.sh
>> @@ -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
>> +		say "$(gettext "The following untracked files would NOT be saved but need to be removed by stash save:")"
>
> I think "ls-files --killed" was not adjusted for the new world order
> when submodules were introduced.  With this change, you see t7402
> break,...
> Exactly the same breakage this patch introduces triggers in t7610,
> too.
>
> I think another patch to teach "ls-files --killed" what to do with
> submodules is needed as a preliminary step before this patch.
Which may be just the matter of doing this.
-- >8 --
Subject: treat_directory(): do not declare submodules in index to be untracked

When the working tree walker encounters a directory, it asks this function if it should descend into it, show it as an untracked directory, or do something else. When the directory is the top of the submodule working tree, we used to say "That is an untracked directory", which was quite bogus. It is an entity that is tracked in the index of the repository we are looking at, and that is not to be descended into it. Return path_none.

The existing case that path_untracked is returned for a newly discovered submodule that is not tracked in the index (this only happens when DIR_NO_GITLINKS option is not used) is unchanged and returns path_untracked, but that is exactly because the submodule is not tracked in the index.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to dir.c +1 −1
diff --git a/dir.c b/dir.c
index 897c874..b99c40e 100644
--- a/dir.c
+++ b/dir.c
@@ -1038,7 +1038,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 	case index_gitdir:
 		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
 			return path_none;
-		return path_untracked;
+		return path_none;
 
 	case index_nonexistent:
 		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
Petr Baudis· Jun 30, 2013, 13:20 UTC · re: Junio C Hamano · lore

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

  Hi!
On Fri, Jun 28, 2013 at 11:39:16AM -0700, Junio C Hamano wrote:
> 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.
  Thanks! I agree with all of your changes.
> -- >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
  Hmm, it's a pity that the note that `git reset --hard` itself should
perhaps also abort in that case got lost. I don't insist on mentioning
it in the commit message, though.
On Fri, Jun 28, 2013 at 02:30:15PM -0700, Junio C Hamano wrote:
> -- >8 --
> Subject: treat_directory(): do not declare submodules in index to be untracked
  Oh, you are truly awesome! I admit that properly reviewing this patch
is a little out of my depth right now as I'm not familiar with this
infrastructure. I'd just like to note...
Show 5 quoted lines
>  	case index_gitdir:
>  		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
>  			return path_none;
> -		return path_untracked;
> +		return path_none;
...that the if-test can be removed now as both branches are the same.
				Petr "Pasky" Baudis
Junio C Hamano· Jun 30, 2013, 19:14 UTC · re: Petr Baudis · lore

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

Petr Baudis <pasky@ucw.cz> writes:
Show 18 quoted lines
>   Hi!
>
> On Fri, Jun 28, 2013 at 11:39:16AM -0700, Junio C Hamano wrote:
>> 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.
>
>   Thanks! I agree with all of your changes.
>
>> -- >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
>
>   Hmm, it's a pity that the note that `git reset --hard` itself should
> perhaps also abort in that case got lost. I don't insist on mentioning
> it in the commit message, though.

I do not agree with your `git reset --hard` at all. With the command, the user demands "no matter what, I want get rid of any funny state in my working tree so that I can start my work from that specified commit (default to HEAD)".

Imagine that this is you did to arrive that "funny state":
	$ git rm foo ;# foo used to be tracked and in HEAD
        $ cp /somewhere/else/foo foo
	$ cp /somewhere/else/bar bar ;# bar is not in HEAD
	$ cp /somewhere/else/bar baz ;# baz is in HEAD
        ... do various other things ...

and then "git reset --hard". At that point, "foo" and "bar" are not tracked and completely unrelated to the project. "baz" is tracked and have unrelated contents from that of "HEAD".

In order to satisfy your desire to go back to the state of HEAD with minimal collateral amage, we need to get rid of the updated "foo" and "baz" and replace them with those from HEAD. We do not have to touch "bar" so we leave it as-is.

And the "killed" case is just like "foo" and "baz". If the state you want to go back to with "--hard" has a directory (a file) where your working tree's funny state has a file (a directory), the local cruft needs to go away to satisify your request.

I do not mind if you are proposing a different and new kind of reset that fails if it has to overwrite any local changes (be it tracked or untracked), but that is not "reset --hard". It is something else.

Show 15 quoted lines
> On Fri, Jun 28, 2013 at 02:30:15PM -0700, Junio C Hamano wrote:
>> -- >8 --
>> Subject: treat_directory(): do not declare submodules in index to be untracked
>
>   Oh, you are truly awesome! I admit that properly reviewing this patch
> is a little out of my depth right now as I'm not familiar with this
> infrastructure. I'd just like to note...
>
>>  	case index_gitdir:
>>  		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
>>  			return path_none;
>> -		return path_untracked;
>> +		return path_none;
>
> ...that the if-test can be removed now as both branches are the same.

Thanks for noticing. What was queued on 'pu' should already have fixed that one.

Petr Baudis· Jul 6, 2013, 14:42 UTC · re: Junio C Hamano · lore

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

  Hi!
  (tl;dr - I disagree but this issue is perhaps not so important
in practice)
On Sun, Jun 30, 2013 at 12:14:26PM -0700, Junio C Hamano wrote:
> I do not agree with your `git reset --hard` at all.  With the
> command, the user demands "no matter what, I want get rid of any
> funny state in my working tree so that I can start my work from that
> specified commit (default to HEAD)".
  Yeah, but this normally concerns only tracked files; `git reset
--hard` does not imply `git clean`. I'm worried when a tool normally
behaves in a way that follows an apparent rule but its behavior is
defined in such a way that in a corner case this rule is violated (but
it's ok since it's a - non-obvious - implication of the specification).
Show 16 quoted lines
> Imagine that this is you did to arrive that "funny state":
> 
> 	$ git rm foo ;# foo used to be tracked and in HEAD
>         $ cp /somewhere/else/foo foo
> 	$ cp /somewhere/else/bar bar ;# bar is not in HEAD
> 	$ cp /somewhere/else/bar baz ;# baz is in HEAD
>         ... do various other things ...
> 
> and then "git reset --hard".  At that point, "foo" and "bar" are not
> tracked and completely unrelated to the project.  "baz" is tracked
> and have unrelated contents from that of "HEAD".
> 
> In order to satisfy your desire to go back to the state of HEAD with
> minimal collateral amage, we need to get rid of the updated "foo"
> and "baz" and replace them with those from HEAD.  We do not have to
> touch "bar" so we leave it as-is.
  Perhaps we misundertood each other here. I certainly don't care to
keep local changes as a whole - a command behaving like that wouldn't
be very useful for me; for me, the crucial distinction is between
tracked and untracked files. Therefore, from my viewpoint it's fine
to overwrite baz, but not to overwrite foo.
Show 8 quoted lines
> And the "killed" case is just like "foo" and "baz".  If the state
> you want to go back to with "--hard" has a directory (a file) where
> your working tree's funny state has a file (a directory), the local
> cruft needs to go away to satisify your request.
> 
> I do not mind if you are proposing a different and new kind of reset
> that fails if it has to overwrite any local changes (be it tracked
> or untracked), but that is not "reset --hard".  It is something else.
  Hmm, I suppose the assumption I would prefer is that "the only command
that will destroy (currently) untracked data without warning is `git
clean`"; even though (unlike in case of git stash) the current reset
--hard behavior wouldn't surprise me, I suspect it can be a bad surprise
for many Git users when they hit this situation; but since I didn't
notice any actual complaint yet, so I don't care enough to press this
further for now anyway. :-)
-- 
				Petr "Pasky" Baudis
	For every complex problem there is an answer that is clear,
	simple, and wrong.  -- H. L. Mencken
Junio C Hamano· Jul 1, 2013, 21:59 UTC · re: Petr Baudis · lore

[PATCH v2 0/2] Safety for "stash save"

The second patch is the real thing, but "ls-files --killed" needed to be taught about submodules to avoid breaking "stash save" in a repository with submodules, as the new safety uses it to detect what will need to be killed (i.e. file D/F in the working tree getting removed because we need to check out D as a file, or file D in the working tree getting removed because we need to check out D/F as a file) when reverting to the state in HEAD.

Junio C Hamano (1):
  treat_directory(): do not declare submodules to be untracked
Petr Baudis (1):
  git stash: avoid data loss when "git stash save" kills a directory
 Documentation/git-stash.txt         | 12 ++++++++++--
 dir.c                               |  4 +---
 git-stash.sh                        | 12 ++++++++++++
 t/t3010-ls-files-killed-modified.sh | 23 ++++++++++++++++++++---
 t/t3903-stash.sh                    | 18 ++++++++++++++++++
 5 files changed, 61 insertions(+), 8 deletions(-)
-- 
1.8.3.2-798-g923e168
Junio C Hamano· Jul 1, 2013, 21:59 UTC · re: Junio C Hamano · lore

[PATCH v2 1/2] treat_directory(): do not declare submodules to be untracked

When the working tree walker encounters a directory, it asks the function treat_directory() if it should descend into it, show it as an untracked directory, or do something else. When the directory is the top of the submodule working tree, we used to say "That is an untracked directory", which was bogus.

It is an entity that is tracked in the index of the repository we are looking at, and that is not to be descended into it. Return path_none, not path_untracked, to report that.

The existing case that path_untracked is returned for a newly discovered submodule that is not tracked in the index (this only happens when DIR_NO_GITLINKS option is not used) is unchanged, but that is exactly because the submodule is not tracked in the index.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 dir.c                               |  4 +---
 t/t3010-ls-files-killed-modified.sh | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 6 deletions(-)
Show changes to 2 files +21 −6

dir.c, t/t3010-ls-files-killed-modified.sh

diff --git a/dir.c b/dir.c
index 897c874..0480419 100644
--- a/dir.c
+++ b/dir.c
@@ -1036,9 +1036,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 		return path_recurse;
 
 	case index_gitdir:
-		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
-			return path_none;
-		return path_untracked;
+		return path_none;
 
 	case index_nonexistent:
 		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 262e617..f611d79 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -11,6 +11,8 @@ This test prepares the following in the cache:
     path1       - a symlink
     path2/file2 - a file in a directory
     path3/file3 - a file in a directory
+    submod1/	- a submodule
+    submod2/	- another submodule
 
 and the following on the filesystem:
 
@@ -21,9 +23,11 @@ and the following on the filesystem:
     path4	- a file
     path5	- a symlink
     path6/file6 - a file in a directory
+    submod1/	- a submodule (modified from the cache)
+    submod2/	- a submodule (matches the cache)
 
-git ls-files -k should report that existing filesystem
-objects except path4, path5 and path6/file6 to be killed.
+git ls-files -k should report that existing filesystem objects
+path0/*, path1/*, path2 and path3 to be killed.
 
 Also for modification test, the cache and working tree have:
 
@@ -33,7 +37,7 @@ Also for modification test, the cache and working tree have:
     path10	- a non-empty file, cache dirtied.
 
 We should report path0, path1, path2/file2, path3/file3, path7 and path8
-modified without reporting path9 and path10.
+modified without reporting path9 and path10.  submod1 is also modified.
 '
 . ./test-lib.sh
 
@@ -48,6 +52,18 @@ test_expect_success 'git update-index --add to add various paths.' '
 	: >path9 &&
 	date >path10 &&
 	git update-index --add -- path0 path?/file? path7 path8 path9 path10 &&
+	for i in 1 2
+	do
+		git init submod$i &&
+		(
+			cd submod$i && git commit --allow-empty -m "empty $i"
+		) || break
+	done &&
+	git update-index --add submod[12]
+	(
+		cd submod1 &&
+		git commit --allow-empty -m "empty 1 (updated)"
+	) &&
 	rm -fr path?	# leave path10 alone
 '
 
@@ -94,6 +110,7 @@ test_expect_success 'validate git ls-files -m output.' '
 	path3/file3
 	path7
 	path8
+	submod1
 	EOF
 	test_cmp .expected .output
 '
-- 
1.8.3.2-798-g923e168
Junio C Hamano· Jul 1, 2013, 21:59 UTC · re: Junio C Hamano · lore

[PATCH v2 2/2] git stash: avoid data loss when "git stash save" kills a directory

From: Petr Baudis <pasky@ucw.cz>

"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, if the state you are restoring to has a non-directory at the same path as the directory.

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(-)
Show changes to 3 files +40 −2

Documentation/git-stash.txt, git-stash.sh, t/t3903-stash.sh

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.2-798-g923e168

← back to recent threads