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

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

[Marc Strapetz]

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

	

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

[Thomas Gummerer]

On 03/03, Marc Strapetz wrote:
> 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.

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

diff --git a/git-stash.sh b/git-stash.sh
index fc8f8ae640..058ad0bed8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b172..506004aece 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

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

[Marc Strapetz]

On 03.03.2018 16:46, Thomas Gummerer wrote:
> On 03/03, Marc Strapetz wrote:
>> 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.
> 
> Thanks for the bug report and the reproduction recipe.  The following
> patch should fix it:

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

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

[Junio C Hamano]

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

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

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?

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

[Marc Strapetz]

On 09.03.2018 23:18, Junio C Hamano wrote:
> Marc Strapetz <marc.strapetz@syntevo.com> writes:
> 
>> 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.
> 
> Soooo..., does it mean that the patch Thomas posted and you
> confirmed trades one issue with another issue with a similar
> graveness?

 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

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

[Thomas Gummerer]

On 03/10, Marc Strapetz wrote:
> On 09.03.2018 23:18, Junio C Hamano wrote:
> >Marc Strapetz <marc.strapetz@syntevo.com> writes:
> >
> >>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.
> >
> >Soooo..., does it mean that the patch Thomas posted and you
> >confirmed trades one issue with another issue with a similar
> >graveness?

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

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

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.

> -Marc

[PATCH v2 1/2] stash push: avoid printing errors

[Thomas Gummerer]

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

> Either way I'll try to address this as soon as I can get some
> time to look at it.

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

diff --git a/git-stash.sh b/git-stash.sh
index fc8f8ae640..058ad0bed8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b172..fbfda4b243 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

[PATCH v2 2/2] stash push -u: don't create empty stash

[Thomas Gummerer]

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

diff --git a/git-stash.sh b/git-stash.sh
index 058ad0bed8..7a4ec98f6b 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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 () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index fbfda4b243..5e7078c083 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

Re: [PATCH v2 1/2] stash push: avoid printing errors

[Marc Strapetz]

On 14.03.2018 22:46, Thomas Gummerer wrote:
> 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>
> ---
> 
>> Either way I'll try to address this as soon as I can get some
>> time to look at it.
> 
> 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!

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

Re: [PATCH v2 2/2] stash push -u: don't create empty stash

[Junio C Hamano]

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

>  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 $@)")

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

Re: [PATCH v2 2/2] stash push -u: don't create empty stash

[Thomas Gummerer]

On 03/15, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> >  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 $@)")
> 
> Don't you need a pair of double-quotes around that $@?

Ah yes indeed.  Will fix, thanks!

Re: [PATCH v2 1/2] stash push: avoid printing errors

[Thomas Gummerer]

On 03/15, Marc Strapetz wrote:
> On 14.03.2018 22:46, Thomas Gummerer wrote:
> >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>
> >---
> >
> >>Either way I'll try to address this as soon as I can get some
> >>time to look at it.
> >
> >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!
> 
> 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.

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 :/

> -Marc
> 
> 
> 
> 

[PATCH v3 0/2] stash push -u -- <pathspec> fixes

[Thomas Gummerer]

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:

diff --git a/git-stash.sh b/git-stash.sh
index 7a4ec98f6b..dbedc7fb9f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5e7078c083..7efc52fe11 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

[PATCH v3 1/2] stash push: avoid printing errors

[Thomas Gummerer]

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

diff --git a/git-stash.sh b/git-stash.sh
index fc8f8ae640..4de9f9bea8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b172..8b1a8d2982 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

[PATCH v3 2/2] stash push -u: don't create empty stash

[Thomas Gummerer]

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

diff --git a/git-stash.sh b/git-stash.sh
index 4de9f9bea8..dbedc7fb9f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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 () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 8b1a8d2982..7efc52fe11 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

Re: [PATCH v3 1/2] stash push: avoid printing errors

[Junio C Hamano]

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

> @@ -322,9 +322,12 @@ push_stash () {
>  
>  		if test $# != 0
>  		then
> -			git add -u -- "$@" |
> -			git checkout-index -z --force --stdin

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.

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

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

> +				git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R

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

> +			fi

[PATCH v4 0/3] stash push -u -- <pathspec> fixes

[Junio C Hamano]

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

[PATCH v4 1/3] stash: fix nonsense pipeline

[Junio C Hamano]

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

diff --git a/git-stash.sh b/git-stash.sh
index b48b164748..4c92ec931f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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

[PATCH v4 2/3] stash push: avoid printing errors

[Junio C Hamano]

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

diff --git a/git-stash.sh b/git-stash.sh
index 4c92ec931f..fa61151548 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 1cd85e70e9..f5b102a958 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

[PATCH v4 3/3] stash push -u: don't create empty stash

[Junio C Hamano]

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

diff --git a/git-stash.sh b/git-stash.sh
index fa61151548..1652f64b5c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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 () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f5b102a958..7e0b1c248c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

Re: [PATCH v4 0/3] stash push -u -- <pathspec> fixes

[Thomas Gummerer]

On 03/16, Junio C Hamano wrote:
> Here is a preliminary fix for an earlier copy-pasto, plus two
> patches from your v3.

Thanks for catching and fixing this!

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

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

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

Re: [PATCH v3 0/2] stash push -u -- <pathspec> fixes

[Marc Strapetz]

On 16.03.2018 21:43, Thomas Gummerer wrote:
> 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.

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

Re: [PATCH v3 0/2] stash push -u -- <pathspec> fixes

[Thomas Gummerer]

On 03/19, Marc Strapetz wrote:
> On 16.03.2018 21:43, Thomas Gummerer wrote:
> >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.
> 
> 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:

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.

> $ 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
> 
> 

[PATCH v5 0/3] stash push -u -- <pathspec> fixes

[Thomas Gummerer]

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

[PATCH v5 1/3] stash: fix nonsense pipeline

[Thomas Gummerer]

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

diff --git a/git-stash.sh b/git-stash.sh
index b48b164748..4c92ec931f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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

[PATCH v5 2/3] stash push: avoid printing errors

[Thomas Gummerer]

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

diff --git a/git-stash.sh b/git-stash.sh
index 4c92ec931f..5e06f96da5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index bfde4057ad..2f9045553e 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -228,4 +228,50 @@ test_expect_success 'stash previously ignored file' '
 	test_path_is_file ignored.d/foo
 '
 
+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_expect_success 'stash -u -- <tracked> <untracked> clears changes in both' '
+	>tracked &&
+	git add tracked &&
+	>untracked &&
+	git stash push -u -- tracked untracked &&
+	test_path_is_missing tracked &&
+	test_path_is_missing untracked
+'
+
+test_expect_success 'stash --all -- <ignored> stashes ignored file' '
+	>ignored.d/bar &&
+	git stash push --all -- ignored.d/bar &&
+	test_path_is_missing ignored.d/bar
+'
+
+test_expect_success 'stash --all -- <tracked> <ignored> clears changes in both' '
+	>tracked &&
+	git add tracked &&
+	>ignored.d/bar &&
+	git stash push --all -- tracked ignored.d/bar &&
+	test_path_is_missing tracked &&
+	test_path_is_missing ignored.d/bar
+'
+
+test_expect_success 'stash -u -- <ignored> leaves ignored file alone' '
+	>ignored.d/bar &&
+	git stash push -u -- ignored.d/bar &&
+	test_path_is_file ignored.d/bar
+'
+
 test_done
-- 
2.15.1.33.g3165b24a68

[PATCH v5 3/3] stash push -u: don't create empty stash

[Thomas Gummerer]

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

diff --git a/git-stash.sh b/git-stash.sh
index 5e06f96da5..4e55f278bd 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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 () {
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index 2f9045553e..3ea5b9bb3f 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -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

Re: [PATCH v5 0/3] stash push -u -- <pathspec> fixes

[Marc Strapetz]

On 20.03.2018 00:21, Thomas Gummerer wrote:
> 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.

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

-Marc

Re: [PATCH v5 2/3] stash push: avoid printing errors

[Junio C Hamano]

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

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

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

> diff --git a/git-stash.sh b/git-stash.sh
> index 4c92ec931f..5e06f96da5 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -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

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.

Re: [PATCH v5 2/3] stash push: avoid printing errors

[Thomas Gummerer]

On 03/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > ...
> > 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.
> 
> 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 ;-)

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.

> > diff --git a/git-stash.sh b/git-stash.sh
> > index 4c92ec931f..5e06f96da5 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -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 -- "$@"

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.

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

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?

> 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 -- "$@"

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. 

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

[PATCH] stash: drop superfluos pathspec parameter (was: Re: [PATCH v5 2/3] stash push: avoid printing errors)

[Thomas Gummerer]

On 03/21, Thomas Gummerer wrote:
>
> 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.

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

diff --git a/git-stash.sh b/git-stash.sh
index 4e55f278bd..d31924aea3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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

Re: [PATCH v5 2/3] stash push: avoid printing errors

[Junio C Hamano]

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

>> > diff --git a/git-stash.sh b/git-stash.sh
>> > index 4c92ec931f..5e06f96da5 100755
>> > --- a/git-stash.sh
>> > +++ b/git-stash.sh
>> > @@ -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 -- "$@"
>
> 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.

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.

Re: [PATCH] stash: drop superfluos pathspec parameter

[Junio C Hamano]

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

> On 03/21, Thomas Gummerer wrote:
>>
>> 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.
>
> 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.

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

Thanks.

>
> 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(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 4e55f278bd..d31924aea3 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -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