Re: Apparent bug in 'git stash push <subdir>' loses untracked files

[Junio C Hamano <gitster@pobox.com>]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Ah interesting, what you have below looks good to me indeed, it
> matches what I'd expect it to do and fixes the bug that was reported.
> Thanks! 
>
> I've taken the liberty to take what you have below and turned into a
> proper patch, giving you authorship of it, as you wrote the code for
> the fix.  Hope that's the appropriate credit for you for this patch.

Not so fast.

I know why the updated code works like "--hard -- <pathspec>", but I
do not quite get what the original was trying to do and how it is
different.  Even with your proposed log message, which describes a
symptom (i.e. "untracked files ... be deleted"), it is unclear why
this deletion was happening in the first place.  Specifically, it is
unclear why that "clean --force -q -d" was in there, and are we
breaking other cases by rewriting this codepath without it?

In any case, instead of the 

	ls-files -z -- "$@" | checkout-index -z --force --stdin
	diff-index -p HEAD -- "$@" | apply --index -R

sequence, a shorter variant that should work is

	add -u -- "$@"
	diff-index -p --cached HEAD -- "$@" | apply --index -R

Both of these share the same idea.  

 - The first step is to match what is in the index and what is in
   the working tree (i.e. make "diff-files" silent).  The version
   you have does so by checking out what is in the index to the
   working tree.  The shorter one goes the other way and updates the
   index with what is in the working tree.

 - Once that is done, we ask diff-index what got changed since the
   HEAD in the working tree (or in the index in the updated
   one---after the first step that makes the two match, comparing
   with the working tree and comparing with the index should result
   in the same diff; I have a suspicion that "--cached" is faster,
   but we need to benchmark to pick), and ask apply to get rid of
   all these changes, which includes removal of added files, and
   resurrection of removed files.

I think the original that did 'git reset -- "$@"' upfront lost new
paths from the index (without removing it from the working tree), I
am guessing that it is why "clean" was there to get rid of them, and
if that is the reason, I can understand why the original code was
behaving incorrectly---it would get rid of new files that did not
exist in HEAD correctly, but it also would remove untracked ones,
because after that first 'reset', the code couldn't tell between
them.

And I think that is what we want, i.e. why the original was wrong
and how the new one fixes it, to describe in the log message to
justify this change.

One thing that I didn't think through and you need to verify is if
we need to do anything extra to deal with binary files (in the old
days, we needed --full-index and --binary options to produce and
apply a binary patch; I do not offhand know if that is still the
case) in the final "diff-index piped to apply -R --index" dance.

So I am asking you to fill in quite a lot of gaps that I didn't do
with only the above two-liner ;-)  You should take the authorship
and, if you like, credit me with helped-by: or something.

Thanks.


>
> [...]
>
> --- >8 ---
> From: Junio C Hamano <gitster@pobox.com>
> Subject: [PATCH] stash: don't delete untracked files that match pathspec
>
> Currently when 'git stash push -- <pathspec>' is used, untracked files
> that match the pathspec will be deleted, even though they do not end up
> in a stash anywhere.
>
> Untracked files should be left along by 'git stash push -- <pathspec>'
> unless the --untracked or --all flags are given.  Fix that.
>
> Reported-by: Reid Price <reid.price@gmail.com>
> Test-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  git-stash.sh     |  5 ++---
>  t/t3903-stash.sh | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 1114005ce2..a979bfb665 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -322,10 +322,9 @@ push_stash () {
>  
>  		if test $# != 0
>  		then
> -			git reset -q -- "$@"
> -			git ls-files -z --modified -- "$@" |
> +			git ls-files -z -- "$@" |
>  			git checkout-index -z --force --stdin
> -			git clean --force -q -d -- "$@"
> +			git diff-index -p HEAD -- "$@" | git apply --index -R
>  		else
>  			git reset --hard -q
>  		fi
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 39c7f2ebd7..6952a031b2 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1064,4 +1064,20 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
>  	test foo,bar = $(cat foo),$(cat bar)
>  '
>  
> +test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
> +	git reset &&
> +	>subdir/untracked &&
> +	>subdir/tracked1 &&
> +	>subdir/tracked2 &&
> +	git add subdir/tracked* &&
> +	git stash -- subdir/ &&
> +	test_path_is_missing subdir/tracked1 &&
> +	test_path_is_missing subdir/tracked2 &&
> +	test_path_is_file subdir/untracked &&
> +	git stash pop &&
> +	test_path_is_file subdir/tracked1 &&
> +	test_path_is_file subdir/tracked2 &&
> +	test_path_is_file subdir/untracked
> +'
> +
>  test_done