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

[Thomas Rast <trast@student.ethz.ch>]

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