Re: [PATCH] Make stashing nothing exit 1

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 23, 2019, 00:23 UTC
Message-ID: <871s0quigf.fsf@evledraar.gmail.com>
In-Reply-To: <20190522235705.68452-1-odnoletkov@mail.ru>

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.