Re: [BUG] "git stash -- path" reports wrong unstaged changes

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 18, 2017, 18:36 UTC
Message-ID: <20170318183658.GC27158@hank>
In-Reply-To: <20170317145039.dmcb3qyqbzfvtmgz@sigill.intra.peff.net>

On 03/17, Jeff King wrote:

Show 25 quoted lines

> I used "git stash -- path" for the first time today and happened to
> notice an oddity. If I run:
> 
> 	git init -q repo
> 	cd repo
> 	
> 	for i in one two; do
> 		echo content >$i
> 		git add $i
> 	done
> 	git commit -qm base
> 	
> 	for i in one two; do
> 		echo change >$i
> 	done
> 	git stash -- one
> 
> it says:
> 
>   Saved working directory and index state WIP on master: 20cfadf base
>   Unstaged changes after reset:
>   M	one
>   M	two
> 
> Even though "one" no longer has unstaged changes.

Yeah, this is clearly not right. Thanks for catching this before it got into any release.

Show 38 quoted lines

> If I run with GIT_TRACE=1, that message is generated by:
> 
>   git reset -- one
> 
> which makes sense. At that stage we've just reset the index, but the
> working tree file still has modifications. In the non-pathspec case we
> run "git reset --hard", which takes care of the index and the working
> tree.
> 
> It's really "checkout-index" that cleans the working tree, but it
> doesn't have porcelain finery like an "Unstaged changes" message. I
> think the patch below would fix it, but I wonder if we can do it in a
> way that doesn't involve calling diff-files twice.
> 
> -Peff
> 
> ---
> diff --git a/git-stash.sh b/git-stash.sh
> index 9c70662cc..9a4bb503a 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -299,10 +299,15 @@ push_stash () {
>  	then
>  		if test $# != 0
>  		then
> -			git reset ${GIT_QUIET:+-q} -- "$@"
> +			git reset -q -- "$@"
>  			git ls-files -z --modified -- "$@" |
>  			git checkout-index -z --force --stdin
>  			git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> +			if test -z "$GIT_QUIET" && ! git diff-files --quiet
> +			then
> +				say "$(gettext "Unstaged changes after reset:")"
> +				git diff-files --name-status
> +			fi
>  		else
>  			git reset --hard ${GIT_QUIET:+-q}
>  		fi

This would mean the user gets something like in your case above:

    Unstaged changes after reset:
     M	two

As a user that doesn't know the internal implementation of push_stash, this would make me wonder why git stash would mention a file that is not provided as pathspec, but not the one that was provided in the pathspec argument.

I think one option would be to to just keep quiet about the exact changes that git stash push makes, similar to what we do in the --include-untracked and in the -p case. The other option would be to find the files that are affected and print them, but that would probably be a bit too noisy especially in cases such as git stash push -- docs/*.

Also from reading the code in the -p case, when --keep-index is given, the git reset there doesn't respect $GIT_QUIET at all, and also doesn't respect the pathspec argument, which seems like another bug. I can submit a patch series for those, but I won't get to it before tomorrow :)