* [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 related [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-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-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
* 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 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
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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).