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

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'.

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

index fc8f8ae640..058ad0bed8 100755
@@ -320,7 +320,7 @@ push_stash () {
 			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
-		if test $# != 0
+		if test $# != 0 && git ls-files --error-unmatch -- "$@" >/dev/null 2>/dev/null
 		then
 			git add -u -- "$@" |
 			git checkout-index -z --force --stdin

index aefde7b172..506004aece 100755
@@ -1096,4 +1096,11 @@ test_expect_success 'stash -- <subdir> works with binary files' '
 	test_path_is_file subdir/untracked
 '
 
+test_expect_success 'stash -- <untracked> doesnt print error' '
+	>untracked &&
+	git stash push -u -- untracked 2>actual&&
+	test_path_is_missing untracked &&
+	test_line_count = 0 actual
+'
+
 test_done

2.16.2.395.g2e18187dfd

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

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?

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

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'.

---

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(-)

index fc8f8ae640..058ad0bed8 100755
@@ -320,7 +320,7 @@ push_stash () {
 			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
-		if test $# != 0
+		if test $# != 0 && git ls-files --error-unmatch -- "$@" >/dev/null 2>/dev/null
 		then
 			git add -u -- "$@" |
 			git checkout-index -z --force --stdin

index aefde7b172..fbfda4b243 100755
@@ -1096,4 +1096,11 @@ test_expect_success 'stash -- <subdir> works with binary files' '
 	test_path_is_file subdir/untracked
 '
 
+test_expect_success 'stash -u -- <untracked> doesnt print error' '
+	>untracked &&
+	git stash push -u -- untracked 2>actual &&
+	test_path_is_missing untracked &&
+	test_line_count = 0 actual
+'
+
 test_done

2.16.2.804.g6dcf76e11

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.

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

index 058ad0bed8..7a4ec98f6b 100755
@@ -39,7 +39,7 @@ fi
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
 	git diff-files --quiet --ignore-submodules -- "$@" &&
-	(test -z "$untracked" || test -z "$(untracked_files)")
+	(test -z "$untracked" || test -z "$(untracked_files $@)")
 }
 
 untracked_files () {

index fbfda4b243..5e7078c083 100755
@@ -1103,4 +1103,10 @@ test_expect_success 'stash -u -- <untracked> doesnt print error' '
 	test_line_count = 0 actual
 '
 
+test_expect_success 'stash -u -- <non-existant> shows no changes when there are none' '
+	git stash push -u -- non-existant >actual &&
+	echo "No local changes to save" >expect &&
+	test_i18ncmp expect actual
+'
+
 test_done

2.16.2.804.g6dcf76e11

Ah yes indeed. Will fix, thanks!

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

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:

index 7a4ec98f6b..dbedc7fb9f 100755
@@ -39,7 +39,7 @@ fi
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
 	git diff-files --quiet --ignore-submodules -- "$@" &&
-	(test -z "$untracked" || test -z "$(untracked_files $@)")
+	(test -z "$untracked" || test -z "$(untracked_files "$@")")
 }
 
 untracked_files () {

@@ -320,11 +320,14 @@ push_stash () {
 			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
-		if test $# != 0 && git ls-files --error-unmatch -- "$@" >/dev/null 2>/dev/null
+		if test $# != 0
 		then
-			git add -u -- "$@" |
-			git checkout-index -z --force --stdin
-			git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
+			if git ls-files --error-unmatch -- "$@" >/dev/null 2>/dev/null
+			then
+				git add -u -- "$@" |
+				git checkout-index -z --force --stdin
+				git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
+			fi
 		else
 			git reset --hard -q
 		fi

index 5e7078c083..7efc52fe11 100755
@@ -1103,6 +1103,15 @@ test_expect_success 'stash -u -- <untracked> doesnt print error' '
 	test_line_count = 0 actual
 '
 
+test_expect_success 'stash -u -- <untracked> leaves rest of working tree in place' '
+	>tracked &&
+	git add tracked &&
+	>untracked &&
+	git stash push -u -- untracked &&
+	test_path_is_missing untracked &&
+	test_path_is_file tracked
+'
+
 test_expect_success 'stash -u -- <non-existant> shows no changes when there are none' '
 	git stash push -u -- non-existant >actual &&
 	echo "No local changes to save" >expect &&

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

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

2.16.2.804.g6dcf76e11

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'.

---
 git-stash.sh     |  9 ++++++---
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

index fc8f8ae640..4de9f9bea8 100755
@@ -322,9 +322,12 @@ push_stash () {
 
 		if test $# != 0
 		then
-			git add -u -- "$@" |
-			git checkout-index -z --force --stdin
-			git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
+			if git ls-files --error-unmatch -- "$@" >/dev/null 2>/dev/null
+			then
+				git add -u -- "$@" |
+				git checkout-index -z --force --stdin
+				git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
+			fi
 		else
 			git reset --hard -q
 		fi

index aefde7b172..8b1a8d2982 100755
@@ -1096,4 +1096,20 @@ test_expect_success 'stash -- <subdir> works with binary files' '
 	test_path_is_file subdir/untracked
 '
 
+test_expect_success 'stash -u -- <untracked> doesnt print error' '
+	>untracked &&
+	git stash push -u -- untracked 2>actual &&
+	test_path_is_missing untracked &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'stash -u -- <untracked> leaves rest of working tree in place' '
+	>tracked &&
+	git add tracked &&
+	>untracked &&
+	git stash push -u -- untracked &&
+	test_path_is_missing untracked &&
+	test_path_is_file tracked
+'
+
 test_done

2.16.2.804.g6dcf76e11

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.

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

index 4de9f9bea8..dbedc7fb9f 100755
@@ -39,7 +39,7 @@ fi
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
 	git diff-files --quiet --ignore-submodules -- "$@" &&
-	(test -z "$untracked" || test -z "$(untracked_files)")
+	(test -z "$untracked" || test -z "$(untracked_files "$@")")
 }
 
 untracked_files () {

index 8b1a8d2982..7efc52fe11 100755
@@ -1112,4 +1112,10 @@ test_expect_success 'stash -u -- <untracked> leaves rest of working tree in plac
 	test_path_is_file tracked
 '
 
+test_expect_success 'stash -u -- <non-existant> shows no changes when there are none' '
+	git stash push -u -- non-existant >actual &&
+	echo "No local changes to save" >expect &&
+	test_i18ncmp expect actual
+'
+
 test_done

2.16.2.804.g6dcf76e11

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(-)

2.17.0-rc0

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/[email protected]/

---
 git-stash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b48b164748..4c92ec931f 100755
@@ -315,9 +315,9 @@ push_stash () {
 
 		if test $# != 0
 		then
-			git add -u -- "$@" |
-			git checkout-index -z --force --stdin
-			git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
+			git add -u -- "$@"
+			git diff-index -p --cached --binary HEAD -- "$@" |
+			git apply --index -R
 		else
 			git reset --hard -q
 		fi

2.17.0-rc0

From: Thomas Gummerer <[email protected]>

'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]

---
 git-stash.sh     |  7 ++++---
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

index 4c92ec931f..fa61151548 100755
@@ -313,13 +313,14 @@ push_stash () {
 			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
-		if test $# != 0
+		if test $# = 0
+		then
+			git reset --hard -q
+		elif git ls-files --error-unmatch -- "$@" >/dev/null 2>&1
 		then
 			git add -u -- "$@"
 			git diff-index -p --cached --binary HEAD -- "$@" |
 			git apply --index -R
-		else
-			git reset --hard -q
 		fi
 
 		if test "$keep_index" = "t" && test -n "$i_tree"

index 1cd85e70e9..f5b102a958 100755
@@ -1003,4 +1003,20 @@ test_expect_success 'stash -- <subdir> works with binary files' '
 	test_path_is_file subdir/untracked
 '
 
+test_expect_success 'stash -u -- <untracked> doesnt print error' '
+	>untracked &&
+	git stash push -u -- untracked 2>actual &&
+	test_path_is_missing untracked &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'stash -u -- <untracked> leaves rest of working tree in place' '
+	>tracked &&
+	git add tracked &&
+	>untracked &&
+	git stash push -u -- untracked &&
+	test_path_is_missing untracked &&
+	test_path_is_file tracked
+'
+
 test_done

2.17.0-rc0

From: Thomas Gummerer <[email protected]>

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.

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

index fa61151548..1652f64b5c 100755
@@ -39,7 +39,7 @@ fi
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
 	git diff-files --quiet --ignore-submodules -- "$@" &&
-	(test -z "$untracked" || test -z "$(untracked_files)")
+	(test -z "$untracked" || test -z "$(untracked_files "$@")")
 }
 
 untracked_files () {

index f5b102a958..7e0b1c248c 100755
@@ -1019,4 +1019,10 @@ test_expect_success 'stash -u -- <untracked> leaves rest of working tree in plac
 	test_path_is_file tracked
 '
 
+test_expect_success 'stash -u -- <non-existant> shows no changes when there are none' '
+	git stash push -u -- non-existant >actual &&
+	echo "No local changes to save" >expect &&
+	test_i18ncmp expect actual
+'
+
 test_done

2.17.0-rc0

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(-)

2.15.1.33.g3165b24a68

From: Junio C Hamano <[email protected]>

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/[email protected]/

---
 git-stash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b48b164748..4c92ec931f 100755
@@ -315,9 +315,9 @@ push_stash () {
 
 		if test $# != 0
 		then
-			git add -u -- "$@" |
-			git checkout-index -z --force --stdin
-			git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
+			git add -u -- "$@"
+			git diff-index -p --cached --binary HEAD -- "$@" |
+			git apply --index -R
 		else
 			git reset --hard -q
 		fi

2.15.1.33.g3165b24a68

'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.

---
 git-stash.sh                       |  6 +++--
 t/t3905-stash-include-untracked.sh | 46 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

index 4c92ec931f..5e06f96da5 100755
@@ -308,14 +308,16 @@ push_stash () {
 	if test -z "$patch_mode"
 	then
 		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
-		if test -n "$untracked"
+		if test -n "$untracked" && test $# = 0
 		then
 			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
 		if test $# != 0
 		then
-			git add -u -- "$@"
+			test -z "$untracked" && UPDATE_OPTION="-u" || UPDATE_OPTION=
+			test "$untracked" = "all" && FORCE_OPTION="--force" || FORCE_OPTION=
+			git add $UPDATE_OPTION $FORCE_OPTION -- "$@"
 			git diff-index -p --cached --binary HEAD -- "$@" |
 			git apply --index -R
 		else

2.15.1.33.g3165b24a68

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 ;-)

	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

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

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.

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.

---
 git-stash.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4e55f278bd..d31924aea3 100755
@@ -310,7 +310,7 @@ push_stash () {
 		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
 		if test -n "$untracked" && test $# = 0
 		then
-			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
+			git clean --force --quiet -d $CLEAN_X_OPTION
 		fi
 
 		if test $# != 0

2.15.1.33.g3165b24a68

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

Thanks.

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.

---
 git-stash.sh                       | 2 +-
 t/t3905-stash-include-untracked.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

index 5e06f96da5..4e55f278bd 100755
@@ -39,7 +39,7 @@ fi
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
 	git diff-files --quiet --ignore-submodules -- "$@" &&
-	(test -z "$untracked" || test -z "$(untracked_files)")
+	(test -z "$untracked" || test -z "$(untracked_files "$@")")
 }
 
 untracked_files () {

index 2f9045553e..3ea5b9bb3f 100755
@@ -274,4 +274,10 @@ test_expect_success 'stash -u -- <ignored> leaves ignored file alone' '
 	test_path_is_file ignored.d/bar
 '
 
+test_expect_success 'stash -u -- <non-existant> shows no changes when there are none' '
+	git stash push -u -- non-existant >actual &&
+	echo "No local changes to save" >expect &&
+	test_i18ncmp expect actual
+'
+
 test_done

2.15.1.33.g3165b24a68

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

-Marc

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

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.