Re: [PATCH v3] checkout: separate autostash conflict advice from branch-switch message

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

Hi Harald

On 01/09/2026 10:49, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> 
> "git checkout -m" stashes the user's local changes when it cannot
> perform the checkout, and then applies the stash.  When applying the
> stash results in conflicts, the advice on how to deal with them is
> printed directly on top of the branch-switch message ("Switched to
> branch ..."), making the two hard to tell apart.  Print a blank line
> in between so that the advice and the branch-switch message are
> visually distinct.
> 
> To make this possible, "git stash apply", "pop" and "branch" now exit
> with status 2 when applying the stash entry resulted in conflicts, in
> which case the stash entry is left in place; other failures exit with
> status 1, as before.  The exit statuses are documented in the "git
> stash" documentation.

Other commands such as merge-tree and merge strategies use 1 to indicate 
conflicts and another non-zero exit code for errors. That matches the 
way grep and diff use the exit code to distinguish differences from 
errors. It is confusing if we start using a different convention here. 
I've left a few comments below, but the exit code is my main concern. It 
would be nice to separate out the stash changes into a separate commit 
as well.

> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
> index 50bb89f483..3e41ffcf43 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 and non-zero
> +on failure.  The subcommands that apply a stash entry, i.e. `apply`,
> +`pop` and `branch`, exit with status 2 when applying the stash entry
> +resulted in conflicts, in which case the stash entry is left in place.
> +Other failures exit with status 1 (usage errors exit with status 129).

Thanks for documenting this, I think we'd be better to avoid giving 
specific exit codes for errors and say "a non-zero exit code other than 
1" unless we have a good way of enforcing that.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 72c52571f8..86c7ac4ffa 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -10,6 +10,7 @@
>   #include "object-name.h"
>   #include "parse-options.h"
>   #include "refs.h"
> +#include "stash.h"
>   #include "lockfile.h"
>   #include "cache-tree.h"
>   #include "unpack-trees.h"
> @@ -640,8 +641,9 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
>   		die(_("could not write index"));
>   }
>   
> -static int do_apply_stash(const char *prefix, struct stash_info *info,
> -			  int index, int quiet,
> +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)

The indentation is strange here

>   {
> @@ -716,11 +718,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>   	clean = merge_ort_nonrecursive(&o, head, merge, merge_base);
>   
>   	/*
> -	 * 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.
> +	 * Translate the value of 'clean' so 'ret' is STASH_APPLY_CLEAN
> +	 * when the merge was clean, STASH_APPLY_CONFLICT when it was
> +	 * unclean, and a negative value if it encountered an error.
>   	 */
> -	ret = clean >= 0 ? !clean : clean;
> +	ret = clean >= 0 ? (clean ? STASH_APPLY_CLEAN : STASH_APPLY_CONFLICT)
> +			 : clean;

Nested ternary operators are not particularly readable, if we stick with 
an exit code of 1 for conflicts the original code does not need to be 
modified.

>   
>   	if (ret < 0)
>   		rollback_lock_file(&lock);
> @@ -739,7 +742,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>   
>   	if (has_index) {
>   		if (reset_tree(&index_tree, 0, 0))
> -			ret = -1;
> +			ret = STASH_APPLY_ERROR;

This seems a bit pointless when we're still returning -1 implicitly 
everywhere else where we have "return error(...).


>   	} else {
>   		unstage_changes_unless_new(&c_tree);
>   	}
> @@ -2492,9 +2495,13 @@ int cmd_stash(int argc,
>   	strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
>   		    (uintmax_t)pid);
>   
> -	if (fn)
> -		return !!fn(argc, argv, prefix, repo);
> -	else if (!argc)
> +	if (fn) {
> +		ret = fn(argc, argv, prefix, repo);
> +
> +		if (ret < 0)
> +			return 1;
> +		return ret;

Looking at the callers of do_apply_stash(), apply_stash() returns the 
result of do_apply_stash(), pop_stash() and branch_stash() return the 
result of do_drop_stash() if do_apply_stash() returns 0. do_drop_stash() 
always returns 0 so we're safe, but that analysis should be in the 
commit message.


> @@ -4832,13 +4837,15 @@ static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
>   		strvec_push(&store.args, stash_oid);
>   		if (run_command(&store))
>   			ret = error(_("cannot store %s"), stash_oid);
> -		else if (attempt_apply)
> +		else if (attempt_apply && ret == STASH_APPLY_CONFLICT)
>   			fprintf(stderr,
>   				_("Your local changes are stashed, however applying them\n"
>   				  "resulted in conflicts.  You can either resolve the conflicts\n"
>   				  "and then discard the stash with \"git stash drop\", or, if you\n"
>   				  "do not want to resolve them now, run \"git reset --hard\" and\n"
>   				  "apply the local changes later by running \"git stash pop\".\n"));

We only print this if we know there were conflicts - good.

> +		else if (attempt_apply)
> +			ret = error(_("could not apply autostash"));

We know we've saved the stash so we should tell the user that we have, 
rather than leaving when wondering what's happened to their stashed changes.

The rest of the changes in this file look good.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index da27a6599a..93e8e98216 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1797,7 +1797,7 @@ test_expect_success 'apply with custom conflict labels' '
>   	echo stashed >conflict-file &&
>   	git stash push -m "stashed" &&
>   	test_commit label-upstream conflict-file upstream-content &&
> -	test_must_fail git -c merge.conflictStyle=diff3 stash apply --label-ours=UP --label-theirs=STASH &&
> +	test_expect_code 2 git -c merge.conflictStyle=diff3 stash apply --label-ours=UP --label-theirs=STASH &&
>   	test_grep "^<<<<<<< UP" conflict-file &&
>   	test_grep "^||||||| Stash base" conflict-file &&
>   	test_grep "^>>>>>>> STASH" conflict-file
> @@ -1809,11 +1809,33 @@ test_expect_success 'apply with empty conflict labels' '
>   	echo stashed >conflict-file &&
>   	git stash push -m "stashed" &&
>   	test_commit empty-label-upstream conflict-file upstream-content &&
> -	test_must_fail git stash apply --label-ours= --label-theirs= &&
> +	test_expect_code 2 git stash apply --label-ours= --label-theirs= &&
>   	test_grep "^<<<<<<<$" conflict-file &&
>   	test_grep "^>>>>>>>$" conflict-file
>   '
>   
> +test_expect_success 'apply exits 2 on conflicts and keeps the stash entry' '

Aren't we testing that above?

> +	git reset --hard initial &&
> +	test_commit exit-code-base conflict-file base-content &&

We've just reset to a known starting point that has paths file and 
other-file, so why do we need to create a new commit in order to stash 
something?

> +	echo stashed >conflict-file &&
> +	git stash push -m stashed &&
> +	test_commit exit-code-upstream conflict-file upstream-content &&
> +	test_expect_code 2 git stash apply &&
> +	git stash list >list &&
> +	test_grep stashed list
> +'
> +
> +test_expect_success 'pop exits 2 on conflicts and keeps the stash entry' '

This is good, we should be checking "stash branch" as well.

> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 0ddd1ad7aa..9ea9462914 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -236,10 +236,18 @@ test_expect_success 'checkout -m creates a recoverable stash on conflict' '
>   	test_must_fail git checkout side 2>stderr &&
>   	test_grep "Your local changes" stderr &&
>   	git checkout -m side >actual 2>&1 &&
> -	test_grep "resulted in conflicts" actual &&
> -	test_grep "git stash drop" actual &&
> -	test_grep "git stash pop" actual &&
> -	test_grep "The following paths have local changes" actual &&
> +	cat >expect <<-EOF &&
> +	Your local changes are stashed, however applying them
> +	resulted in conflicts.  You can either resolve the conflicts
> +	and then discard the stash with "git stash drop", or, if you
> +	do not want to resolve them now, run "git reset --hard" and
> +	apply the local changes later by running "git stash pop".
> +
> +	Switched to branch ${SQ}side${SQ}
> +	The following paths have local changes:
> +	M	one
> +	EOF
> +	test_cmp expect actual &&

Nice, it is much easier to see what we're checking now

Thanks

Phillip