threads / patch / 50807

patchMake stashing nothing exit 1

Subject: [PATCH] Make stashing nothing exit 1

## tl;dr

10 messages between Mar 22, 2019 and Apr 12, 2020. Diffs are folded; open one to read it.

replies: 9people: 8as markdown or json

Keith Smiley· Mar 22, 2019, 23:12 UTC · lore

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

Show changes to git-stash.sh +1 −2

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

Ævar Arnfjörð Bjarmason· Mar 23, 2019, 07:54 UTC · re: Keith Smiley · lore

Re: [PATCH] Make stashing nothing exit 1

On Sat, Mar 23 2019, Keith Smiley wrote:

Show 19 quoted lines

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

Eric Sunshine· Mar 24, 2019, 03:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] Make stashing nothing exit 1

On Sat, Mar 23, 2019 at 3:54 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 18 quoted lines

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

Johannes Schindelin· Mar 25, 2019, 15:29 UTC · re: Eric Sunshine · lore

Re: [PATCH] Make stashing nothing exit 1

Hi Eric,

On Sat, 23 Mar 2019, Eric Sunshine wrote:

Show 22 quoted lines

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

Show 7 quoted lines

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

Maxim Mazurok· Apr 12, 2020, 13:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] Make stashing nothing exit 1

Show 12 quoted lines

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

Junio C Hamano· Mar 24, 2019, 12:42 UTC · re: Keith Smiley · lore

Re: [PATCH] Make stashing nothing exit 1

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?

Show 19 quoted lines

>  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

Maksim Odnoletkov· May 22, 2019, 23:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make stashing nothing exit 1

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

Show 14 quoted lines

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

Ævar Arnfjörð Bjarmason· May 23, 2019, 00:23 UTC · re: Maksim Odnoletkov · lore

Re: [PATCH] Make stashing nothing exit 1

On Thu, May 23 2019, Maksim Odnoletkov wrote:

Show 42 quoted lines

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

Johannes Sixt· May 23, 2019, 06:14 UTC · re: Maksim Odnoletkov · lore

Re: [PATCH] Make stashing nothing exit 1

Am 23.05.19 um 01:57 schrieb Maksim Odnoletkov:

Show 16 quoted lines

> 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

Maksim Odnoletkov· May 23, 2019, 09:49 UTC · re: Johannes Sixt · lore

Re: [PATCH] Make stashing nothing exit 1

Show 23 quoted lines

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