{"thread":{"id":"50807","subject":"[PATCH] Make stashing nothing exit 1","startedAt":"2019-03-22T23:12:36Z","lastAt":"2020-04-12T13:08:11Z","messageCount":10,"participants":["Keith Smiley","Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano","Johannes Schindelin","Maksim Odnoletkov","Johannes Sixt","Maxim Mazurok"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"372284","messageId":"01020169a7ad6af3-ad50e2d1-19fb-46eb-b397-759f8d579e8b-000000@eu-west-1.amazonses.com","threadId":"50807","inReplyTo":null,"subject":"[PATCH] Make stashing nothing exit 1","fromName":"Keith Smiley","fromEmail":"keithbsmiley@gmail.com","sentAt":"2019-03-22T23:12:34Z","receivedAt":"2019-03-22T23:12:36Z","isPatch":true,"sender":{"key":"keithbsmiley@gmail.com","avatar":null},"body":"In the case there are no files to stash, but the user asked to stash, we\nshould exit 1 since the stashing failed.\n---\n git-stash.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 789ce2f41d4a3..ca362b1a31277 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -318,7 +318,7 @@ push_stash () {\n \tif no_changes \"$@\"\n \tthen\n \t\tsay \"$(gettext \"No local changes to save\")\"\n-\t\texit 0\n+\t\texit 1\n \tfi\n \n \tgit reflog exists $ref_stash ||\n\n--\nhttps://github.com/git/git/pull/587\n"},{"id":"372299","messageId":"87d0mic9fm.fsf@evledraar.gmail.com","threadId":"50807","inReplyTo":"01020169a7ad6af3-ad50e2d1-19fb-46eb-b397-759f8d579e8b-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-23T07:54:37Z","receivedAt":"2019-03-23T07:54:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Mar 23 2019, Keith Smiley wrote:\n\n> In the case there are no files to stash, but the user asked to stash, we\n> should exit 1 since the stashing failed.\n> ---\n> git-stash.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 789ce2f41d4a3..ca362b1a31277 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -318,7 +318,7 @@ push_stash () {\n> \tif no_changes \"$@\"\n> \tthen\n> \t\tsay \"$(gettext \"No local changes to save\")\"\n> -\t\texit 0\n> +\t\texit 1\n> \tfi\n>\n> \tgit reflog exists $ref_stash ||\n\nThanks for contributing, some points:\n\n * stash is currently (in the 'next' branch) being rewritten in C. It's\n a better move at this point to patch that version, this code is about\n to be deleted.\n\n * This is missing a corresponding test, and skimming the stash manpage\n we should document how these exit codes are supposed to act.\n\n * Shouldn't we do this consistently across all the other sub-commands?\n Trying some of them seems 'push' may be the odd one out, but maybe\n I've missed some (and this would/should be covered by\n tests). I.e. some single test that does a bunch of ops with no\n entries / nothing to stash and asserts exit codes.\n"},{"id":"372320","messageId":"CAPig+cT99t1y8kMdGLF_GU0cvaVm=GKMRJ+xRcgS80rRgdQZDQ@mail.gmail.com","threadId":"50807","inReplyTo":"87d0mic9fm.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-24T03:37:17Z","receivedAt":"2019-03-24T03:37:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 23, 2019 at 3:54 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Sat, Mar 23 2019, Keith Smiley wrote:\n> > In the case there are no files to stash, but the user asked to stash, we\n> > should exit 1 since the stashing failed.\n> > ---\n> > diff --git a/git-stash.sh b/git-stash.sh\n> > @@ -318,7 +318,7 @@ push_stash () {\n> > if no_changes \"$@\"\n> > then\n> > say \"$(gettext \"No local changes to save\")\"\n> > - exit 0\n> > + exit 1\n> > fi\n>\n> * Shouldn't we do this consistently across all the other sub-commands?\n> Trying some of them seems 'push' may be the odd one out, but maybe\n> I've missed some (and this would/should be covered by\n> tests). I.e. some single test that does a bunch of ops with no\n> entries / nothing to stash and asserts exit codes.\n\nA bigger question is why is this change desirable? What is the\njustification for turning this into an error and possibly breaking\nexisting automation scripts? Arguing that this case should be an\n\"error\" is difficult considering that there are many other commands\n(inside and outside of Git) which exit with 0 when they have nothing\nto do. I can't find the message in the archive right now, but I recall\na few months ago Junio shooting down an analogous change to some other\ncommand, so the justification needs to be a strong one.\n\nAlso, your Signed-off-by: is missing. See\nDocumentation/SubmittingPatches. Thanks.\n"},{"id":"372358","messageId":"xmqqh8bsphny.fsf@gitster-ct.c.googlers.com","threadId":"50807","inReplyTo":"01020169a7ad6af3-ad50e2d1-19fb-46eb-b397-759f8d579e8b-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-24T12:42:57Z","receivedAt":"2019-03-24T12:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Smiley writes:\n\n> In the case there are no files to stash, but the user asked to stash, we\n> should exit 1 since the stashing failed.\n> ---\n\nSorry, but I fail to see why this is a good change. Did you have\nsome script that wanted the exit code from \"git stash\" to indicate\nif it had anything to stash and change the behaviour based on it?\n\nIs it a big enough hassle to figure out if the \"stash\" command did\nsomething yourself that justifies forcing existing scripts that rely\non \"no-op is merely a normal exit\" behaviour other people have\nwritten in the past several years?\n\n> git-stash.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 789ce2f41d4a3..ca362b1a31277 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -318,7 +318,7 @@ push_stash () {\n> \tif no_changes \"$@\"\n> \tthen\n> \t\tsay \"$(gettext \"No local changes to save\")\"\n> -\t\texit 0\n> +\t\texit 1\n> \tfi\n> \n> \tgit reflog exists $ref_stash ||\n>\n> --\n> https://github.com/git/git/pull/587\n"},{"id":"372432","messageId":"nycvar.QRO.7.76.6.1903251625250.41@tvgsbejvaqbjf.bet","threadId":"50807","inReplyTo":"CAPig+cT99t1y8kMdGLF_GU0cvaVm=GKMRJ+xRcgS80rRgdQZDQ@mail.gmail.com","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-25T15:29:42Z","receivedAt":"2019-03-25T15:30:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Sat, 23 Mar 2019, Eric Sunshine wrote:\n\n> On Sat, Mar 23, 2019 at 3:54 AM Ævar Arnfjörð Bjarmason\n> wrote:\n> > On Sat, Mar 23 2019, Keith Smiley wrote:\n> > > In the case there are no files to stash, but the user asked to stash, we\n> > > should exit 1 since the stashing failed.\n> > > ---\n> > > diff --git a/git-stash.sh b/git-stash.sh\n> > > @@ -318,7 +318,7 @@ push_stash () {\n> > > if no_changes \"$@\"\n> > > then\n> > > say \"$(gettext \"No local changes to save\")\"\n> > > - exit 0\n> > > + exit 1\n> > > fi\n> >\n> > * Shouldn't we do this consistently across all the other sub-commands?\n> > Trying some of them seems 'push' may be the odd one out, but maybe\n> > I've missed some (and this would/should be covered by\n> > tests). I.e. some single test that does a bunch of ops with no\n> > entries / nothing to stash and asserts exit codes.\n>\n> A bigger question is why is this change desirable?\n\nIndeed. When I run `git stash`, my intention is to make sure that I can\nget back whatever edits I had made, but right now, I want a clean\nworktree.\n\nSo for me, `git stash` does *the exact right thing*.\n\nI could see, however, that other users might think that it is more like a\n\"uh oh, I have modifications that I do not want to commit right now!\nPlease, Git, put all my local changes into a stash\", and when there are\nnot even any changes to stash, they want the command to fail.\n\nHowever, I think that this is not only a change in behavior, but probably\na minor use case compared to what I feel *my* use case is ;-)\n\nAs such, the new behavior should be hidden behind an option (say,\n`--fail-if-clean`).\n\n> What is the justification for turning this into an error and possibly\n> breaking existing automation scripts? Arguing that this case should be\n> an \"error\" is difficult considering that there are many other commands\n> (inside and outside of Git) which exit with 0 when they have nothing to\n> do. I can't find the message in the archive right now, but I recall a\n> few months ago Junio shooting down an analogous change to some other\n> command, so the justification needs to be a strong one.\n\nIndeed, the commit message should make a case for the change. Otherwise,\nit will be less convincing...\n\nCiao,\nJohannes\n\n>\n> Also, your Signed-off-by: is missing. See\n> Documentation/SubmittingPatches. Thanks.\n>\n"},{"id":"376085","messageId":"20190522235705.68452-1-odnoletkov@mail.ru","threadId":"50807","inReplyTo":"xmqqh8bsphny.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Maksim Odnoletkov","fromEmail":"odnoletkov@mail.ru","sentAt":"2019-05-22T23:57:05Z","receivedAt":"2019-05-22T23:58:49Z","isPatch":true,"sender":{"key":"odnoletkov@mail.ru","avatar":"https://gravatar.com/avatar/01d7e7ba42a8bdd4fb31365847af911cf3d01144e5782b0fabae72f2229f30d9?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Keith Smiley writes:\n> \n> > In the case there are no files to stash, but the user asked to stash, we\n> > should exit 1 since the stashing failed.\n> > ---\n> \n> Sorry, but I fail to see why this is a good change. Did you have\n> some script that wanted the exit code from \"git stash\" to indicate\n> if it had anything to stash and change the behaviour based on it?\n> \n> Is it a big enough hassle to figure out if the \"stash\" command did\n> something yourself that justifies forcing existing scripts that rely\n> on \"no-op is merely a normal exit\" behaviour other people have\n> written in the past several years?\n\nThe problem with current behaviour is it makes it hard to use stash in\nscripts. A natural stash use case is: wrap some operation requiring a\nclean working tree with a stash push-pop pair. But that doesn't work\nproperly when working tree is already clean - push silently does nothing\nand following pop becomes unbalanced. You have to keep that in mind and\nwork around with something like:\n\nif ! git diff-index --exit-code --quiet HEAD\nthen\n\tgit stash push\n\ttrap 'git stash pop' EXIT\nfi\n\nWith this change this can be simplified to:\n\ngit stash push && trap 'git stash pop' EXIT\n\nI don't mind keeping this new behaviour behind an option for\ncompatibility. Or alternatively resolve this use case by supporting\n--allow-empty in stash-push. But my feeling is it is natural for 'git\nstash push' to report error for no-op case because the command is\nexplicitly about creating new stash entries. A close analogy is 'git\ncommit' which errors on no-op. Contrary all commands treating no-op as a\nsuccess I'm aware of are not about creating new objects but about\nquerying or syncing.\n"},{"id":"376087","messageId":"871s0quigf.fsf@evledraar.gmail.com","threadId":"50807","inReplyTo":"20190522235705.68452-1-odnoletkov@mail.ru","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-23T00:23:44Z","receivedAt":"2019-05-23T00:23:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 23 2019, Maksim Odnoletkov wrote:\n\n> Junio C Hamano writes:\n>\n>> Keith Smiley writes:\n>>\n>> > In the case there are no files to stash, but the user asked to stash, we\n>> > should exit 1 since the stashing failed.\n>> > ---\n>>\n>> Sorry, but I fail to see why this is a good change. Did you have\n>> some script that wanted the exit code from \"git stash\" to indicate\n>> if it had anything to stash and change the behaviour based on it?\n>>\n>> Is it a big enough hassle to figure out if the \"stash\" command did\n>> something yourself that justifies forcing existing scripts that rely\n>> on \"no-op is merely a normal exit\" behaviour other people have\n>> written in the past several years?\n>\n> The problem with current behaviour is it makes it hard to use stash in\n> scripts. A natural stash use case is: wrap some operation requiring a\n> clean working tree with a stash push-pop pair. But that doesn't work\n> properly when working tree is already clean - push silently does nothing\n> and following pop becomes unbalanced. You have to keep that in mind and\n> work around with something like:\n>\n> if ! git diff-index --exit-code --quiet HEAD\n> then\n> \tgit stash push\n> \ttrap 'git stash pop' EXIT\n> fi\n>\n> With this change this can be simplified to:\n>\n> git stash push && trap 'git stash pop' EXIT\n>\n> I don't mind keeping this new behaviour behind an option for\n> compatibility. Or alternatively resolve this use case by supporting\n> --allow-empty in stash-push. But my feeling is it is natural for 'git\n> stash push' to report error for no-op case because the command is\n> explicitly about creating new stash entries. A close analogy is 'git\n> commit' which errors on no-op. Contrary all commands treating no-op as a\n> success I'm aware of are not about creating new objects but about\n> querying or syncing.\n\nI view \"stash push\" more like just \"push\", or even a special case for\n\"reset --hard\", in both of those cases we don't exit non-zero if there's\nnothing to do, i.e. if there's nothing to push, or if \"reset --hard\"\nends up needing to do nothing.\n\nOn the other hand as you point out it can also be viewed as \"create new\nstash entry\", just like \"create new commit\", and we error out on that.\n\nIn practice I bet there's very few scripters of \"git commit\" that don't\nwant to actually create a commit, whereas \"stash push\" is more likely to\nbe used like \"reset --hard\", i.e. just \"wipe/save-wipe changes if\nneeded\".\n\nI don't mind an --exit-code for it, or even a change in the default\nbehavior, just pointing out that it's a bit more nuanced than just a\nmissing exit code, given \"push\", \"reset\" etc. prior art.\n"},{"id":"376095","messageId":"30d1e067-4184-30e8-bfd2-6ba8da0fc1f5@kdbg.org","threadId":"50807","inReplyTo":"20190522235705.68452-1-odnoletkov@mail.ru","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-05-23T06:14:03Z","receivedAt":"2019-05-23T06:14:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.05.19 um 01:57 schrieb Maksim Odnoletkov:\n> The problem with current behaviour is it makes it hard to use stash in\n> scripts. A natural stash use case is: wrap some operation requiring a\n> clean working tree with a stash push-pop pair. But that doesn't work\n> properly when working tree is already clean - push silently does nothing\n> and following pop becomes unbalanced. You have to keep that in mind and\n> work around with something like:\n> \n> if ! git diff-index --exit-code --quiet HEAD\n> then\n> \tgit stash push\n> \ttrap 'git stash pop' EXIT\n> fi\n> \n> With this change this can be simplified to:\n> \n> git stash push && trap 'git stash pop' EXIT\n\nIn a script, shouldn't you better use 'create' + 'store' instead of 'push'?\n\n-- Hannes\n"},{"id":"376099","messageId":"CAEYVt4pv1JCZf_tXC4S3-D940MfrohEo0+qst73GScm9+5Rbvg@mail.gmail.com","threadId":"50807","inReplyTo":"30d1e067-4184-30e8-bfd2-6ba8da0fc1f5@kdbg.org","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Maksim Odnoletkov","fromEmail":"odnoletkov@mail.ru","sentAt":"2019-05-23T09:49:51Z","receivedAt":"2019-05-23T09:50:07Z","isPatch":true,"sender":{"key":"odnoletkov@mail.ru","avatar":"https://gravatar.com/avatar/01d7e7ba42a8bdd4fb31365847af911cf3d01144e5782b0fabae72f2229f30d9?d=mp&s=160"},"body":"> On 23 May 2019, at 07:14, Johannes Sixt wrote:\n>\n>> Am 23.05.19 um 01:57 schrieb Maksim Odnoletkov:\n>> The problem with current behaviour is it makes it hard to use stash in\n>> scripts. A natural stash use case is: wrap some operation requiring a\n>> clean working tree with a stash push-pop pair. But that doesn't work\n>> properly when working tree is already clean - push silently does nothing\n>> and following pop becomes unbalanced. You have to keep that in mind and\n>> work around with something like:\n>>\n>> if ! git diff-index --exit-code --quiet HEAD\n>> then\n>> git stash push\n>> trap 'git stash pop' EXIT\n>> fi\n>>\n>> With this change this can be simplified to:\n>>\n>> git stash push && trap 'git stash pop' EXIT\n>\n> In a script, shouldn't you better use 'create' + 'store' instead of 'push'?\n>\n> -- Hannes\n\nJust like 'push' 'create' doesn't error on no-op and\ndoesn't create a stash commit – so you still need to handle this\nedge case manually.\n\nI was thinking of using create-apply pair for this use case but push-pop\nhas added benefit of preserving a user-accessible stash entry for manual\nrecovery in case stash can't be cleanly applied after the operation.\n"},{"id":"395293","messageId":"8F6FC77C-1784-4EF1-B005-2C01516A5BEB@mazurok.com","threadId":"50807","inReplyTo":"87d0mic9fm.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Make stashing nothing exit 1","fromName":"Maxim Mazurok","fromEmail":"maxim@mazurok.com","sentAt":"2020-04-12T13:08:01Z","receivedAt":"2020-04-12T13:08:11Z","isPatch":true,"sender":{"key":"maxim@mazurok.com","avatar":null},"body":"> * stash is currently (in the 'next' branch) being rewritten in C. It's\n> a better move at this point to patch that version, this code is about\n> to be deleted.\n>\n> * This is missing a corresponding test, and skimming the stash manpage\n> we should document how these exit codes are supposed to act.\n>\n> * Shouldn't we do this consistently across all the other sub-commands?\n> Trying some of them seems 'push' may be the odd one out, but maybe\n> I've missed some (and this would/should be covered by\n> tests). I.e. some single test that does a bunch of ops with no\n> entries / nothing to stash and asserts exit codes.\n\nI was doing git automation where I’m doing this:\n1. git stash push --keep-index --message all-changes\n2. git stash push --message staged-changed\n3. git checkout -B my_branch master\n4. git checkout stash -- .\n6. git stash drop\n7. git stash pop\nThe 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\n\nSo, I also found that \"git stash push” doesn’t return 1 and I think it’s not in line with “git stash pop” behavior.\n\nIn my workflow it makes harder to find the problem.\n\nAnother problem is that sometimes “git stash” doesn’t seem to work.\nYou can see by downloading full log from here: https://github.com/Maxim-Mazurok/google-api-typings-generator/runs/580274897\nThat 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.\nBecause it only happens sometimes, I assume that it may be because filesystem not flushing all the changes immediately.\nAny help on this would be appreciated.\n\nRegarding “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\n\nRegards,\nMaxim Mazurok"}]}