[PATCH] Make stashing nothing exit 1

[PATCH] Make stashing nothing exit 1

[Keith Smiley]

In the case there are no files to stash, but the user asked to stash, we
should exit 1 since the stashing failed.
---
 git-stash.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index 789ce2f41d4a3..ca362b1a31277 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -318,7 +318,7 @@ push_stash () {
 	if no_changes "$@"
 	then
 		say "$(gettext "No local changes to save")"
-		exit 0
+		exit 1
 	fi
 
 	git reflog exists $ref_stash ||

--
https://github.com/git/git/pull/587

Re: [PATCH] Make stashing nothing exit 1

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 23 2019, Keith Smiley wrote:

> In the case there are no files to stash, but the user asked to stash, we
> should exit 1 since the stashing failed.
> ---
>  git-stash.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 789ce2f41d4a3..ca362b1a31277 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -318,7 +318,7 @@ push_stash () {
>  	if no_changes "$@"
>  	then
>  		say "$(gettext "No local changes to save")"
> -		exit 0
> +		exit 1
>  	fi
>
>  	git reflog exists $ref_stash ||

Thanks for contributing, some points:

 * stash is currently (in the 'next' branch) being rewritten in C. It's
   a better move at this point to patch that version, this code is about
   to be deleted.

 * This is missing a corresponding test, and skimming the stash manpage
   we should document how these exit codes are supposed to act.

 * Shouldn't we do this consistently across all the other sub-commands?
   Trying some of them seems 'push' may be the odd one out, but maybe
   I've missed some (and this would/should be covered by
   tests). I.e. some single test that does a bunch of ops with no
   entries / nothing to stash and asserts exit codes.

Re: [PATCH] Make stashing nothing exit 1

[Eric Sunshine]

On Sat, Mar 23, 2019 at 3:54 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Sat, Mar 23 2019, Keith Smiley wrote:
> > In the case there are no files to stash, but the user asked to stash, we
> > should exit 1 since the stashing failed.
> > ---
> > diff --git a/git-stash.sh b/git-stash.sh
> > @@ -318,7 +318,7 @@ push_stash () {
> >       if no_changes "$@"
> >       then
> >               say "$(gettext "No local changes to save")"
> > -             exit 0
> > +             exit 1
> >       fi
>
>  * Shouldn't we do this consistently across all the other sub-commands?
>    Trying some of them seems 'push' may be the odd one out, but maybe
>    I've missed some (and this would/should be covered by
>    tests). I.e. some single test that does a bunch of ops with no
>    entries / nothing to stash and asserts exit codes.

A bigger question is why is this change desirable? What is the
justification for turning this into an error and possibly breaking
existing automation scripts? Arguing that this case should be an
"error" is difficult considering that there are many other commands
(inside and outside of Git) which exit with 0 when they have nothing
to do. I can't find the message in the archive right now, but I recall
a few months ago Junio shooting down an analogous change to some other
command, so the justification needs to be a strong one.

Also, your Signed-off-by: is missing. See
Documentation/SubmittingPatches.  Thanks.

Re: [PATCH] Make stashing nothing exit 1

[Junio C Hamano]

Keith Smiley <keithbsmiley@gmail.com> writes:

> In the case there are no files to stash, but the user asked to stash, we
> should exit 1 since the stashing failed.
> ---

Sorry, but I fail to see why this is a good change.  Did you have
some script that wanted the exit code from "git stash" to indicate
if it had anything to stash and change the behaviour based on it?

Is it a big enough hassle to figure out if the "stash" command did
something yourself that justifies forcing existing scripts that rely
on "no-op is merely a normal exit" behaviour other people have
written in the past several years?

>  git-stash.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 789ce2f41d4a3..ca362b1a31277 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -318,7 +318,7 @@ push_stash () {
>  	if no_changes "$@"
>  	then
>  		say "$(gettext "No local changes to save")"
> -		exit 0
> +		exit 1
>  	fi
>  
>  	git reflog exists $ref_stash ||
>
> --
> https://github.com/git/git/pull/587

Re: [PATCH] Make stashing nothing exit 1

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]

Hi Eric,

On Sat, 23 Mar 2019, Eric Sunshine wrote:

> On Sat, Mar 23, 2019 at 3:54 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Sat, Mar 23 2019, Keith Smiley wrote:
> > > In the case there are no files to stash, but the user asked to stash, we
> > > should exit 1 since the stashing failed.
> > > ---
> > > diff --git a/git-stash.sh b/git-stash.sh
> > > @@ -318,7 +318,7 @@ push_stash () {
> > >       if no_changes "$@"
> > >       then
> > >               say "$(gettext "No local changes to save")"
> > > -             exit 0
> > > +             exit 1
> > >       fi
> >
> >  * Shouldn't we do this consistently across all the other sub-commands?
> >    Trying some of them seems 'push' may be the odd one out, but maybe
> >    I've missed some (and this would/should be covered by
> >    tests). I.e. some single test that does a bunch of ops with no
> >    entries / nothing to stash and asserts exit codes.
>
> A bigger question is why is this change desirable?

Indeed. When I run `git stash`, my intention is to make sure that I can
get back whatever edits I had made, but right now, I want a clean
worktree.

So for me, `git stash` does *the exact right thing*.

I could see, however, that other users might think that it is more like a
"uh oh, I have modifications that I do not want to commit right now!
Please, Git, put all my local changes into a stash", and when there are
not even any changes to stash, they want the command to fail.

However, I think that this is not only a change in behavior, but probably
a minor use case compared to what I feel *my* use case is ;-)

As such, the new behavior should be hidden behind an option (say,
`--fail-if-clean`).

> What is the justification for turning this into an error and possibly
> breaking existing automation scripts? Arguing that this case should be
> an "error" is difficult considering that there are many other commands
> (inside and outside of Git) which exit with 0 when they have nothing to
> do. I can't find the message in the archive right now, but I recall a
> few months ago Junio shooting down an analogous change to some other
> command, so the justification needs to be a strong one.

Indeed, the commit message should make a case for the change. Otherwise,
it will be less convincing...

Ciao,
Johannes

>
> Also, your Signed-off-by: is missing. See
> Documentation/SubmittingPatches.  Thanks.
>

Re: [PATCH] Make stashing nothing exit 1

[Maksim Odnoletkov]

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

> Keith Smiley <keithbsmiley@gmail.com> writes:
> 
> > In the case there are no files to stash, but the user asked to stash, we
> > should exit 1 since the stashing failed.
> > ---
> 
> Sorry, but I fail to see why this is a good change.  Did you have
> some script that wanted the exit code from "git stash" to indicate
> if it had anything to stash and change the behaviour based on it?
> 
> Is it a big enough hassle to figure out if the "stash" command did
> something yourself that justifies forcing existing scripts that rely
> on "no-op is merely a normal exit" behaviour other people have
> written in the past several years?

The problem with current behaviour is it makes it hard to use stash in
scripts. A natural stash use case is: wrap some operation requiring a
clean working tree with a stash push-pop pair. But that doesn't work
properly when working tree is already clean - push silently does nothing
and following pop becomes unbalanced. You have to keep that in mind and
work around with something like:

if ! git diff-index --exit-code --quiet HEAD
then
	git stash push
	trap 'git stash pop' EXIT
fi

With this change this can be simplified to:

git stash push && trap 'git stash pop' EXIT

I don't mind keeping this new behaviour behind an option for
compatibility. Or alternatively resolve this use case by supporting
--allow-empty in stash-push. But my feeling is it is natural for 'git
stash push' to report error for no-op case because the command is
explicitly about creating new stash entries. A close analogy is 'git
commit' which errors on no-op. Contrary all commands treating no-op as a
success I'm aware of are not about creating new objects but about
querying or syncing.

Re: [PATCH] Make stashing nothing exit 1

[Ævar Arnfjörð Bjarmason]

On Thu, May 23 2019, Maksim Odnoletkov wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Keith Smiley <keithbsmiley@gmail.com> writes:
>>
>> > In the case there are no files to stash, but the user asked to stash, we
>> > should exit 1 since the stashing failed.
>> > ---
>>
>> Sorry, but I fail to see why this is a good change.  Did you have
>> some script that wanted the exit code from "git stash" to indicate
>> if it had anything to stash and change the behaviour based on it?
>>
>> Is it a big enough hassle to figure out if the "stash" command did
>> something yourself that justifies forcing existing scripts that rely
>> on "no-op is merely a normal exit" behaviour other people have
>> written in the past several years?
>
> The problem with current behaviour is it makes it hard to use stash in
> scripts. A natural stash use case is: wrap some operation requiring a
> clean working tree with a stash push-pop pair. But that doesn't work
> properly when working tree is already clean - push silently does nothing
> and following pop becomes unbalanced. You have to keep that in mind and
> work around with something like:
>
> if ! git diff-index --exit-code --quiet HEAD
> then
> 	git stash push
> 	trap 'git stash pop' EXIT
> fi
>
> With this change this can be simplified to:
>
> git stash push && trap 'git stash pop' EXIT
>
> I don't mind keeping this new behaviour behind an option for
> compatibility. Or alternatively resolve this use case by supporting
> --allow-empty in stash-push. But my feeling is it is natural for 'git
> stash push' to report error for no-op case because the command is
> explicitly about creating new stash entries. A close analogy is 'git
> commit' which errors on no-op. Contrary all commands treating no-op as a
> success I'm aware of are not about creating new objects but about
> querying or syncing.

I view "stash push" more like just "push", or even a special case for
"reset --hard", in both of those cases we don't exit non-zero if there's
nothing to do, i.e. if there's nothing to push, or if "reset --hard"
ends up needing to do nothing.

On the other hand as you point out it can also be viewed as "create new
stash entry", just like "create new commit", and we error out on that.

In practice I bet there's very few scripters of "git commit" that don't
want to actually create a commit, whereas "stash push" is more likely to
be used like "reset --hard", i.e. just "wipe/save-wipe changes if
needed".

I don't mind an --exit-code for it, or even a change in the default
behavior, just pointing out that it's a bit more nuanced than just a
missing exit code, given "push", "reset" etc. prior art.

Re: [PATCH] Make stashing nothing exit 1

[Johannes Sixt]

Am 23.05.19 um 01:57 schrieb Maksim Odnoletkov:
> The problem with current behaviour is it makes it hard to use stash in
> scripts. A natural stash use case is: wrap some operation requiring a
> clean working tree with a stash push-pop pair. But that doesn't work
> properly when working tree is already clean - push silently does nothing
> and following pop becomes unbalanced. You have to keep that in mind and
> work around with something like:
> 
> if ! git diff-index --exit-code --quiet HEAD
> then
> 	git stash push
> 	trap 'git stash pop' EXIT
> fi
> 
> With this change this can be simplified to:
> 
> git stash push && trap 'git stash pop' EXIT

In a script, shouldn't you better use 'create' + 'store' instead of 'push'?

-- Hannes

Re: [PATCH] Make stashing nothing exit 1

[Maksim Odnoletkov]

> On 23 May 2019, at 07:14, Johannes Sixt <j6t@kdbg.org> wrote:
>
>> Am 23.05.19 um 01:57 schrieb Maksim Odnoletkov:
>> The problem with current behaviour is it makes it hard to use stash in
>> scripts. A natural stash use case is: wrap some operation requiring a
>> clean working tree with a stash push-pop pair. But that doesn't work
>> properly when working tree is already clean - push silently does nothing
>> and following pop becomes unbalanced. You have to keep that in mind and
>> work around with something like:
>>
>> if ! git diff-index --exit-code --quiet HEAD
>> then
>>    git stash push
>>    trap 'git stash pop' EXIT
>> fi
>>
>> With this change this can be simplified to:
>>
>> git stash push && trap 'git stash pop' EXIT
>
> In a script, shouldn't you better use 'create' + 'store' instead of 'push'?
>
> -- Hannes

Just like 'push' 'create' doesn't error on no-op and
doesn't create a stash commit – so you still need to handle this
edge case manually.

I was thinking of using create-apply pair for this use case but push-pop
has added benefit of preserving a user-accessible stash entry for manual
recovery in case stash can't be cleanly applied after the operation.

Re: [PATCH] Make stashing nothing exit 1

[Maxim Mazurok]

> * stash is currently (in the 'next' branch) being rewritten in C. It's
>   a better move at this point to patch that version, this code is about
>   to be deleted.
>
> * This is missing a corresponding test, and skimming the stash manpage
>   we should document how these exit codes are supposed to act.
>
> * Shouldn't we do this consistently across all the other sub-commands?
>   Trying some of them seems 'push' may be the odd one out, but maybe
>   I've missed some (and this would/should be covered by
>   tests). I.e. some single test that does a bunch of ops with no
>   entries / nothing to stash and asserts exit codes.

I was doing git automation where I’m doing this:
1. git stash push --keep-index --message all-changes
2. git stash push  --message staged-changed
3. git checkout -B my_branch master
4. git checkout stash -- .
6. git stash drop
7. git stash pop
The goal is to do this for every folder and put changes for this folder in it’s own branch. More info here: https://github.com/Maxim-Mazurok/google-api-typings-generator/blob/e4d277b67ee34999e2fea04cba7d06c97af9e6bb/bin/auto-update/index.ts#L57

So, I also found that "git stash push” doesn’t return 1 and I think it’s not in line with “git stash pop” behavior.

In my workflow it makes harder to find the problem.

Another problem is that sometimes “git stash” doesn’t seem to work.
You can see by downloading full log from here: https://github.com/Maxim-Mazurok/google-api-typings-generator/runs/580274897
That at the bottom, when processing gapi.client.youtubereporting it says that both stashes (form step 1 and 2) were created, but then when running “git stash list” it shows only one stash.
Because it only happens sometimes, I assume that it may be because filesystem not flushing all the changes immediately.
Any help on this would be appreciated.

Regarding “git stash” return code, do you think it makes sense to add this to C implementation, create tests and find other potential inconsistencies? I would be happy to help. And it seems like other people also are facing this problem because “git stash” doesn’t fail: https://stackoverflow.com/questions/34114700/git-stash-pop-only-if-successfully-stashed-before

Regards,
Maxim Mazurok