On 03/17, Jeff King wrote:
> 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.
> 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 :)