Git development
 help / color / mirror / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Harald Nordgren via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Harald Nordgren <haraldnordgren@gmail.com>
Subject: Re: [PATCH v4 0/2] checkout -m: refine autostash fallback
Date: Thu, 3 Sep 2026 15:00:27 +0100	[thread overview]
Message-ID: <66283358-9b87-4485-a036-ba91e6d7fcad@gmail.com> (raw)
In-Reply-To: <pull.2364.v4.git.git.1788373743.gitgitgadget@gmail.com>

Hi Harald

On 02/09/2026 19:29, Harald Nordgren via GitGitGadget wrote:
> Avoiding checkout -m autostash retries when no tracked local changes exist
> and visually separating autostash conflict advice from the subsequent
> branch-switch message. Addresses #leftoverbits from here:
> https://lore.kernel.org/git/cfd09dbf-8d77-4464-8030-3a0ffb4aeae7@gmail.com/
> 
> Changes in v4:
> 
>   * Conflicts now exit with status 1 like merge-tree, other failures exit 128
>     so exit 1 unambiguously means conflicts. Stash changes split into their
>     own commit.

Thanks for changing the exit codes, I don't have anything to add to 
Junio's comments.

Thanks

Phillip

>   * The autostash apply helpers use the return value (enum
>     stash_apply_result) instead of an out-parameter, and only claim conflicts
>     when git stash apply actually reported them.
> 
> Changes in v3:
> 
>   * Use enum for git stash return values, to separate conflict from generic
>     error.
> 
> Changes in v2:
> 
>   * Simplify logic and combine to one commit.
>   * Test full output with test_cmp.
> 
> Harald Nordgren (2):
>    stash: reserve exit status 1 for conflicts
>    checkout: separate autostash conflict advice from branch-switch
>      message
> 
>   Documentation/git-stash.adoc |  9 +++++
>   builtin/checkout.c           | 15 ++++----
>   builtin/stash.c              | 32 ++++++++++++-----
>   sequencer.c                  | 66 ++++++++++++++++++++++--------------
>   sequencer.h                  | 19 +++++++----
>   stash.h                      | 21 ++++++++++++
>   t/t3903-stash.sh             | 25 ++++++++++++--
>   t/t7201-co.sh                | 16 ++++++---
>   8 files changed, 149 insertions(+), 54 deletions(-)
>   create mode 100644 stash.h
> 
> 
> base-commit: 1630431f326e15fcde608827b5ff38422528eb59
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2364%2FHaraldNordgren%2Fhn%2Fgit-checkout-m-leftoverbits-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2364/HaraldNordgren/hn/git-checkout-m-leftoverbits-v4
> Pull-Request: https://github.com/git/git/pull/2364
> 
> Range-diff vs v3:
> 
>   1:  8e1979dd6c ! 1:  ff43221802 checkout: separate autostash conflict advice from branch-switch message
>       @@ Metadata
>        Author: Harald Nordgren <haraldnordgren@gmail.com>
>        
>         ## Commit message ##
>       -    checkout: separate autostash conflict advice from branch-switch message
>       +    stash: reserve exit status 1 for conflicts
>        
>       -    "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.
>       +    "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.
>        
>       -    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.
>       +    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>
>        
>       @@ Documentation/git-stash.adoc: include::includes/cmd-config-section-all.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).
>       ++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.
>        +
>         
>         SEE ALSO
>         --------
>        
>       - ## builtin/checkout.c ##
>       -@@ builtin/checkout.c: static int switch_branches(const struct checkout_opts *opts,
>       - 	int flag, writeout_error = 0;
>       - 	int do_merge = 1;
>       - 	int created_autostash = 0;
>       -+	enum stash_apply_result autostash_res = STASH_APPLY_CLEAN;
>       - 	struct strbuf old_commit_shortname = STRBUF_INIT;
>       - 	struct strbuf autostash_msg = STRBUF_INIT;
>       - 	const char *stash_label_base = NULL;
>       -@@ builtin/checkout.c: static int switch_branches(const struct checkout_opts *opts,
>       - 				git_config_push_parameter(cfg.buf);
>       - 				strbuf_release(&cfg);
>       - 			}
>       --			apply_autostash_ref(the_repository,
>       --					    "CHECKOUT_AUTOSTASH_HEAD",
>       --					    new_branch_info->name,
>       --					    "local",
>       --					    stash_label_base,
>       --					    autostash_msg.buf);
>       -+			autostash_res = apply_autostash_ref(the_repository,
>       -+				    "CHECKOUT_AUTOSTASH_HEAD",
>       -+				    new_branch_info->name,
>       -+				    "local",
>       -+				    stash_label_base,
>       -+				    autostash_msg.buf);
>       - 		}
>       - 		if (ret) {
>       - 			branch_info_release(&old_branch_info);
>       -@@ builtin/checkout.c: static int switch_branches(const struct checkout_opts *opts,
>       - 	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
>       - 		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
>       -
>       -+	if (autostash_res == STASH_APPLY_CONFLICT && !opts->quiet)
>       -+		fputc('\n', stderr);
>       - 	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
>       -
>       - 	if (created_autostash) {
>       -
>         ## builtin/stash.c ##
>        @@
>         #include "object-name.h"
>       @@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_t
>         
>        -static int 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)
>        +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)
>       ++					      const char *label_ours,
>       ++					      const char *label_theirs,
>       ++					      const char *label_base)
>         {
>       + 	int clean, ret;
>       + 	int has_index = index;
>        @@ builtin/stash.c: 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
>       + 	 * 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.
>       ++	 * merge was clean, and 1 if the merge was unclean or a negative value
>       ++	 * if it encountered an error.
>         	 */
>       --	ret = clean >= 0 ? !clean : clean;
>       -+	ret = clean >= 0 ? (clean ? STASH_APPLY_CLEAN : STASH_APPLY_CONFLICT)
>       -+			 : clean;
>       + 	ret = clean >= 0 ? !clean : clean;
>         
>       - 	if (ret < 0)
>       - 		rollback_lock_file(&lock);
>       -@@ builtin/stash.c: 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;
>       - 	} else {
>       - 		unstage_changes_unless_new(&c_tree);
>       - 	}
>        @@ builtin/stash.c: int cmd_stash(int argc,
>         	strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
>         		    (uintmax_t)pid);
>       @@ builtin/stash.c: int cmd_stash(int argc,
>        +	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 1;
>       ++			return 128;
>        +		return ret;
>        +	} else if (!argc)
>         		return !!push_stash_unassumed(0, NULL, prefix, repo);
>       @@ sequencer.c: static int apply_save_autostash_oid(const char *stash_oid, int atte
>         			strvec_pushf(&child.args, "--label-base=%s", label_base);
>         		strvec_push(&child.args, stash_oid);
>         		ret = run_command(&child);
>       -+		if (ret && ret != STASH_APPLY_CONFLICT)
>       ++		if (ret > 1)
>        +			ret = STASH_APPLY_ERROR;
>         	}
>         
>       @@ sequencer.c: static int apply_save_autostash_oid(const char *stash_oid, int atte
>         				  "do not want to resolve them now, run \"git reset --hard\" and\n"
>         				  "apply the local changes later by running \"git stash pop\".\n"));
>        +		else if (attempt_apply)
>       -+			ret = error(_("could not apply autostash"));
>       ++			ret = error(_("could not apply autostash; "
>       ++				      "your changes are safe in the stash"));
>         		else
>         			fprintf(stderr,
>         				_("Autostash exists; creating a new stash entry.\n"
>       @@ stash.h (new)
>        +	 * The stash could not be applied because it resulted in
>        +	 * conflicts.  The stash entry is left in place.  The "git stash
>        +	 * apply", "pop" and "branch" subcommands exit with this status
>       -+	 * in this case.
>       ++	 * in this case, mirroring the convention of "git merge-tree" and
>       ++	 * the merge strategies.
>        +	 */
>       -+	STASH_APPLY_CONFLICT = 2,
>       ++	STASH_APPLY_CONFLICT = 1,
>        +
>        +	/* Something went wrong. */
>        +	STASH_APPLY_ERROR = -1,
>       @@ stash.h (new)
>        +#endif /* STASH_H */
>        
>         ## t/t3903-stash.sh ##
>       -@@ t/t3903-stash.sh: test_expect_success 'apply with custom conflict labels' '
>       +@@ t/t3903-stash.sh: test_expect_success 'stash.index=false overridden by --index' '
>       + 	test_cmp expect file
>       + '
>       +
>       +-test_expect_success 'apply with custom conflict labels' '
>       ++test_expect_success 'apply exits 1 on conflicts' '
>       + 	git reset --hard initial &&
>       + 	test_commit label-base conflict-file base-content &&
>         	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_expect_code 1 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
>       @@ t/t3903-stash.sh: test_expect_success 'apply with empty conflict labels' '
>         	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_expect_code 1 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' '
>       ++test_expect_success 'pop exits 1 on conflicts and keeps the stash entry' '
>        +	git reset --hard initial &&
>       -+	test_commit exit-code-base conflict-file base-content &&
>       -+	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 &&
>       ++	echo stashed >file &&
>       ++	git stash push -m pop-stashed &&
>       ++	test_commit pop-upstream file upstream-content &&
>       ++	test_expect_code 1 git stash pop &&
>        +	git stash list >list &&
>       -+	test_grep stashed list
>       ++	test_grep pop-stashed list
>        +'
>        +
>       -+test_expect_success 'pop exits 2 on conflicts and keeps the stash entry' '
>       ++test_expect_success 'stash branch exits with a non-1 status on errors' '
>        +	git reset --hard initial &&
>       -+	test_commit pop-exit-code-base pop-file base-content &&
>       -+	echo stashed >pop-file &&
>       -+	git stash push -m pop-stashed &&
>       -+	test_commit pop-exit-code-upstream pop-file upstream-content &&
>       -+	test_expect_code 2 git stash pop &&
>       ++	echo stashed >file &&
>       ++	git stash push -m branch-stashed &&
>       ++	test_expect_code 128 git stash branch conflicting-branch refs/heads/does-not-exist &&
>        +	git stash list >list &&
>       -+	test_grep pop-stashed list
>       ++	test_grep branch-stashed list
>        +'
>        +
>         test_expect_success 'stash show --include-untracked includes untracked files' '
>         	git reset --hard &&
>         
>       -
>       - ## t/t7201-co.sh ##
>       -@@ t/t7201-co.sh: 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 &&
>       - 	git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual &&
>       - 	sed /^index/d actual >actual.trimmed &&
>       - 	cat >expect <<-EOF &&
>   -:  ---------- > 2:  935fa0a9ae checkout: separate autostash conflict advice from branch-switch message
> 


  parent reply	other threads:[~2026-09-03 14:00 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-25 15:34 [PATCH 0/2] checkout -m: refine autostash fallback Harald Nordgren via GitGitGadget
2026-07-25 15:34 ` [PATCH 1/2] sequencer: teach autostash apply to report conflicts Harald Nordgren via GitGitGadget
2026-08-27 13:01   ` Phillip Wood
2026-08-31 10:18     ` Harald Nordgren
2026-08-31 13:15       ` phillip.wood123
2026-07-25 15:34 ` [PATCH 2/2] checkout -m: refine autostash fallback Harald Nordgren via GitGitGadget
2026-07-28 22:49   ` Junio C Hamano
2026-08-27 13:05   ` Phillip Wood
2026-08-26 19:14 ` [PATCH 0/2] " Junio C Hamano
2026-08-27 13:12   ` Phillip Wood
2026-08-31 12:00 ` [PATCH v2] checkout: print blank line after autostash conflict advice Harald Nordgren via GitGitGadget
2026-08-31 17:19   ` Junio C Hamano
2026-09-01  9:31     ` Phillip Wood
2026-09-01 13:50       ` Junio C Hamano
2026-09-01  9:49 ` [PATCH v3] checkout: separate autostash conflict advice from branch-switch message Harald Nordgren via GitGitGadget
2026-09-01 13:42   ` Phillip Wood
2026-09-01 17:31     ` Junio C Hamano
2026-09-02 18:29 ` [PATCH v4 0/2] checkout -m: refine autostash fallback Harald Nordgren via GitGitGadget
2026-09-02 18:29   ` [PATCH v4 1/2] stash: reserve exit status 1 for conflicts Harald Nordgren via GitGitGadget
2026-09-02 19:51     ` Junio C Hamano
2026-09-02 20:08       ` Junio C Hamano
2026-09-03 13:57       ` Phillip Wood
2026-09-03 14:45         ` Harald Nordgren
2026-09-03 18:42           ` Junio C Hamano
2026-09-03 19:09             ` Harald Nordgren
2026-09-03 19:45               ` Junio C Hamano
2026-09-04  8:16                 ` Harald Nordgren
2026-09-04 15:09                   ` Phillip Wood
2026-09-04 16:42                     ` Harald Nordgren
2026-09-04 15:21                   ` Junio C Hamano
2026-09-02 18:29   ` [PATCH v4 2/2] checkout: separate autostash conflict advice from branch-switch message Harald Nordgren via GitGitGadget
2026-09-02 19:52     ` Junio C Hamano
2026-09-03 14:00   ` Phillip Wood [this message]
2026-09-03 14:39 ` [PATCH v5 0/2] checkout -m: refine autostash fallback Harald Nordgren via GitGitGadget
2026-09-03 14:39   ` [PATCH v5 1/2] stash: reserve exit status 1 for conflicts Harald Nordgren via GitGitGadget
2026-09-03 14:39   ` [PATCH v5 2/2] checkout: separate autostash conflict advice from branch-switch message Harald Nordgren via GitGitGadget
2026-09-03 18:53   ` [PATCH v5 0/2] checkout -m: refine autostash fallback Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=66283358-9b87-4485-a036-ba91e6d7fcad@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=haraldnordgren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox