git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] Make stashing nothing exit 1
@ 2019-03-22 23:12 Keith Smiley
  2019-03-23  7:54 ` Ævar Arnfjörð Bjarmason
  2019-03-24 12:42 ` Junio C Hamano
  0 siblings, 2 replies; 10+ messages in thread
From: Keith Smiley @ 2019-03-22 23:12 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-03-22 23:12 [PATCH] Make stashing nothing exit 1 Keith Smiley
@ 2019-03-23  7:54 ` Ævar Arnfjörð Bjarmason
  2019-03-24  3:37   ` Eric Sunshine
  2020-04-12 13:08   ` Maxim Mazurok
  2019-03-24 12:42 ` Junio C Hamano
  1 sibling, 2 replies; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-03-23  7:54 UTC (permalink / raw)
  To: Keith Smiley; +Cc: git


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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-03-23  7:54 ` Ævar Arnfjörð Bjarmason
@ 2019-03-24  3:37   ` Eric Sunshine
  2019-03-25 15:29     ` Johannes Schindelin
  2020-04-12 13:08   ` Maxim Mazurok
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Sunshine @ 2019-03-24  3:37 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Keith Smiley, Git List

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-03-22 23:12 [PATCH] Make stashing nothing exit 1 Keith Smiley
  2019-03-23  7:54 ` Ævar Arnfjörð Bjarmason
@ 2019-03-24 12:42 ` Junio C Hamano
  2019-05-22 23:57   ` Maksim Odnoletkov
  1 sibling, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2019-03-24 12:42 UTC (permalink / raw)
  To: Keith Smiley; +Cc: git

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-03-24  3:37   ` Eric Sunshine
@ 2019-03-25 15:29     ` Johannes Schindelin
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2019-03-25 15:29 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, Keith Smiley, Git List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-03-24 12:42 ` Junio C Hamano
@ 2019-05-22 23:57   ` Maksim Odnoletkov
  2019-05-23  0:23     ` Ævar Arnfjörð Bjarmason
  2019-05-23  6:14     ` Johannes Sixt
  0 siblings, 2 replies; 10+ messages in thread
From: Maksim Odnoletkov @ 2019-05-22 23:57 UTC (permalink / raw)
  To: gitster; +Cc: git, keithbsmiley

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-05-22 23:57   ` Maksim Odnoletkov
@ 2019-05-23  0:23     ` Ævar Arnfjörð Bjarmason
  2019-05-23  6:14     ` Johannes Sixt
  1 sibling, 0 replies; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-23  0:23 UTC (permalink / raw)
  To: Maksim Odnoletkov; +Cc: gitster, git, keithbsmiley


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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-05-22 23:57   ` Maksim Odnoletkov
  2019-05-23  0:23     ` Ævar Arnfjörð Bjarmason
@ 2019-05-23  6:14     ` Johannes Sixt
  2019-05-23  9:49       ` Maksim Odnoletkov
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2019-05-23  6:14 UTC (permalink / raw)
  To: Maksim Odnoletkov; +Cc: gitster, git, keithbsmiley

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-05-23  6:14     ` Johannes Sixt
@ 2019-05-23  9:49       ` Maksim Odnoletkov
  0 siblings, 0 replies; 10+ messages in thread
From: Maksim Odnoletkov @ 2019-05-23  9:49 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: gitster, git, keithbsmiley

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Make stashing nothing exit 1
  2019-03-23  7:54 ` Ævar Arnfjörð Bjarmason
  2019-03-24  3:37   ` Eric Sunshine
@ 2020-04-12 13:08   ` Maxim Mazurok
  1 sibling, 0 replies; 10+ messages in thread
From: Maxim Mazurok @ 2020-04-12 13:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Keith Smiley, git

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-04-12 13:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-22 23:12 [PATCH] Make stashing nothing exit 1 Keith Smiley
2019-03-23  7:54 ` Ævar Arnfjörð Bjarmason
2019-03-24  3:37   ` Eric Sunshine
2019-03-25 15:29     ` Johannes Schindelin
2020-04-12 13:08   ` Maxim Mazurok
2019-03-24 12:42 ` Junio C Hamano
2019-05-22 23:57   ` Maksim Odnoletkov
2019-05-23  0:23     ` Ævar Arnfjörð Bjarmason
2019-05-23  6:14     ` Johannes Sixt
2019-05-23  9:49       ` Maksim Odnoletkov

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git