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

[Phillip Wood <phillip.wood123@gmail.com>]

On 02/09/2026 20:51, Junio C Hamano wrote:
> "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.

I think the analysis in the middle of the third paragraph is useful to 
make it clear that the return paths have been audited correctly. I agree 
the rest could be condensed or cut.

Thanks

Phillip

> 
>> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
>> index 50bb89f483..fc6a9a008c 100644
>> --- a/Documentation/git-stash.adoc
>> +++ b/Documentation/git-stash.adoc
>> @@ -426,6 +426,15 @@ include::includes/cmd-config-section-all.adoc[]
>>   :git-stash: 1
>>   include::config/stash.adoc[]
>>   
>> +EXIT STATUS
>> +-----------
>> +
>> +The `git stash` subcommands exit with status 0 on success.  The
>> +subcommands that apply a stash entry, i.e. `apply`, `pop` and `branch`,
>> +exit with status 1 when applying the stash entry resulted in conflicts,
>> +in which case the stash entry is left in place, and with a non-zero
>> +status other than 1 when they fail for other reasons.
>> +
> 
> Great.
> 
>> +static enum stash_apply_result do_apply_stash(const char *prefix,
>> +					      struct stash_info *info,
>> +					      int index, int quiet,
>> +					      const char *label_ours,
>> +					      const char *label_theirs,
>> +					      const char *label_base)
>>   {
>>   	int clean, ret;
>>   	int has_index = index;
>> @@ -717,8 +720,8 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>>   
>>   	/*
>>   	 * If 'clean' >= 0, reverse the value for 'ret' so 'ret' is 0 when the
>> -	 * merge was clean, and nonzero if the merge was unclean or encountered
>> -	 * an error.
>> +	 * merge was clean, and 1 if the merge was unclean or a negative value
>> +	 * if it encountered an error.
>>   	 */
>>   	ret = clean >= 0 ? !clean : clean;
> 
> OK.
> 
>> +	if (fn) {
>> +		ret = fn(argc, argv, prefix, repo);
>> +
>> +		/*
>> +		 * The subcommand implementations return 0 on success, a
>> +		 * negative value on failure, and STASH_APPLY_CONFLICT
>> +		 * when applying a stash entry resulted in conflicts.
>> +		 * Map failures to 128, the status die() uses, so that
>> +		 * exit status 1 unambiguously indicates conflicts.
>> +		 */
>> +		if (ret < 0)
>> +			return 128;
>> +		return ret;
>> +	} else if (!argc)
>>   		return !!push_stash_unassumed(0, NULL, prefix, repo);
> 
> Style.  Once one of "if", "else if" and "else" cascade gains
> {braches}, others should do so as well.
> 
>> +static enum stash_apply_result apply_save_autostash_oid(const char *stash_oid,
>> +							int attempt_apply,
>> +							const char *label_ours,
>> +							const char *label_theirs,
>> +							const char *label_base,
>> +							const char *stash_msg)
>>   {
>>   	struct child_process child = CHILD_PROCESS_INIT;
>> -	int ret = 0;
>> +	enum stash_apply_result ret = STASH_APPLY_CLEAN;
>>   
>>   	if (attempt_apply) {
>>   		child.git_cmd = 1;
>> @@ -4816,9 +4819,11 @@ static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
>>   			strvec_pushf(&child.args, "--label-base=%s", label_base);
>>   		strvec_push(&child.args, stash_oid);
>>   		ret = run_command(&child);
>> +		if (ret > 1)
>> +			ret = STASH_APPLY_ERROR;
> 
> This kind of code that assigns any random "int" that is returned by
> run_command() to "enum ret" that has much narrower valid value range
> and then makes corrections annoys me a bit.
> 
> One way to do this cleanly might be to make a small helper function
> do_stash_apply(), and use it like so:
> 
> 	if (attempt_apply)
> 		ret = do_stash_apply(stash_oid, label_ours, label_theirs,
> 				     label_base);
> 
> The implementation of do_stash_apply() would be like what you have
> in "if (attempt_apply) {...}" block, perhaps like:
> 
> 	static enum stash_apply_result do_stash_apply(const char *stash_oid,
> 						      const char *label_ours,
> 						      const char *label_theirs,
> 						      const char *label_base)
> 	{
> 		struct child_process child = CHILD_PROCESS_INIT;
> 
> 		child.git_cmd = 1;
> 		...
>                  strvec_push(&child.args, stash_oid);
>                  switch (run_command(&child)) {
> 		case 0: return STASH_APPLY_CLEAN;
> 		case 1: return STASH_APPLY_CONFLICT;
> 		default: return STASH_APPLY_ERROR;
> 		}
> 	}
> 
>> -	if (attempt_apply && !ret)
>> +	if (attempt_apply && ret == STASH_APPLY_CLEAN)
>>   		fprintf(stderr, _("Applied autostash.\n"));
>>   	else {
>>   		struct child_process store = CHILD_PROCESS_INIT;
> 
> Good, and the rest of this function is good.