Re: [PATCH v4 1/2] stash: reserve exit status 1 for conflicts

[Junio C Hamano <gitster@pobox.com>]

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

> "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Harald Nordgren <haraldnordgren@gmail.com>
>>
>> "git stash apply", "pop" and "branch" exit with status 1 both when
>> applying the stash entry resulted in conflicts and when they fail for
>> other reasons, so callers cannot tell the two apart.
>>
>> Follow the convention of "git merge-tree" and the merge strategies,
>> which exit with status 1 to indicate conflicts and with a different
>> non-zero status for errors: those subcommands now exit with status 1
>> only when applying the stash entry resulted in conflicts, in which
>> case the stash entry is left in place, and exit with status 128, the
>> status die() uses, when they fail for other reasons.  Document the
>> exit statuses.
>>
>> cmd_stash() used to collapse the return values of the subcommand
>> implementations to a boolean.  It now maps negative values, which
>> signal a failure, to 128 and passes everything else through as-is.
>> The only implementations that return a positive value are "apply",
>> "pop" and "branch", which return the value of do_apply_stash():
>> "apply" returns it directly, and "pop" and "branch" drop the stash
>> entry, via do_drop_stash(), which always returns 0, only when the
>> application succeeded.  The positive value is always 1, as
>> do_apply_stash() only returns a positive value when the three-way
>> merge was unclean.
>>
>> Make the convention explicit by introducing enum stash_apply_result
>> with the values STASH_APPLY_CLEAN, STASH_APPLY_CONFLICT and
>> STASH_APPLY_ERROR, and use it for the in-process autostash helpers,
>> too.  They spawn "git stash apply" and can now tell conflicts apart
>> from other failures, e.g. a crash or death by signal of the child,
>> which map to exit statuses above 1.  Since we know the stash entry
>> was saved, tell users so in the error message instead of leaving them
>> wondering what happened to their stashed changes.
>>
>> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
>> ---
>
> The above is on the overly verbose side.  The first two paragraphs
> give enough discussion and the remainder mostly repeats with small
> details sprinkled in, which can probably be shortened to 1/4 of the
> amount of text, but it is OK.

Just for fun, I complained to an AI agent I had nearby with the
above four lines of critique, which spit back the following as a
replacement for the last two paragraphs.

    Update cmd_stash() to map negative return values to 128 while
    passing positive values through.  Formalize this with enum
    stash_apply_result and update the autostash helpers accordingly
    to distinguish conflicts from errors and inform users that their
    stash was preserved.

As I said it is OK already, you do not have to adopt this shortened
version, but I personally think that this level of detail is enough.