[PATCH] stash: Don't fail if work dir contains file named 'HEAD'

[PATCH] stash: Don't fail if work dir contains file named 'HEAD'

[Jonathon Mah]

When performing a plain "git stash" (without --patch), git-diff would fail
with "fatal: ambiguous argument 'HEAD': both revision and filename". The
output was piped into git-update-index, masking the failed exit status.
The output is now sent to a temporary file (which is cleaned up by
existing code), and the exit status is checked. The "HEAD" arg to the
git-diff invocation has been disambiguated too, of course.

In patch mode, "git stash -p" would fail harmlessly, leaving the working
dir untouched.

Signed-off-by: Jonathon Mah <me@JonathonMah.com>
---
 git-stash.sh                       |    5 ++-
 t/t3903-stash.sh                   |   25 +++++++++++++++++++
 t/t3904-stash-patch.sh             |   47 ++++++++++++++++++++++-------------
 t/t3905-stash-include-untracked.sh |   13 +++++++++-
 4 files changed, 69 insertions(+), 21 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index c766692..a46f32a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,8 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
+			git diff --name-only -z HEAD -- > "$TMP-stagenames" &&
+			git update-index -z --add --remove --stdin < "$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
 		) ) ||
@@ -134,7 +135,7 @@ create_stash () {
 		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
 		die "$(gettext "Cannot save the current worktree state")"
 
-		git diff-tree -p HEAD $w_tree > "$TMP-patch" &&
+		git diff-tree -p HEAD $w_tree -- > "$TMP-patch" &&
 		test -s "$TMP-patch" ||
 		die "$(gettext "No changes selected")"
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index fcdb182..8f1d07a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -601,4 +601,29 @@ test_expect_success 'stash apply shows status same as git status (relative to cu
 	test_cmp expect actual
 '
 
+cat > expect << EOF
+diff --git a/HEAD b/HEAD
+new file mode 100644
+index 0000000..fe0cbee
+--- /dev/null
++++ b/HEAD
+@@ -0,0 +1 @@
++file-not-a-ref
+EOF
+
+test_expect_success 'stash where working directory contains "HEAD" file' '
+	git stash clear &&
+	git reset --hard &&
+	echo file-not-a-ref > HEAD &&
+	git add HEAD &&
+	git stash &&
+	git diff-files --quiet &&
+	git diff-index --cached --quiet HEAD &&
+	test_tick &&
+	test $(git rev-parse stash^) = $(git rev-parse HEAD) &&
+	git diff stash^..stash > output &&
+	test_cmp output expect &&
+	git stash drop
+'
+
 test_done
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 781fd71..70655c1 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -7,7 +7,8 @@ test_expect_success PERL 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
 	echo dummy > bar &&
-	git add bar dir/foo &&
+	echo committed > HEAD &&
+	git add bar dir/foo HEAD &&
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
@@ -17,47 +18,57 @@ test_expect_success PERL 'setup' '
 	save_head
 '
 
-# note: bar sorts before dir, so the first 'n' is always to skip 'bar'
+# note: order of files with unstaged changes: HEAD bar dir/foo
 
 test_expect_success PERL 'saying "n" does nothing' '
+	set_state HEAD HEADfile_work HEADfile_index &&
 	set_state dir/foo work index &&
-	(echo n; echo n) | test_must_fail git stash save -p &&
-	verify_state dir/foo work index &&
-	verify_saved_state bar
+	(echo n; echo n; echo n) | test_must_fail git stash save -p &&
+	verify_state HEAD HEADfile_work HEADfile_index &&
+	verify_saved_state bar &&
+	verify_state dir/foo work index
 '
 
 test_expect_success PERL 'git stash -p' '
-	(echo n; echo y) | git stash save -p &&
-	verify_state dir/foo head index &&
+	(echo y; echo n; echo y) | git stash save -p &&
+	verify_state HEAD committed HEADfile_index &&
 	verify_saved_state bar &&
+	verify_state dir/foo head index &&
 	git reset --hard &&
 	git stash apply &&
-	verify_state dir/foo work head &&
-	verify_state bar dummy dummy
+	verify_state HEAD HEADfile_work committed &&
+	verify_state bar dummy dummy &&
+	verify_state dir/foo work head
 '
 
 test_expect_success PERL 'git stash -p --no-keep-index' '
-	set_state dir/foo work index &&
+	set_state HEAD HEADfile_work HEADfile_index &&
 	set_state bar bar_work bar_index &&
-	(echo n; echo y) | git stash save -p --no-keep-index &&
-	verify_state dir/foo head head &&
+	set_state dir/foo work index &&
+	(echo y; echo n; echo y) | git stash save -p --no-keep-index &&
+	verify_state HEAD committed committed &&
 	verify_state bar bar_work dummy &&
+	verify_state dir/foo head head &&
 	git reset --hard &&
 	git stash apply --index &&
-	verify_state dir/foo work index &&
-	verify_state bar dummy bar_index
+	verify_state HEAD HEADfile_work HEADfile_index &&
+	verify_state bar dummy bar_index &&
+	verify_state dir/foo work index
 '
 
 test_expect_success PERL 'git stash --no-keep-index -p' '
-	set_state dir/foo work index &&
+	set_state HEAD HEADfile_work HEADfile_index &&
 	set_state bar bar_work bar_index &&
-	(echo n; echo y) | git stash save --no-keep-index -p &&
+	set_state dir/foo work index &&
+	(echo y; echo n; echo y) | git stash save --no-keep-index -p &&
+	verify_state HEAD committed committed &&
 	verify_state dir/foo head head &&
 	verify_state bar bar_work dummy &&
 	git reset --hard &&
 	git stash apply --index &&
-	verify_state dir/foo work index &&
-	verify_state bar dummy bar_index
+	verify_state HEAD HEADfile_work HEADfile_index &&
+	verify_state bar dummy bar_index &&
+	verify_state dir/foo work index
 '
 
 test_expect_success PERL 'none of this moved HEAD' '
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index ef44fb2..7f75622 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -17,6 +17,7 @@ test_expect_success 'stash save --include-untracked some dirty working directory
 	echo 3 > file &&
 	test_tick &&
 	echo 1 > file2 &&
+	echo 1 > HEAD &&
 	mkdir untracked &&
 	echo untracked >untracked/untracked &&
 	git stash --include-untracked &&
@@ -35,6 +36,13 @@ test_expect_success 'stash save --include-untracked cleaned the untracked files'
 '
 
 cat > expect.diff <<EOF
+diff --git a/HEAD b/HEAD
+new file mode 100644
+index 0000000..d00491f
+--- /dev/null
++++ b/HEAD
+@@ -0,0 +1 @@
++1
 diff --git a/file2 b/file2
 new file mode 100644
 index 0000000..d00491f
@@ -51,6 +59,7 @@ index 0000000..5a72eb2
 +untracked
 EOF
 cat > expect.lstree <<EOF
+HEAD
 file2
 untracked
 EOF
@@ -58,7 +67,8 @@ EOF
 test_expect_success 'stash save --include-untracked stashed the untracked files' '
 	test "!" -f file2 &&
 	test ! -e untracked &&
-	git diff HEAD stash^3 -- file2 untracked >actual &&
+	test "!" -f HEAD &&
+	git diff HEAD stash^3 -- HEAD file2 untracked >actual &&
 	test_cmp expect.diff actual &&
 	git ls-tree --name-only stash^3: >actual &&
 	test_cmp expect.lstree actual
@@ -75,6 +85,7 @@ git clean --force --quiet
 
 cat > expect <<EOF
  M file
+?? HEAD
 ?? actual
 ?? expect
 ?? file2
-- 
1.7.8



Jonathon Mah
me@JonathonMah.com

Re: [PATCH] stash: Don't fail if work dir contains file named 'HEAD'

[Thomas Rast]

Jonathon Mah <me@JonathonMah.com> writes:
> When performing a plain "git stash" (without --patch), git-diff would fail
> with "fatal: ambiguous argument 'HEAD': both revision and filename". The
> output was piped into git-update-index, masking the failed exit status.
> The output is now sent to a temporary file (which is cleaned up by
> existing code), and the exit status is checked. The "HEAD" arg to the
> git-diff invocation has been disambiguated too, of course.

Thanks, good catch.

> In patch mode, "git stash -p" would fail harmlessly, leaving the working
> dir untouched.

Note that this only affects stash -p, not add/reset/commit -p, because
it is the only one that does an extra patch dance on top of the
git-add--interactive work.  stash -p uses a 'diff-index -p HEAD'
invocation in the %patch_modes of git-add--interactive, but diff-index
doesn't need disambiguation as the first argument is always the (sole)
tree-ish.

I had to look and verify, so perhaps you can put a paragraph to this
effect in the commit message.

> -			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
> +			git diff --name-only -z HEAD -- > "$TMP-stagenames" &&
> +			git update-index -z --add --remove --stdin < "$TMP-stagenames" &&

Style nit: we usually spell it >foo.  I saw that git-stash is already
inconsistent, but let's at least not make it worse.

While reading this I also wondered if there was a good reason it didn't
just use 'add -u', and indeed: 7aa5d43 (stash: Don't overwrite files
that have gone from the index, 2010-04-18) changed it *away* from add -u
because that was broken.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
[...]
> +test_expect_success 'stash where working directory contains "HEAD" file' '
> +	git stash clear &&
> +	git reset --hard &&
> +	echo file-not-a-ref > HEAD &&
> +	git add HEAD &&
> +	git stash &&
> +	git diff-files --quiet &&
> +	git diff-index --cached --quiet HEAD &&
> +	test_tick &&

What's the tick good for if you don't create any commits after it?  You
should put it immediately before the 'git stash'.

> +	test $(git rev-parse stash^) = $(git rev-parse HEAD) &&

This should probably have its arguments quoted, to avoid confusing
'test' if something goes horribly wrong (e.g. stash^ is not valid).
There are plenty of existing lines of this form in other tests however.

> +	git diff stash^..stash > output &&
> +	test_cmp output expect &&
> +	git stash drop

Perhaps this could go after the 'git stash' as a

  test_when_finished "git stash drop"

> +'

> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
[...]
> -# note: bar sorts before dir, so the first 'n' is always to skip 'bar'
> +# note: order of files with unstaged changes: HEAD bar dir/foo
>  
>  test_expect_success PERL 'saying "n" does nothing' '
> +	set_state HEAD HEADfile_work HEADfile_index &&
>  	set_state dir/foo work index &&
> -	(echo n; echo n) | test_must_fail git stash save -p &&
> -	verify_state dir/foo work index &&
> -	verify_saved_state bar
> +	(echo n; echo n; echo n) | test_must_fail git stash save -p &&
> +	verify_state HEAD HEADfile_work HEADfile_index &&
> +	verify_saved_state bar &&
> +	verify_state dir/foo work index
>  '

Other reviewers may want to read these hunks in word diff mode, where it
is far easier to verify that the functionality tested is a superset:

  test_expect_success PERL 'saying "n" does nothing' '
          {+set_state HEAD HEADfile_work HEADfile_index &&+}
          set_state dir/foo work index &&
          (echo n; echo {+n; echo+} n) | test_must_fail git stash save -p &&
          verify_state [-dir/foo work index-]{+HEAD HEADfile_work HEADfile_index+} &&
          verify_saved_state bar {+&&+}
  {+      verify_state dir/foo work index+}
  '

> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
[...]
>  test_expect_success 'stash save --include-untracked stashed the untracked files' '
>  	test "!" -f file2 &&
>  	test ! -e untracked &&
> -	git diff HEAD stash^3 -- file2 untracked >actual &&
> +	test "!" -f HEAD &&
> +	git diff HEAD stash^3 -- HEAD file2 untracked >actual &&

Again not something you started, but we may want to clean this up to say

  test_path_is_missing file2 &&
  test_path_is_missing untracked &&
  test_path_is_missing HEAD &&

etc.  This is nicer (gives a message) if the test fails.  It also barfs
if there is a freak bug that made HEAD a device or some such :-)


Anyway, except for the test_tick those were just style nits.  You can
add

  Acked-by: Thomas Rast <trast@student.ethz.ch>

when you reroll.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] stash: Don't fail if work dir contains file named 'HEAD'

[Jonathon Mah]

Thanks for the feedback, Thomas. I should note, this bug initially came up on #git several days ago.

I've tried to take on all your suggestions; patch v2 imminent.

On 2011-12-30, at 02:15, Thomas Rast wrote:

> Jonathon Mah <me@JonathonMah.com> writes:
>> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> [...]
> 
> Other reviewers may want to read these hunks in word diff mode, where it
> is far easier to verify that the functionality tested is a superset:
> 
>  test_expect_success PERL 'saying "n" does nothing' '
>          {+set_state HEAD HEADfile_work HEADfile_index &&+}
>          set_state dir/foo work index &&
>          (echo n; echo {+n; echo+} n) | test_must_fail git stash save -p &&
>          verify_state [-dir/foo work index-]{+HEAD HEADfile_work HEADfile_index+} &&
>          verify_saved_state bar {+&&+}
>  {+      verify_state dir/foo work index+}
>  '

I added a note to the message: In t3904, checks and operations on each file are in the order they'll appear when interactively staging.

That is, "echo y/n; echo y/n; ..." for the three files corresponds to the surrounding checks.



Jonathon Mah
me@JonathonMah.com

Re: [PATCH] stash: Don't fail if work dir contains file named 'HEAD'

[Junio C Hamano]

Jonathon Mah <me@JonathonMah.com> writes:

> When performing a plain "git stash" (without --patch), git-diff would fail
> with "fatal: ambiguous argument 'HEAD': both revision and filename". The
> output was piped into git-update-index, masking the failed exit status.
> The output is now sent to a temporary file (which is cleaned up by
> existing code), and the exit status is checked. The "HEAD" arg to the
> git-diff invocation has been disambiguated too, of course.
>
> In patch mode, "git stash -p" would fail harmlessly, leaving the working
> dir untouched.
>
> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> ---

Thanks.