git stash push -u always warns "pathspec '...' did not match any files"

Reproducible in a test repository with following steps:

$ touch untracked $ git stash push -u -- untracked Saved working directory and index state WIP on master: 0096475 init fatal: pathspec 'untracked' did not match any files error: unrecognized input

The file is stashed correctly, though.

Tested with Git 2.16.2 on Linux and Windows.

-Marc

On 03/03, Marc Strapetz wrote:

Thanks for the bug report and the reproduction recipe. The following patch should fix it:

--- >8 ---
Subject: [PATCH] stash push: avoid printing errors

Currently 'git stash push -u -- <pathspec>' prints the following errors if <pathspec> only matches untracked files:

    fatal: pathspec 'untracked' did not match any files
    error: unrecognized input

This is because we first clean up the untracked files using 'git clean <pathspec>', and then use a command chain involving 'git add -u <pathspec>' and 'git apply' to clear the changes to files that are in the index and were stashed.

As the <pathspec> only includes untracked files that were already removed by 'git clean', the 'git add' call will barf, and so will 'git apply', as there are no changes that need to be applied.

Fix this by making sure to only call this command chain if there are still files that match <pathspec> after the call to 'git clean'.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

On 03.03.2018 16:46, Thomas Gummerer wrote:

Thanks, I can confirm that the misleading warning message is fixed.

What I've noticed now is that when using -u option, Git won't warn if the pathspec is actually not matching a file. Also, an empty stash may be created. For example:

$ git stash push -u -- nonexisting Saved working directory and index state WIP on master: 171081d initial import

I would probably expect to see an error message as for:

$ git stash push -- nonexisting error: pathspec 'nonexisting' did not match any file(s) known to git. Did you forget to 'git add'?

That said, this is no problem for us, because I know that the paths I'm providing to "git stash push" do exist. I just wanted to point out.

-Marc

Marc Strapetz <marc.strapetz@syntevo.com> writes:

Soooo..., does it mean that the patch Thomas posted and you confirmed trades one issue with another issue with a similar graveness? Is this something we want to "fix" without adding another breakage?

On 09.03.2018 23:18, Junio C Hamano wrote:

 From my understanding these are two separate problems for which the new 
one was somewhat hidden by the one Thomas has fixed: Thomas has fixed 
post-processing code after the stash has already been saved away. The 
problem I'm referring to is a missing check for invalid paths before the 
stash is saved away.

-Marc

On 03/10, Marc Strapetz wrote:

I've been meaning to follow up on this, but haven't found the time to do so yet, sorry.

Yeah, just to demonstrate what the new problem Marc describes is, currently 'git stash push -u <unknown>' would produce the following output, and create a new stash:

    $ git stash push -u unknown
    Saved working directory and index state WIP on master: 7e31236f65 Sixth batch for 2.17
    fatal: pathspec 'unknown' did not match any files
    error: unrecognized input
    $

With the patch I posted it would just print 

    $ git stash push -u unknown
    Saved working directory and index state WIP on master: 7e31236f65 Sixth batch for 2.17
    $

and produce a new stash as before. Both of those end up confusing to the user, dunno which one is better. What really should happen is

    $ git stash push -u unknown
    No local changes to save
    $

and not creating a stash. So these were many words to basically say that I think my patch is still the right thing to do, but it may or may not confuse the user more if they are hitting the other bug Marc noted. Either way I'll try to address this as soon as I can get some time to look at it.

Currently 'git stash push -u -- <pathspec>' prints the following errors if <pathspec> only matches untracked files:

    fatal: pathspec 'untracked' did not match any files
    error: unrecognized input

This is because we first clean up the untracked files using 'git clean <pathspec>', and then use a command chain involving 'git add -u <pathspec>' and 'git apply' to clear the changes to files that are in the index and were stashed.

As the <pathspec> only includes untracked files that were already removed by 'git clean', the 'git add' call will barf, and so will 'git apply', as there are no changes that need to be applied.

Fix this by making sure to only call this command chain if there are still files that match <pathspec> after the call to 'git clean'.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

I finally got around to do this. The fix (in the second patch) turns out to be fairly simple, I just forgot to pass the pathspec along to one function whene originally introducing the pathspec feature in git stash push (more explanation in the commit message for the patch itself). Thanks Marc for reporting the two breakages!

v2 also fixes a couple of typos in the first patch which I failed to notice when I sent it out last time.

 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

When introducing the stash push feature, and thus allowing users to pass in a pathspec to limit the files that would get stashed in df6bba0937 ("stash: teach 'push' (and 'create_stash') to honor pathspec", 2017-02-28), this developer missed one place where the pathspec should be passed in.

Namely in the call to the 'untracked_files()' function in the 'no_changes()' function. This resulted in 'git stash push -u -- <non-existant>' creating an empty stash when there are untracked files in the repository other that don't match the pathspec.

As 'git stash' never creates empty stashes, this behaviour is wrong and confusing for users. Instead it should just show a message "No local changes to save", and not create a stash.

Luckily the 'untracked_files()' function already correctly respects pathspecs that are passed to it, so the fix is simply to pass the pathspec along to the function.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

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

Don't you need a pair of double-quotes around that $@?

On 03/15, Junio C Hamano wrote:

Ah yes indeed.  Will fix, thanks!

On 14.03.2018 22:46, Thomas Gummerer wrote:

Thanks, I confirm that both issues are resolved. There is another issue now which seems to be a regression. When *successfully* stashing an untracked file, local modifications of other files are cleared, too.

$ git init
$ touch file1
$ git add file1
$ git commit -m "initial import"
$ echo "a" > file1
$ touch file2
$ git status --porcelain
  M file1
?? file2
$ git stash push -u -- file2
Saved working directory and index state WIP on master: 25352d7 initial 
import
$ git status
On branch master
nothing to commit, working tree clean

Hence, by stashing just "file2" the local modification of "file1" became reset.

-Marc

On 03/15, Marc Strapetz wrote:

Ah yes, this is indeed a regression, thanks for catching it. I'll fix it in the re-roll and add another test for this case. Sorry about the false starts here :/

Thanks Marc for catching the regression I almost introduced and Junio for the review of the second patch. Here's a re-roll that should fix the issues of v2.

Interdiff below:

When introducing the stash push feature, and thus allowing users to pass in a pathspec to limit the files that would get stashed in df6bba0937 ("stash: teach 'push' (and 'create_stash') to honor pathspec", 2017-02-28), this developer missed one place where the pathspec should be passed in.

Namely in the call to the 'untracked_files()' function in the 'no_changes()' function. This resulted in 'git stash push -u -- <non-existant>' creating an empty stash when there are untracked files in the repository other that don't match the pathspec.

As 'git stash' never creates empty stashes, this behaviour is wrong and confusing for users. Instead it should just show a message "No local changes to save", and not create a stash.

Luckily the 'untracked_files()' function already correctly respects pathspecs that are passed to it, so the fix is simply to pass the pathspec along to the function.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Currently 'git stash push -u -- <pathspec>' prints the following errors if <pathspec> only matches untracked files:

    fatal: pathspec 'untracked' did not match any files
    error: unrecognized input

This is because we first clean up the untracked files using 'git clean <pathspec>', and then use a command chain involving 'git add -u <pathspec>' and 'git apply' to clear the changes to files that are in the index and were stashed.

As the <pathspec> only includes untracked files that were already removed by 'git clean', the 'git add' call will barf, and so will 'git apply', as there are no changes that need to be applied.

Fix this by making sure to only call this command chain if there are still files that match <pathspec> after the call to 'git clean'.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     |  9 ++++++---
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

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

This obviously is not something this patch breaks, but I am finding this pipeline that was already here quite puzzling.

The "add -u" is about adding the changes in paths that match the pathspec to the index; the output from it is meant for human consumption and certainly is not something "--stdin" should expect to be understandable, let alone with "-z".

	... goes and digs ...

I think you mis-copied the suggestion in

    https://public-inbox.org/git/xmqqpo7byjwb.fsf@gitster.mtv.corp.google.com/

when you made bba067d2 ("stash: don't delete untracked files that match pathspec", 2018-01-06), and nobody caught that breakage during the review.

And the same breakage is inherited here; just drop "|" and downstream "checkout-index" and you should be OK.

And while at it, let's split this to two lines after "|".

Here is a preliminary fix for an earlier copy-pasto, plus two patches from your v3.

I tried to reduce the nesting level by swapping the order of if/elif chain; please double check the logic to ensure I didn't make stupid mistakes while doing so.

Junio C Hamano (1):
  stash: fix nonsense pipeline

Thomas Gummerer (2):
  stash push: avoid printing errors
  stash push -u: don't create empty stash

 git-stash.sh     | 13 +++++++------
 t/t3903-stash.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)

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

'git stash push -u -- <pathspec>' prints the following errors if <pathspec> only matches untracked files:

    fatal: pathspec 'untracked' did not match any files
    error: unrecognized input

This is because we first clean up the untracked files using 'git clean <pathspec>', and then use a command chain involving 'git add -u <pathspec>' and 'git apply' to clear the changes to files that are in the index and were stashed.

As the <pathspec> only includes untracked files that were already removed by 'git clean', the 'git add' call will barf, and so will 'git apply', as there are no changes that need to be applied.

Fix this by making sure to only call this command chain if there are still files that match <pathspec> after the call to 'git clean'.

[jc: swapped the order of if/elseif chain to reduce nesting levels]

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-stash.sh     |  7 ++++---
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

An earlier change bba067d2 ("stash: don't delete untracked files that match pathspec", 2018-01-06) was made by taking a suggestion in a list discussion [1] but did not copy the suggested snippet correctly. And the bug was unnoticed during the review and slipped through.

This fixes it.

[1] https://public-inbox.org/git/xmqqpo7byjwb.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-stash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

When introducing the stash push feature, and thus allowing users to pass in a pathspec to limit the files that would get stashed in df6bba0937 ("stash: teach 'push' (and 'create_stash') to honor pathspec", 2017-02-28), this developer missed one place where the pathspec should be passed in.

Namely in the call to the 'untracked_files()' function in the 'no_changes()' function. This resulted in 'git stash push -u -- <non-existant>' creating an empty stash when there are untracked files in the repository other that don't match the pathspec.

As 'git stash' never creates empty stashes, this behaviour is wrong and confusing for users. Instead it should just show a message "No local changes to save", and not create a stash.

Luckily the 'untracked_files()' function already correctly respects pathspecs that are passed to it, so the fix is simply to pass the pathspec along to the function.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

On 03/16, Junio C Hamano wrote:

Thanks for catching and fixing this!

I looked over what you send, and the patches and the changes you made look good to me.

Thanks again Marc for all the testing and Junio for fixing up my brown paper bag copy-pasto.

This iteration addresses the breakage Marc noticed with the latest version of the patches, adds some more tests, and moves all the new tests to t3905 instead of t3903, which I just noticed exists and is a much better match for the new tests.

Patch 1 and 3 are the same as in the previous round, Patch 2 is mostly rewritten. Instead of trying to avoid part of the pipeline we're using to get rid of changes, we now are getting rid of the 'git clean' call in the pathspec case, and use the existing pipeline to get rid of changes in untracked files as well. I'm not adding an interdiff, because Patch 2 is mostly rewritten and the other two are unchanged, so it is probably easiest to just review patch 2.

Junio C Hamano (1):
  stash: fix nonsense pipeline

Thomas Gummerer (2):
  stash push: avoid printing errors
  stash push -u: don't create empty stash

 git-stash.sh                       | 12 +++++----
 t/t3905-stash-include-untracked.sh | 52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 5 deletions(-)

From: Junio C Hamano <gitster@pobox.com>

An earlier change bba067d2 ("stash: don't delete untracked files that match pathspec", 2018-01-06) was made by taking a suggestion in a list discussion [1] but did not copy the suggested snippet correctly. And the bug was unnoticed during the review and slipped through.

This fixes it.

[1] https://public-inbox.org/git/xmqqpo7byjwb.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-stash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

'git stash push -u -- <pathspec>' prints the following errors if <pathspec> only matches untracked files:

    fatal: pathspec 'untracked' did not match any files
    error: unrecognized input

This is because we first clean up the untracked files using 'git clean <pathspec>', and then use a command chain involving 'git add -u <pathspec>' and 'git apply' to clear the changes to files that are in the index and were stashed.

As the <pathspec> only includes untracked files that were already removed by 'git clean', the 'git add' call will barf, and so will 'git apply', as there are no changes that need to be applied.

Fix this by avoiding the 'git clean' if a pathspec is given, and use the pipeline that's used for pathspec mode to get rid of the untracked files as well.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh                       |  6 +++--
 t/t3905-stash-include-untracked.sh | 46 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

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

That made me wonder if we can get rid of 'git clean' altogether by pretending that we saw a pathspec '.' that match everything when no pathspec is given---that way we only have to worry about a single codepath.

But I guess doing it this way can minimize potential damage. Those who do not use pathspec when running "git stash" won't be affected even if this change had some bugs ;-)

Thanks, I'll take the change as-is.

I however wonder if we restructure the code to

	if test $# = 0
	then
		# no pathspec
		if test -n "$untracked"
		then
			git clean --force --quiet -d $CLEAN_OPTION -- "$@"
		fi
		git reset --hard -q
	else
		test -z "$untracked" && UPDATE=-u || UPDATE=
		test "$untracked" = all && FORCE=--force || FORCE=
		git add $UPDATE $FORCE-- "$@"
		git diff-index -p --cached --binary HEAD -- "$@" |
		git apply --index -R
	fi

it becomes easier to understand what is going on.

That way, once we have a plumbing command to help the else clause of the above, i.e. "git checkout --index <tree-ish> -- <pathspec>" [*1*], then we can lose the if/then/else and rewrite the whole "we have created stash, so it's time to get rid of local modifications to the paths that match the pathspec" code to:

	if test "$untracked"
	then
		git clean --force --quiet -d $CLEAN_OPTION -- "$@"
	fi
	git checkout --index HEAD -- "$@"

[Footnote] cf. https://public-inbox.org/git/xmqq4loqplou.fsf@gitster.mtv.corp.google.com/

What we want in the case in the code in question when there is pathspec is "match the index entries and the working tree files to those that appear in a given tree for paths that match the given pathspec". This is close to "git checkout <tree-ish> -- <pathspec>" but not quite. Current "git checkout <tree-ish> -- <pathspec>" is an overlay operation in that paths that match <pathspec> but do not exist in <tree-ish> are *NOT* removed from the working tree. We obviously cannot change the behaviour of the command.

But we can add an option to ask for the new behaviour. In general, for an operation that affects the index and the working tree, we can have "--cached" mode that affects only the index without touching the working tree, and "--index" mode that affects both.

"git reset <tree-ish> -- <pathspec>", which is a UI mistake, is better spelled "git checkout --cached <tree-ish> -- <pathspec>". We take paths that match <pathspec> from <tree-ish> and stuff into the index, and remove entries from the index for paths that do not exist in <tree-ish>. And if we extend that to "--index" mode, that is exactly what we want to happen.

On 03/20, Junio C Hamano wrote:

Heh yeah, we found enough bugs in this code so far, so it's probably best to leave the part that's working alone at least for now.

Argh I just noticed we could drop the "$@" here, as this is no longer the pathspec case. It doesn't hurt anything, except it may be a bit confusing when reading the code.

Although if we end up implementing 'git checkout --index <pathspec>', we'd have to add it back, but we do have a test covering this case, so there's no worries about forgetting to add it back.

I like that code structure more than what I have now. I see you already merged what I had to next, and I like keeping the change small now that we're in the rc period (assuming you want to get this into 2.17?) Maybe we can restructure the code as a separate cleanup once 2.17 is out, so this has more time to cook in master and hopefully we'd notice regressions before the next release?

Yeah, this would be nice to have. I wanted to have a look at what it would take to implement 'git checkout --{cached,index}', but I'm not familiar with the checkout code at all, so it will probably be a while until I can get around to do it.

On 03/21, Thomas Gummerer wrote:

Here's a patch for that. Not sure it's worth doing, but since we're already touching the area, this may be a good cleanup.

This is based on top of tg/stash-untracked-with-pathspec-fix.

--- >8 ---
Subject: [PATCH] stash: drop superfluos pathspec parameter

Since 833622a945 ("stash push: avoid printing errors", 2018-03-19) we don't use the 'git clean' call for the pathspec case anymore. The commit however forgot to remove the pathspec argument to the call. Remove the superfluos argument to make the code a little more obvious.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

OK, I can go either way, and since you have already written a change, let's not waste it ;-)

Thanks.

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

Yes, we could, but we do not have to. I think it is fine to leave it in especially if we envision that the codepaths for two cases will be unified in the future.

When introducing the stash push feature, and thus allowing users to pass in a pathspec to limit the files that would get stashed in df6bba0937 ("stash: teach 'push' (and 'create_stash') to honor pathspec", 2017-02-28), this developer missed one place where the pathspec should be passed in.

Namely in the call to the 'untracked_files()' function in the 'no_changes()' function. This resulted in 'git stash push -u -- <non-existant>' creating an empty stash when there are untracked files in the repository other that don't match the pathspec.

As 'git stash' never creates empty stashes, this behaviour is wrong and confusing for users. Instead it should just show a message "No local changes to save", and not create a stash.

Luckily the 'untracked_files()' function already correctly respects pathspecs that are passed to it, so the fix is simply to pass the pathspec along to the function.

Reported-by: Marc Strapetz <marc.strapetz@syntevo.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh                       | 2 +-
 t/t3905-stash-include-untracked.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

On 20.03.2018 00:21, Thomas Gummerer wrote:

Thanks, Thomas. All of my manual tests have been working fine now.

-Marc

On 16.03.2018 21:43, Thomas Gummerer wrote:

Thanks, existing issues are fixed, but cleanup of the stashed files seems to not work properly when stashing a mixture of tracked and untracked files:

$ git init
$ touch file1
$ touch file2
$ git add file1 file2
$ git commit -m "initial import"
$ echo "a" > file1
$ echo "b" > file2
$ touch file3
$ git status --porcelain
  M file1
  M file2
?? file3
$ git stash push -u -- file1 file3
Saved working directory and index state WIP on master: 48fb140 initial 
import
$ git status --porcelain
  M file1
  M file2

file1 and file3 are properly stashed, but file1 still remains modified in the working tree. When instead stashing file1 and file2, results are fine:

$ git stash push -u -- file1 file2 Saved working directory and index state WIP on master: 48fb140 initial import $ git status On branch master nothing to commit, working tree clean

-Marc

On 03/19, Marc Strapetz wrote:

Thanks for the continued testing, and sorry I just can't seem to get this right :/ The problem here was that I misunderstood what 'git ls-files --error-unmatch' does without testing it myself. I'll send v5 in a bit, which will hopefully finally fix all the cases.