checkout -m: refine autostash fallback

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/[email protected]/

Harald Nordgren (2):
sequencer: teach autostash apply to report conflicts checkout -m: refine autostash fallback

 builtin/checkout.c | 18 ++++++++++++++----
 builtin/commit.c   |  2 +-
 builtin/merge.c    |  6 +++---
 sequencer.c        | 29 +++++++++++++++++++----------
 sequencer.h        |  3 ++-
 t/t7201-co.sh      | 17 ++++++++++++++++-
 6 files changed, 55 insertions(+), 20 deletions(-)

base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2364%2FHaraldNordgren%2Fhn%2Fgit-checkout-m-leftoverbits-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2364/HaraldNordgren/hn/git-checkout-m-leftoverbits-v1
Pull-Request: https://github.com/git/git/pull/2364

gitgitgadget

From: Harald Nordgren <[email protected]>

Add a conflicted parameter to apply_save_autostash_oid() and apply_save_autostash_ref() so callers can learn whether applying the stash resulted in conflicts. Thread the parameter through apply_autostash_ref() and update existing callers to pass NULL.

---
 builtin/checkout.c |  3 ++-
 builtin/commit.c   |  2 +-
 builtin/merge.c    |  6 +++---
 sequencer.c        | 29 +++++++++++++++++++----------
 sequencer.h        |  3 ++-
 5 files changed, 27 insertions(+), 16 deletions(-)

index aee84ca897..72aafa4049 100644
@@ -1242,7 +1242,8 @@ static int switch_branches(const struct checkout_opts *opts,
 					    new_branch_info->name,
 					    "local",
 					    stash_label_base,
-					    autostash_msg.buf);
+					    autostash_msg.buf,
+					    NULL);
 		}
 		if (ret) {
 			branch_info_release(&old_branch_info);

index 28f6174503..d678a81865 100644
@@ -1980,7 +1980,7 @@ int cmd_commit(int argc,
 	}
 
 	apply_autostash_ref(the_repository, "MERGE_AUTOSTASH",
-			    NULL, NULL, NULL, NULL);
+			    NULL, NULL, NULL, NULL, NULL);
 
 cleanup:
 	free_commit_extra_headers(extra);

index 5b46a596f0..cecb8fb716 100644
@@ -538,7 +538,7 @@ static void finish(struct commit *head_commit,
 
 	if (new_head)
 		apply_autostash_ref(the_repository, "MERGE_AUTOSTASH",
-				    NULL, NULL, NULL, NULL);
+				    NULL, NULL, NULL, NULL, NULL);
 	strbuf_release(&reflog_message);
 }
 

@@ -1680,7 +1680,7 @@ int cmd_merge(int argc,
 					  &commit->object.oid,
 					  overwrite_ignore)) {
 			apply_autostash_ref(the_repository, "MERGE_AUTOSTASH",
-					    NULL, NULL, NULL, NULL);
+					    NULL, NULL, NULL, NULL, NULL);
 			ret = 1;
 			goto done;
 		}

@@ -1844,7 +1844,7 @@ int cmd_merge(int argc,
 			fprintf(stderr, _("Merge with strategy %s failed.\n"),
 				use_strategies[0]->name);
 		apply_autostash_ref(the_repository, "MERGE_AUTOSTASH",
-				    NULL, NULL, NULL, NULL);
+				    NULL, NULL, NULL, NULL, NULL);
 		ret = 2;
 		goto done;
 	} else if (best_strategy == wt_strategy)

index 1355a99a09..91a70e39a1 100644
@@ -4748,7 +4748,8 @@ void create_autostash_ref(struct repository *r, const char *refname,
 static int 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)
+				    const char *stash_msg,
+				    bool *conflicted)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 	int ret = 0;

@@ -4783,14 +4784,16 @@ 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) {
+			if (conflicted)
+				*conflicted = true;
 			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"));
-		else
+		} else
 			fprintf(stderr,
 				_("Autostash exists; creating a new stash entry.\n"
 				  "Your changes are safe in the stash.\n"

@@ -4814,7 +4817,7 @@ static int apply_save_autostash(const char *path, int attempt_apply)
 	strbuf_trim(&stash_oid);
 
 	ret = apply_save_autostash_oid(stash_oid.buf, attempt_apply,
-				      NULL, NULL, NULL, NULL);
+				      NULL, NULL, NULL, NULL, NULL);
 
 	unlink(path);
 	strbuf_release(&stash_oid);

@@ -4833,19 +4836,24 @@ int apply_autostash(const char *path)
 
 int apply_autostash_oid(const char *stash_oid)
 {
-	return apply_save_autostash_oid(stash_oid, 1, NULL, NULL, NULL, NULL);
+	return apply_save_autostash_oid(stash_oid, 1, NULL, NULL, NULL, NULL,
+				       NULL);
 }
 
 static int apply_save_autostash_ref(struct repository *r, const char *refname,
 				    int attempt_apply,
 				    const char *label_ours, const char *label_theirs,
 				    const char *label_base,
-				    const char *stash_msg)
+				    const char *stash_msg,
+				    bool *conflicted)
 {
 	struct object_id stash_oid;
 	char stash_oid_hex[GIT_MAX_HEXSZ + 1];
 	int flag, ret;
 
+	if (conflicted)
+		*conflicted = false;
+
 	if (!refs_ref_exists(get_main_ref_store(r), refname))
 		return 0;
 

@@ -4858,7 +4866,7 @@ static int apply_save_autostash_ref(struct repository *r, const char *refname,
 	oid_to_hex_r(stash_oid_hex, &stash_oid);
 	ret = apply_save_autostash_oid(stash_oid_hex, attempt_apply,
 				       label_ours, label_theirs, label_base,
-				       stash_msg);
+				       stash_msg, conflicted);
 
 	refs_delete_ref(get_main_ref_store(r), "", refname,
 			&stash_oid, REF_NO_DEREF);

@@ -4869,16 +4877,17 @@ static int apply_save_autostash_ref(struct repository *r, const char *refname,
 int save_autostash_ref(struct repository *r, const char *refname)
 {
 	return apply_save_autostash_ref(r, refname, 0,
-					NULL, NULL, NULL, NULL);
+					NULL, NULL, NULL, NULL, NULL);
 }
 
 int apply_autostash_ref(struct repository *r, const char *refname,
 			const char *label_ours, const char *label_theirs,
-			const char *label_base, const char *stash_msg)
+			const char *label_base, const char *stash_msg,
+			bool *conflicted)
 {
 	return apply_save_autostash_ref(r, refname, 1,
 					label_ours, label_theirs, label_base,
-					stash_msg);
+					stash_msg, conflicted);
 }
 
 static int checkout_onto(struct repository *r, struct replay_opts *opts,

index 64a9c7fb1b..b39528b6d0 100644
@@ -237,7 +237,8 @@ int apply_autostash(const char *path);
 int apply_autostash_oid(const char *stash_oid);
 int apply_autostash_ref(struct repository *r, const char *refname,
 			const char *label_ours, const char *label_theirs,
-			const char *label_base, const char *stash_msg);
+			const char *label_base, const char *stash_msg,
+			bool *conflicted);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)

gitgitgadget

From: Harald Nordgren <[email protected]>

When unpack_trees() fails under "git checkout -m", only create an autostash and retry if there are tracked local changes. Without such changes, the fallback cannot help and merely repeats the same failure.

Use the conflict result from apply_autostash_ref() to print a blank line before the branch-switch message, visually separating it from the conflict advice.

---
 builtin/checkout.c | 17 +++++++++++++----
 t/t7201-co.sh      | 17 ++++++++++++++++-
 2 files changed, 29 insertions(+), 5 deletions(-)

index 72aafa4049..2e8b2a2348 100644
@@ -838,10 +838,11 @@ static void init_topts(struct unpack_trees_options *topts,
 static int merge_working_tree(const struct checkout_opts *opts,
 			      struct branch_info *old_branch_info,
 			      struct branch_info *new_branch_info,
-			      bool quiet,
+			      bool allow_autostash,
 			      int *writeout_error)
 {
 	int ret;
+	bool can_autostash = false;
 	struct lock_file lock_file = LOCK_INIT;
 	struct tree *new_tree;
 

@@ -888,9 +889,13 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			return 1;
 		}
 
+		if (allow_autostash)
+			can_autostash = has_unstaged_changes(the_repository, 1) ||
+				has_uncommitted_changes(the_repository, 1);
+
 		/* 2-way merge to the new branch */
 		init_topts(&topts, opts->show_progress,
-			   opts->overwrite_ignore, quiet);
+			   opts->overwrite_ignore, can_autostash);
 		init_checkout_metadata(&topts.meta, new_branch_info->refname,
 				       new_branch_info->commit ?
 				       &new_branch_info->commit->object.oid :

@@ -917,7 +922,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		clear_unpack_trees_porcelain(&topts);
 		if (ret == -1) {
 			rollback_lock_file(&lock_file);
-			return MERGE_WORKING_TREE_UNPACK_FAILED;
+			return can_autostash ?
+				MERGE_WORKING_TREE_UNPACK_FAILED : 1;
 		}
 	}
 

@@ -1166,6 +1172,7 @@ static int switch_branches(const struct checkout_opts *opts,
 	int flag, writeout_error = 0;
 	int do_merge = 1;
 	int created_autostash = 0;
+	bool autostash_conflicted = false;
 	struct strbuf old_commit_shortname = STRBUF_INIT;
 	struct strbuf autostash_msg = STRBUF_INIT;
 	const char *stash_label_base = NULL;

@@ -1243,7 +1250,7 @@ static int switch_branches(const struct checkout_opts *opts,
 					    "local",
 					    stash_label_base,
 					    autostash_msg.buf,
-					    NULL);
+					    &autostash_conflicted);
 		}
 		if (ret) {
 			branch_info_release(&old_branch_info);

@@ -1256,6 +1263,8 @@ 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_conflicted && !opts->quiet)
+		fputc('\n', stderr);
 	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
 
 	if (created_autostash) {

index 0ddd1ad7aa..f9696dab36 100755
@@ -240,6 +240,14 @@ test_expect_success 'checkout -m creates a recoverable stash on conflict' '
 	test_grep "git stash drop" actual &&
 	test_grep "git stash pop" actual &&
 	test_grep "The following paths have local changes" actual &&
+	sed -n "/apply the local changes later/,/Switched to branch/p" \
+		actual >separator.actual &&
+	cat >separator.expect <<-EOF &&
+	apply the local changes later by running "git stash pop".
+
+	Switched to branch ${SQ}side${SQ}
+	EOF
+	test_cmp separator.expect separator.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 &&

@@ -262,11 +270,18 @@ test_expect_success 'checkout -m creates a recoverable stash on conflict' '
 	git reset --hard
 '
 
-test_expect_success 'checkout -m which would overwrite untracked file' '
+test_expect_success 'checkout -m only retries untracked-file failure with local changes' '
 	git checkout -f --detach main &&
 	test_commit another-file &&
 	git checkout HEAD^ &&
 	>another-file.t &&
+	test_must_fail env GIT_TRACE2_EVENT="$(pwd)/trace" \
+		git checkout -m @{-1} 2>err &&
+	test_grep "untracked working tree files" err &&
+	grep "\"region_enter\".*\"category\":\"index\",\"label\":\"refresh\"" \
+		trace >refresh.events &&
+	test_line_count = 1 refresh.events &&
+
 	fill 1 2 3 4 5 >one &&
 	test_must_fail git checkout -m @{-1} 2>err &&
 	q_to_tab >expect <<-\EOF &&

gitgitgadget

Thanks. Will queue.

This topic unfortunately has seen no interests from others on the list. Asking Phillip for help, as the leftoverbits comment cited in the cover letter is from him.

Thanks.

Hi Harald

It would be nicer to use the return value to indicate success/error/conflicts rather than adding yet another parameter. Apart from that this looks good.

Thanks

Phillip

Hi Harald

Unfortunately to do that we have to look for local changes before the first call to unpack_trees() so we're trading an occasional inconvenience of an unnecessary stash and unstash for the cost of looking for local changes on every invocation of "git checkout -m". I don't think that is a good trade off, especially as there is no guarantee that stashing the local changes will make unpack_trees() succeed. To do this effectively would require refactoring unpack_trees() to write its error messages to a buffer and return an error flag that indicates all the errors that were encountered. We could then check if the only thing that prevented upack_trees() from succeeding was local changes to files and stash them, or if there are other errors print the error message. I suspect such a change is far from straight forward.

This change is very welcome and could happily be squashed into the first patch as it motivates the changes in it.

That'll teach me to make suggestions for further work when I'm reviewing patches! Thanks for following up on those comments Harald. I've left some comments, in short I think the changes to the error message are a good idea, but I'm not sure sure about the other change in patch 2 though.

Thanks

Phillip

Is the int flexible enough to separate autostash conflict from other errors? Or should we introduce an enum here?

Harald

From: Harald Nordgren <[email protected]>

When "git checkout -m" stashes the user's local changes and then fails to re-apply them because of conflicts, the conflict advice is printed directly on top of the branch-switch message ("Switched to branch ..."), making the two messages hard to tell apart. Print a blank line in between when the stash apply conflicted, so that the advice and the branch-switch message are visually distinct.

To learn whether the stash apply resulted in conflicts, make the autostash apply functions return 1 when the stash could not be applied due to conflicts (and was stored back), while success and error remain 0 and -1 respectively. Checkout can then use the return value to decide whether to print the blank line.

---
checkout -m: refine autostash fallback

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/[email protected]/

Changes in v2:

* Simplify logic and combine to one commit.
* Test full output with test_cmp.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2364%2FHaraldNordgren%2Fhn%2Fgit-checkout-m-leftoverbits-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2364/HaraldNordgren/hn/git-checkout-m-leftoverbits-v2
Pull-Request: https://github.com/git/git/pull/2364

Range-diff vs v1:

 1:  b501b5fcd0 < -:  ---------- sequencer: teach autostash apply to report conflicts
 2:  37becf38c2 ! 1:  b44c53fcf2 checkout -m: refine autostash fallback

@@ Metadata
Author: Harald Nordgren <[email protected]>

## Commit message ##
- checkout -m: refine autostash fallback
+ checkout: print blank line after autostash conflict advice

- When unpack_trees() fails under "git checkout -m", only create an
- autostash and retry if there are tracked local changes. Without such
- changes, the fallback cannot help and merely repeats the same failure.
+ When "git checkout -m" stashes the user's local changes and then fails
+ to re-apply them because of conflicts, the conflict advice is printed
+ directly on top of the branch-switch message ("Switched to branch ..."),
+ making the two messages hard to tell apart. Print a blank line in
+ between when the stash apply conflicted, so that the advice and the
+ branch-switch message are visually distinct.

- Use the conflict result from apply_autostash_ref() to print a blank line
- before the branch-switch message, visually separating it from the
- conflict advice.
+ To learn whether the stash apply resulted in conflicts, make the
+ autostash apply functions return 1 when the stash could not be applied
+ due to conflicts (and was stored back), while success and error remain
+ 0 and -1 respectively. Checkout can then use the return value to decide
+ whether to print the blank line.

## builtin/checkout.c ##
-@@ builtin/checkout.c: static void init_topts(struct unpack_trees_options *topts,
- static int merge_working_tree(const struct checkout_opts *opts,
- struct branch_info *old_branch_info,
- struct branch_info *new_branch_info,
-- bool quiet,
-+ bool allow_autostash,
- int *writeout_error)
- {
- int ret;
-+ bool can_autostash = false;
- struct lock_file lock_file = LOCK_INIT;
- struct tree *new_tree;
-
-@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
- return 1;
- }
-
-+ if (allow_autostash)
-+ can_autostash = has_unstaged_changes(the_repository, 1) ||
-+ has_uncommitted_changes(the_repository, 1);
-+
- /* 2-way merge to the new branch */
- init_topts(&topts, opts->show_progress,
-- opts->overwrite_ignore, quiet);
-+ opts->overwrite_ignore, can_autostash);
- init_checkout_metadata(&topts.meta, new_branch_info->refname,
- new_branch_info->commit ?
- &new_branch_info->commit->object.oid :
-@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
- clear_unpack_trees_porcelain(&topts);
- if (ret == -1) {
- rollback_lock_file(&lock_file);
-- return MERGE_WORKING_TREE_UNPACK_FAILED;
-+ return can_autostash ?
-+ MERGE_WORKING_TREE_UNPACK_FAILED : 1;
- }
- }
-
@@ 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;
-+ bool autostash_conflicted = false;
++ int autostash_res = 0;
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,
- "local",
- stash_label_base,
- autostash_msg.buf,
-- NULL);
-+ &autostash_conflicted);
+ 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_conflicted && !opts->quiet)
++ if (autostash_res == 1 && !opts->quiet)
+ fputc('\n', stderr);
update_refs_for_switch(opts, &old_branch_info, new_branch_info);

if (created_autostash) {

+ ## sequencer.c ##
+@@ sequencer.c: static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
+ if (label_base)
+ strvec_pushf(&child.args, "--label-base=%s", label_base);
+ strvec_push(&child.args, stash_oid);
+- ret = run_command(&child);
++ if (run_command(&child))
++ ret = 1;
+ }
+
+ if (attempt_apply && !ret)
+
## t/t7201-co.sh ##
@@ t/t7201-co.sh: test_expect_success 'checkout -m creates a recoverable stash on conflict' '
- test_grep "git stash drop" actual &&
- test_grep "git stash pop" actual &&
- test_grep "The following paths have local changes" actual &&
-+ sed -n "/apply the local changes later/,/Switched to branch/p" \
-+ actual >separator.actual &&
-+ cat >separator.expect <<-EOF &&
+ 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 separator.expect separator.actual &&
++ 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 &&
-@@ t/t7201-co.sh: test_expect_success 'checkout -m creates a recoverable stash on conflict' '
- git reset --hard
- '
-
--test_expect_success 'checkout -m which would overwrite untracked file' ' -+test_expect_success 'checkout -m only retries untracked-file failure with local changes' '
- git checkout -f --detach main &&
- test_commit another-file &&
- git checkout HEAD^ &&
- >another-file.t &&
-+ test_must_fail env GIT_TRACE2_EVENT="$(pwd)/trace" \
-+ git checkout -m @{-1} 2>err &&
-+ test_grep "untracked working tree files" err &&
-+ grep "\"region_enter\".*\"category\":\"index\",\"label\":\"refresh\"" \
-+ trace >refresh.events &&
-+ test_line_count = 1 refresh.events &&
-+
- fill 1 2 3 4 5 >one &&
- test_must_fail git checkout -m @{-1} 2>err &&
- q_to_tab >expect <<-\EOF &&

 builtin/checkout.c | 15 +++++++++------
 sequencer.c        |  3 ++-
 t/t7201-co.sh      | 16 ++++++++++++----
 3 files changed, 23 insertions(+), 11 deletions(-)

index 55e3a89a85..e4b7383f10 100644
@@ -1166,6 +1166,7 @@ static int switch_branches(const struct checkout_opts *opts,
 	int flag, writeout_error = 0;
 	int do_merge = 1;
 	int created_autostash = 0;
+	int autostash_res = 0;
 	struct strbuf old_commit_shortname = STRBUF_INIT;
 	struct strbuf autostash_msg = STRBUF_INIT;
 	const char *stash_label_base = NULL;

@@ -1237,12 +1238,12 @@ 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);

@@ -1255,6 +1256,8 @@ 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 == 1 && !opts->quiet)
+		fputc('\n', stderr);
 	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
 
 	if (created_autostash) {

index 65afd100d9..5ed9ae86c4 100644
@@ -4815,7 +4815,8 @@ static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
 		if (label_base)
 			strvec_pushf(&child.args, "--label-base=%s", label_base);
 		strvec_push(&child.args, stash_oid);
-		ret = run_command(&child);
+		if (run_command(&child))
+			ret = 1;
 	}
 
 	if (attempt_apply && !ret)

index 0ddd1ad7aa..9ea9462914 100755
@@ -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 &&
 	git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual &&
 	sed /^index/d actual >actual.trimmed &&
 	cat >expect <<-EOF &&

base-commit: c73e85354c275c9d409b26445089bc16940fc527

gitgitgadget

I think a tri-state enum would be clearest - the callers are only interested it "stash applied without conflicts", "stash applied with conflicts" and "something went wrong - could not apply stash"

Thanks

Phillip

	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.
	 */
	ret = clean >= 0 ? !clean : clean;

	if (fn)
		return !!fn(argc, argv, prefix, repo);

correctly, I think this patch needs a bit more work.

(1) Find the values returned by foo_stash() implementations of all

     'git stash foo' subcommands.  Do they follow the 0 == success,
     negative == failure, positive == something else convention?

(2) Update the code above to something like

	if (fn) {
		int ret = fn(argc, argv, prefix, repo);

		if (ret < 0)
			return 1; /* as before */
		if (!ret)
			return 0; /* as before */
		return ret + 1;
	}

      or with whatever necessary tweak to allow the positive "I have
      something to tell the spawning process" case from the bog
      standard and boring 'return error("message");' cases.

(3) document what each of 'git stash foo' signals with its exit status.

For expediency, it may be OK to assume any and all failures from "git stash apply <oid>" come from a conflicted stash application in your first version. If that is what your reviewer recommended, I would agree. But let's help users and future developers (who do not necessarily have to be you) by leaving a note that this code is not doing what it claims to do and needs more work in the code.

Thanks.

I think if the objective of this patch is to tell the caller whether the conflicts message was printed or not then it is correct because the existing code is too caviler about printing that message. We should at least tighten that even if we don't change "git stash" (which I agree we should fix at some point).

	ret = run_command(&child);
	if (ret > 1)
		ret = -1;

would catch run_command() failing and stash dying or being killed by a signal. Then we should change the code below so that it only claims there were conflicts when "ret == 1" and prints a new error message explaining that "git stash apply" failed when "ret == -1"

Thanks

Phillip

From: Harald Nordgren <[email protected]>

"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.

---
checkout -m: refine autostash fallback

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/[email protected]/

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2364%2FHaraldNordgren%2Fhn%2Fgit-checkout-m-leftoverbits-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2364/HaraldNordgren/hn/git-checkout-m-leftoverbits-v3
Pull-Request: https://github.com/git/git/pull/2364

Range-diff vs v2:

1: b44c53fcf2 ! 1: 8e1979dd6c checkout: print blank line after autostash conflict advice
@@ Metadata
Author: Harald Nordgren <[email protected]>

## Commit message ##
- checkout: print blank line after autostash conflict advice
+ checkout: separate autostash conflict advice from branch-switch message

- When "git checkout -m" stashes the user's local changes and then fails
- to re-apply them because of conflicts, the conflict advice is printed
- directly on top of the branch-switch message ("Switched to branch ..."),
- making the two messages hard to tell apart. Print a blank line in
- between when the stash apply conflicted, so that the advice and the
- branch-switch message are visually distinct.
+ "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 learn whether the stash apply resulted in conflicts, make the
- autostash apply functions return 1 when the stash could not be applied
- due to conflicts (and was stored back), while success and error remain
- 0 and -1 respectively. Checkout can then use the return value to decide
- whether to print the blank line.
+ 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.

+ ## Documentation/git-stash.adoc ##
+@@ Documentation/git-stash.adoc: 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).
++
+
+ 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;
-+ int autostash_res = 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,
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 == 1 && !opts->quiet)
++ 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"
+ #include "parse-options.h"
+ #include "refs.h"
++#include "stash.h"
+ #include "lockfile.h"
+ #include "cache-tree.h"
+ #include "unpack-trees.h"
+@@ builtin/stash.c: 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)
+ {
+@@ 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
+- * 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;
+
+ 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);
+
+- 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;
++ } else if (!argc)
+ return !!push_stash_unassumed(0, NULL, prefix, repo);
+
+ /* Assume 'stash push' */
+
## sequencer.c ##
+@@
+ #include "commit.h"
+ #include "sequencer.h"
+ #include "run-command.h"
++#include "stash.h"
+ #include "hook.h"
+ #include "utf8.h"
+ #include "cache-tree.h"
+@@ sequencer.c: void create_autostash_ref(struct repository *r, const char *refname,
+ create_autostash_internal(r, NULL, refname, message, silent);
+ }
+
+-static int 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)
++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;
@@ sequencer.c: static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
- if (label_base)
strvec_pushf(&child.args, "--label-base=%s", label_base);
strvec_push(&child.args, stash_oid);
-- ret = run_command(&child);
-+ if (run_command(&child))
-+ ret = 1;
+ ret = run_command(&child);
++ if (ret && ret != STASH_APPLY_CONFLICT)
++ ret = STASH_APPLY_ERROR;
}

- if (attempt_apply && !ret)
+- if (attempt_apply && !ret)
++ if (attempt_apply && ret == STASH_APPLY_CLEAN)
+ fprintf(stderr, _("Applied autostash.\n"));
+ else {
+ struct child_process store = CHILD_PROCESS_INIT;
+@@ sequencer.c: 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"));
++ else if (attempt_apply)
++ ret = error(_("could not apply autostash"));
+ else
+ fprintf(stderr,
+ _("Autostash exists; creating a new stash entry.\n"
+@@ sequencer.c: static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
+ return ret;
+ }
+
+-static int apply_save_autostash(const char *path, int attempt_apply)
++static enum stash_apply_result apply_save_autostash(const char *path,
++ int attempt_apply)
+ {
+ struct strbuf stash_oid = STRBUF_INIT;
+- int ret = 0;
++ enum stash_apply_result ret = STASH_APPLY_CLEAN;
+
+ if (!read_oneliner(&stash_oid, path,
+ READ_ONELINER_SKIP_IF_EMPTY)) {
+ strbuf_release(&stash_oid);
+- return 0;
++ return STASH_APPLY_CLEAN;
+ }
+ strbuf_trim(&stash_oid);
+
+@@ sequencer.c: static int apply_save_autostash(const char *path, int attempt_apply)
+ return ret;
+ }
+
+-int save_autostash(const char *path)
++enum stash_apply_result save_autostash(const char *path)
+ {
+ return apply_save_autostash(path, 0);
+ }
+
+-int apply_autostash(const char *path)
++enum stash_apply_result apply_autostash(const char *path)
+ {
+ return apply_save_autostash(path, 1);
+ }
+
+-int apply_autostash_oid(const char *stash_oid)
++enum stash_apply_result apply_autostash_oid(const char *stash_oid)
+ {
+ return apply_save_autostash_oid(stash_oid, 1, NULL, NULL, NULL, NULL);
+ }
+
+-static int apply_save_autostash_ref(struct repository *r, const char *refname,
+- int attempt_apply,
+- const char *label_ours, const char *label_theirs,
+- const char *label_base,
+- const char *stash_msg)
++static enum stash_apply_result apply_save_autostash_ref(struct repository *r,
++ const char *refname,
++ int attempt_apply,
++ const char *label_ours,
++ const char *label_theirs,
++ const char *label_base,
++ const char *stash_msg)
+ {
+ struct object_id stash_oid;
+ char stash_oid_hex[GIT_MAX_HEXSZ + 1];
+- int flag, ret;
++ int flag;
++ enum stash_apply_result ret;
+
+ if (!refs_ref_exists(get_main_ref_store(r), refname))
+- return 0;
++ return STASH_APPLY_CLEAN;
+
+ if (!refs_resolve_ref_unsafe(get_main_ref_store(r), refname,
+ RESOLVE_REF_READING, &stash_oid, &flag))
+- return -1;
++ return STASH_APPLY_ERROR;
+ if (flag & REF_ISSYMREF)
+ return error(_("autostash reference is a symref"));
+
+@@ sequencer.c: static int apply_save_autostash_ref(struct repository *r, const char *refname,
+ return ret;
+ }
+
+-int save_autostash_ref(struct repository *r, const char *refname)
++enum stash_apply_result save_autostash_ref(struct repository *r,
++ const char *refname)
+ {
+ return apply_save_autostash_ref(r, refname, 0,
+ NULL, NULL, NULL, NULL);
+ }
+
+-int apply_autostash_ref(struct repository *r, const char *refname,
+- const char *label_ours, const char *label_theirs,
+- const char *label_base, const char *stash_msg)
++enum stash_apply_result apply_autostash_ref(struct repository *r,
++ const char *refname,
++ const char *label_ours,
++ const char *label_theirs,
++ const char *label_base,
++ const char *stash_msg)
+ {
+ return apply_save_autostash_ref(r, refname, 1,
+ label_ours, label_theirs, label_base,
+
+ ## sequencer.h ##
+@@
+
+ #include "strbuf.h"
+ #include "strvec.h"
++#include "stash.h"
+ #include "wt-status.h"
+
+ struct commit;
+@@ sequencer.h: void commit_post_rewrite(struct repository *r,
+ void create_autostash(struct repository *r, const char *path);
+ void create_autostash_ref(struct repository *r, const char *refname,
+ const char *message, bool silent);
+-int save_autostash(const char *path);
+-int save_autostash_ref(struct repository *r, const char *refname);
+-int apply_autostash(const char *path);
+-int apply_autostash_oid(const char *stash_oid);
+-int apply_autostash_ref(struct repository *r, const char *refname,
+- const char *label_ours, const char *label_theirs,
+- const char *label_base, const char *stash_msg);
++enum stash_apply_result save_autostash(const char *path);
++enum stash_apply_result save_autostash_ref(struct repository *r,
++ const char *refname);
++enum stash_apply_result apply_autostash(const char *path);
++enum stash_apply_result apply_autostash_oid(const char *stash_oid);
++enum stash_apply_result apply_autostash_ref(struct repository *r,
++ const char *refname,
++ const char *label_ours,
++ const char *label_theirs,
++ const char *label_base,
++ const char *stash_msg);
+
+ #define SUMMARY_INITIAL_COMMIT (1 << 0)
+ #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
+
+ ## stash.h (new) ##
+@@
++#ifndef STASH_H
++#define STASH_H
++
++enum stash_apply_result {
++ /* The stash was applied cleanly, or there was nothing to apply. */
++ STASH_APPLY_CLEAN = 0,
++
++ /*
++ * 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.
++ */
++ STASH_APPLY_CONFLICT = 2,
++
++ /* Something went wrong. */
++ STASH_APPLY_ERROR = -1,
++};
++
++#endif /* STASH_H */
+
+ ## t/t3903-stash.sh ##
+@@ t/t3903-stash.sh: 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
+@@ t/t3903-stash.sh: 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' '
++ 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 &&
++ git stash list >list &&
++ test_grep stashed list
++'
++
++test_expect_success 'pop exits 2 on conflicts and keeps the stash entry' '
++ 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 &&
++ git stash list >list &&
++ test_grep pop-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' '

 Documentation/git-stash.adoc |  9 +++++
 builtin/checkout.c           | 15 +++++----
 builtin/stash.c              | 27 +++++++++------
 sequencer.c                  | 65 ++++++++++++++++++++++--------------
 sequencer.h                  | 19 +++++++----
 stash.h                      | 20 +++++++++++
 t/t3903-stash.sh             | 26 +++++++++++++--
 t/t7201-co.sh                | 16 ++++++---
 8 files changed, 143 insertions(+), 54 deletions(-)
 create mode 100644 stash.h

index 50bb89f483..3e41ffcf43 100644
@@ -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).
+
 
 SEE ALSO
 --------

index 55e3a89a85..c960f98287 100644
@@ -1166,6 +1166,7 @@ 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;

@@ -1237,12 +1238,12 @@ 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);

@@ -1255,6 +1256,8 @@ 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) {

index 72c52571f8..86c7ac4ffa 100644
@@ -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)
 {

@@ -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;
 
 	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;
 	} 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;
+	} else if (!argc)
 		return !!push_stash_unassumed(0, NULL, prefix, repo);
 
 	/* Assume 'stash push' */

index 64a9c7fb1b..804501b64c 100644
@@ -3,6 +3,7 @@
 
 #include "strbuf.h"
 #include "strvec.h"
+#include "stash.h"
 #include "wt-status.h"
 
 struct commit;

@@ -231,13 +232,17 @@ void commit_post_rewrite(struct repository *r,
 void create_autostash(struct repository *r, const char *path);
 void create_autostash_ref(struct repository *r, const char *refname,
 			  const char *message, bool silent);
-int save_autostash(const char *path);
-int save_autostash_ref(struct repository *r, const char *refname);
-int apply_autostash(const char *path);
-int apply_autostash_oid(const char *stash_oid);
-int apply_autostash_ref(struct repository *r, const char *refname,
-			const char *label_ours, const char *label_theirs,
-			const char *label_base, const char *stash_msg);
+enum stash_apply_result save_autostash(const char *path);
+enum stash_apply_result save_autostash_ref(struct repository *r,
+					   const char *refname);
+enum stash_apply_result apply_autostash(const char *path);
+enum stash_apply_result apply_autostash_oid(const char *stash_oid);
+enum stash_apply_result apply_autostash_ref(struct repository *r,
+					    const char *refname,
+					    const char *label_ours,
+					    const char *label_theirs,
+					    const char *label_base,
+					    const char *stash_msg);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)

new file mode 100644
index 0000000000..bf2f2e1a45
@@ -0,0 +1,20 @@
+#ifndef STASH_H
+#define STASH_H
+
+enum stash_apply_result {
+	/* The stash was applied cleanly, or there was nothing to apply. */
+	STASH_APPLY_CLEAN = 0,
+
+	/*
+	 * 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.
+	 */
+	STASH_APPLY_CONFLICT = 2,
+
+	/* Something went wrong. */
+	STASH_APPLY_ERROR = -1,
+};
+
+#endif /* STASH_H */

index da27a6599a..93e8e98216 100755
@@ -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' '
+	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 &&
+	git stash list >list &&
+	test_grep stashed list
+'
+
+test_expect_success 'pop exits 2 on conflicts and keeps the stash entry' '
+	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 &&
+	git stash list >list &&
+	test_grep pop-stashed list
+'
+
 test_expect_success 'stash show --include-untracked includes untracked files' '
 	git reset --hard &&
 

index 0ddd1ad7aa..9ea9462914 100755
@@ -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 &&
 	git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual &&
 	sed /^index/d actual >actual.trimmed &&
 	cat >expect <<-EOF &&

base-commit: c73e85354c275c9d409b26445089bc16940fc527

gitgitgadget

Hi Harald

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.

;-)

This is half my fault. I should have made it clear that my sample code was merely to illustrate the need and ability to allow the callers to tell general errors and specific failure modes. I agree with you that "1 if X, other non-zero exit if error" would fly much better.

Thanks.

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/[email protected]/

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.
* 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 <[email protected]>

## 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.

@@ 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

gitgitgadget

From: Harald Nordgren <[email protected]>

"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.

---
 Documentation/git-stash.adoc |  9 +++++
 builtin/stash.c              | 32 ++++++++++++-----
 sequencer.c                  | 66 ++++++++++++++++++++++--------------
 sequencer.h                  | 19 +++++++----
 stash.h                      | 21 ++++++++++++
 t/t3903-stash.sh             | 25 ++++++++++++--
 6 files changed, 128 insertions(+), 44 deletions(-)
 create mode 100644 stash.h

index 50bb89f483..fc6a9a008c 100644
@@ -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.
+
 
 SEE ALSO
 --------

index 72c52571f8..d858b7603f 100644
@@ -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,10 +641,12 @@ 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,
-			  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)
 {
 	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;
 

@@ -2492,9 +2495,20 @@ 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);
+
+		/*
+		 * 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);
 
 	/* Assume 'stash push' */

index 64a9c7fb1b..804501b64c 100644
@@ -3,6 +3,7 @@
 
 #include "strbuf.h"
 #include "strvec.h"
+#include "stash.h"
 #include "wt-status.h"
 
 struct commit;

@@ -231,13 +232,17 @@ void commit_post_rewrite(struct repository *r,
 void create_autostash(struct repository *r, const char *path);
 void create_autostash_ref(struct repository *r, const char *refname,
 			  const char *message, bool silent);
-int save_autostash(const char *path);
-int save_autostash_ref(struct repository *r, const char *refname);
-int apply_autostash(const char *path);
-int apply_autostash_oid(const char *stash_oid);
-int apply_autostash_ref(struct repository *r, const char *refname,
-			const char *label_ours, const char *label_theirs,
-			const char *label_base, const char *stash_msg);
+enum stash_apply_result save_autostash(const char *path);
+enum stash_apply_result save_autostash_ref(struct repository *r,
+					   const char *refname);
+enum stash_apply_result apply_autostash(const char *path);
+enum stash_apply_result apply_autostash_oid(const char *stash_oid);
+enum stash_apply_result apply_autostash_ref(struct repository *r,
+					    const char *refname,
+					    const char *label_ours,
+					    const char *label_theirs,
+					    const char *label_base,
+					    const char *stash_msg);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)

new file mode 100644
index 0000000000..14ba4f946d
@@ -0,0 +1,21 @@
+#ifndef STASH_H
+#define STASH_H
+
+enum stash_apply_result {
+	/* The stash was applied cleanly, or there was nothing to apply. */
+	STASH_APPLY_CLEAN = 0,
+
+	/*
+	 * 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, mirroring the convention of "git merge-tree" and
+	 * the merge strategies.
+	 */
+	STASH_APPLY_CONFLICT = 1,
+
+	/* Something went wrong. */
+	STASH_APPLY_ERROR = -1,
+};
+
+#endif /* STASH_H */

index da27a6599a..6529508b06 100755
@@ -1791,13 +1791,13 @@ 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 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

@@ -1809,11 +1809,30 @@ 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 1 git stash apply --label-ours= --label-theirs= &&
 	test_grep "^<<<<<<<$" conflict-file &&
 	test_grep "^>>>>>>>$" conflict-file
 '
 
+test_expect_success 'pop exits 1 on conflicts and keeps the stash entry' '
+	git reset --hard initial &&
+	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 pop-stashed list
+'
+
+test_expect_success 'stash branch exits with a non-1 status on errors' '
+	git reset --hard initial &&
+	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 branch-stashed list
+'
+
 test_expect_success 'stash show --include-untracked includes untracked files' '
 	git reset --hard &&
 

gitgitgadget

From: Harald Nordgren <[email protected]>

"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.

apply_autostash_ref() reports whether applying the stash resulted in conflicts via its enum stash_apply_result return value, so only print the blank line in the conflicted case.

---
 builtin/checkout.c | 15 +++++++++------
 t/t7201-co.sh      | 16 ++++++++++++----
 2 files changed, 21 insertions(+), 10 deletions(-)

index 55e3a89a85..c960f98287 100644
@@ -1166,6 +1166,7 @@ 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;

@@ -1237,12 +1238,12 @@ 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);

@@ -1255,6 +1256,8 @@ 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) {

index 0ddd1ad7aa..9ea9462914 100755
@@ -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 &&
 	git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual &&
 	sed /^index/d actual >actual.trimmed &&
 	cat >expect <<-EOF &&

gitgitgadget

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.

	if (attempt_apply)
		ret = do_stash_apply(stash_oid, label_ours, label_theirs,
				     label_base);

	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;
		}
	}

This iteration looks good to me.

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.

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

Hi Harald

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

Thanks

Phillip

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/[email protected]/

Changes in v5:

* Improve commit messages.
* Create helper do_stash_apply.

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.
* 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              |  33 +++++++---
 sequencer.c                  | 113 ++++++++++++++++++++++-------------
 sequencer.h                  |  19 +++---
 stash.h                      |  21 +++++++
 t/t3903-stash.sh             |  25 +++++++-
 t/t7201-co.sh                |  16 +++--
 8 files changed, 181 insertions(+), 70 deletions(-)
 create mode 100644 stash.h

base-commit: 3cb9185f65410273787f74333cc027d2ea5daada
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2364%2FHaraldNordgren%2Fhn%2Fgit-checkout-m-leftoverbits-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2364/HaraldNordgren/hn/git-checkout-m-leftoverbits-v5
Pull-Request: https://github.com/git/git/pull/2364

Range-diff vs v4:

1: ff43221802 ! 1: fe22b1bfa6 stash: reserve exit status 1 for conflicts

     @@ Commit message
          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.
     +    The only subcommand implementations that can 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.  do_apply_stash() only returns a
     +    positive value when the three-way merge was unclean.  cmd_stash() now
     +    maps negative values to 128 and passes positive values through as the
     +    exit status, so exit status 1 unambiguously indicates conflicts.
     +    enum stash_apply_result makes the convention explicit, and the
     +    autostash helpers use it to tell users that their stashed changes
     +    were saved when applying them fails.
      
          Signed-off-by: Harald Nordgren <[email protected]>
      
     @@ builtin/stash.c: int cmd_stash(int argc,
      +		if (ret < 0)
      +			return 128;
      +		return ret;
     -+	} else if (!argc)
     ++	} else if (!argc) {
       		return !!push_stash_unassumed(0, NULL, prefix, repo);
     ++	}
       
       	/* Assume 'stash push' */
     + 	strvec_push(&args, "push");
      
       ## sequencer.c ##
      @@
     @@ sequencer.c: void create_autostash_ref(struct repository *r, const char *refname
      -				    const char *label_ours, const char *label_theirs,
      -				    const char *label_base,
      -				    const char *stash_msg)
     ++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;
     +-	int ret = 0;
     + 
     +-	if (attempt_apply) {
     +-		child.git_cmd = 1;
     +-		child.no_stdout = 1;
     +-		child.no_stderr = 1;
     +-		strvec_push(&child.args, "stash");
     +-		strvec_push(&child.args, "apply");
     +-		if (label_ours)
     +-			strvec_pushf(&child.args, "--label-ours=%s", label_ours);
     +-		if (label_theirs)
     +-			strvec_pushf(&child.args, "--label-theirs=%s", label_theirs);
     +-		if (label_base)
     +-			strvec_pushf(&child.args, "--label-base=%s", label_base);
     +-		strvec_push(&child.args, stash_oid);
     +-		ret = run_command(&child);
     +-	}
     +-
     +-	if (attempt_apply && !ret)
     ++	child.git_cmd = 1;
     ++	child.no_stdout = 1;
     ++	child.no_stderr = 1;
     ++	strvec_push(&child.args, "stash");
     ++	strvec_push(&child.args, "apply");
     ++	if (label_ours)
     ++		strvec_pushf(&child.args, "--label-ours=%s", label_ours);
     ++	if (label_theirs)
     ++		strvec_pushf(&child.args, "--label-theirs=%s", label_theirs);
     ++	if (label_base)
     ++		strvec_pushf(&child.args, "--label-base=%s", label_base);
     ++	strvec_push(&child.args, stash_oid);
     ++
     ++	switch (run_command(&child)) {
     ++	case 0:
     ++		return STASH_APPLY_CLEAN;
     ++	case STASH_APPLY_CONFLICT:
     ++		return STASH_APPLY_CONFLICT;
     ++	default:
     ++		return STASH_APPLY_ERROR;
     ++	}
     ++}
     ++
      +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;
     -@@ sequencer.c: 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;
     - 	}
     - 
     --	if (attempt_apply && !ret)
     ++
     ++	if (attempt_apply)
     ++		ret = do_stash_apply(stash_oid, label_ours, label_theirs,
     ++				     label_base);
     ++
      +	if (attempt_apply && ret == STASH_APPLY_CLEAN)
       		fprintf(stderr, _("Applied autostash.\n"));
       	else {

2: 935fa0a9ae = 2: d18ff3ea9a checkout: separate autostash conflict advice from branch-switch message

gitgitgadget

From: Harald Nordgren <[email protected]>

"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.

The only subcommand implementations that can 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. do_apply_stash() only returns a positive value when the three-way merge was unclean. cmd_stash() now maps negative values to 128 and passes positive values through as the exit status, so exit status 1 unambiguously indicates conflicts. enum stash_apply_result makes the convention explicit, and the autostash helpers use it to tell users that their stashed changes were saved when applying them fails.

---
 Documentation/git-stash.adoc |   9 +++
 builtin/stash.c              |  33 +++++++---
 sequencer.c                  | 113 ++++++++++++++++++++++-------------
 sequencer.h                  |  19 +++---
 stash.h                      |  21 +++++++
 t/t3903-stash.sh             |  25 +++++++-
 6 files changed, 160 insertions(+), 60 deletions(-)
 create mode 100644 stash.h

index 50bb89f483..fc6a9a008c 100644
@@ -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.
+
 
 SEE ALSO
 --------

index 72c52571f8..7a9843413b 100644
@@ -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,10 +641,12 @@ 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,
-			  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)
 {
 	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;
 

@@ -2492,10 +2495,22 @@ 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);
+
+		/*
+		 * 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);
+	}
 
 	/* Assume 'stash push' */
 	strvec_push(&args, "push");

index 64a9c7fb1b..804501b64c 100644
@@ -3,6 +3,7 @@
 
 #include "strbuf.h"
 #include "strvec.h"
+#include "stash.h"
 #include "wt-status.h"
 
 struct commit;

@@ -231,13 +232,17 @@ void commit_post_rewrite(struct repository *r,
 void create_autostash(struct repository *r, const char *path);
 void create_autostash_ref(struct repository *r, const char *refname,
 			  const char *message, bool silent);
-int save_autostash(const char *path);
-int save_autostash_ref(struct repository *r, const char *refname);
-int apply_autostash(const char *path);
-int apply_autostash_oid(const char *stash_oid);
-int apply_autostash_ref(struct repository *r, const char *refname,
-			const char *label_ours, const char *label_theirs,
-			const char *label_base, const char *stash_msg);
+enum stash_apply_result save_autostash(const char *path);
+enum stash_apply_result save_autostash_ref(struct repository *r,
+					   const char *refname);
+enum stash_apply_result apply_autostash(const char *path);
+enum stash_apply_result apply_autostash_oid(const char *stash_oid);
+enum stash_apply_result apply_autostash_ref(struct repository *r,
+					    const char *refname,
+					    const char *label_ours,
+					    const char *label_theirs,
+					    const char *label_base,
+					    const char *stash_msg);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)

new file mode 100644
index 0000000000..14ba4f946d
@@ -0,0 +1,21 @@
+#ifndef STASH_H
+#define STASH_H
+
+enum stash_apply_result {
+	/* The stash was applied cleanly, or there was nothing to apply. */
+	STASH_APPLY_CLEAN = 0,
+
+	/*
+	 * 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, mirroring the convention of "git merge-tree" and
+	 * the merge strategies.
+	 */
+	STASH_APPLY_CONFLICT = 1,
+
+	/* Something went wrong. */
+	STASH_APPLY_ERROR = -1,
+};
+
+#endif /* STASH_H */

index da27a6599a..6529508b06 100755
@@ -1791,13 +1791,13 @@ 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 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

@@ -1809,11 +1809,30 @@ 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 1 git stash apply --label-ours= --label-theirs= &&
 	test_grep "^<<<<<<<$" conflict-file &&
 	test_grep "^>>>>>>>$" conflict-file
 '
 
+test_expect_success 'pop exits 1 on conflicts and keeps the stash entry' '
+	git reset --hard initial &&
+	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 pop-stashed list
+'
+
+test_expect_success 'stash branch exits with a non-1 status on errors' '
+	git reset --hard initial &&
+	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 branch-stashed list
+'
+
 test_expect_success 'stash show --include-untracked includes untracked files' '
 	git reset --hard &&
 

gitgitgadget

From: Harald Nordgren <[email protected]>

"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.

apply_autostash_ref() reports whether applying the stash resulted in conflicts via its enum stash_apply_result return value, so only print the blank line in the conflicted case.

---
 builtin/checkout.c | 15 +++++++++------
 t/t7201-co.sh      | 16 ++++++++++++----
 2 files changed, 21 insertions(+), 10 deletions(-)

index 55e3a89a85..c960f98287 100644
@@ -1166,6 +1166,7 @@ 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;

@@ -1237,12 +1238,12 @@ 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);

@@ -1255,6 +1256,8 @@ 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) {

index 0ddd1ad7aa..9ea9462914 100755
@@ -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 &&
 	git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual &&
 	sed /^index/d actual >actual.trimmed &&
 	cat >expect <<-EOF &&

gitgitgadget

I would love to have a linter rule for this, since I keep forgetting and it's a waste of time for others to remind me.

Maybe we should copy this over from Linux?
https://github.com/torvalds/linux/blob/v7.2/scripts/checkpatch.pl#L6270

Harald

Does our .clang-format file already cover this?

Both patches look good to me. Thanks. Will queue.

I don't think so. When GitHub CI runs, it doesn't complain about mismatched braces.

Harald

A local "make style" while you develop runs

	git clang-format --style file --diff --extensions c,h

which tells the command to check only the parts of the system that you touched.

I do not think it is used in CI.

It seems to run as part of CI and doesn't catch it then:
'.github/workflows/check-style.yml'

Harald

[Cc'd Karthik for a view on the CI style job]

My recollection is that we made that job never fail because clang-format does not always do a good job of following our style guide and the output is not guaranteed to be stable across different versions. That makes its value debatable as I don't think many people (anyone?) bother checking the output to see what suggestions it made. It would be a lot simpler for contributors if we just devolved style decisions to clang-format so no one had to think about or comment on the style. Whether that tradeoff is worth it depends on how far the output of clang-format deviates from our preferred style.

Thanks

Phillip

I never make any pull request to this project, so I didn't even notice its existence.

My question was more about what is in .clang-format file we already have and if it would have caught this. I was actually hoping that "RemoveBracesLLVM: true" we have at the end, which is explained like so

# Remove optional braces of control statements (if, else, for, and while) # according to the LLVM coding style. This avoids braces on simple # single-statement bodies of statements but keeps braces if one side of # if/else if/.../else cascade has multi-statement body.

would help us, but it only can do a small subset of what we want, i.e., excess braces around both or either if/else body are removed (see [*] below), and it cannot add braces around all the bodies of if/else if.../else cascade only when one of them require them, it seems.

[*] effect of RemoveBracesLLVM.

git clang-format --style file --diff --extensions c,h

index dfd62d4010..c8df287d70 100644
@@ -61,11 +61,10 @@ static void exclude_helpers_from_list(struct string_list *list)
 	size_t i = 0;
 
 	while (i < list->nr) {
-		if (strstr(list->items[i].string, "--")) {
+		if (strstr(list->items[i].string, "--"))
 			unsorted_string_list_delete_item(list, i, 0);
-		} else {
+		else
 			i++;
-		}
 	}
 }
 

make: *** [Makefile:3503: style] Error 1

Thanks for the history! Seems that when linter output is not consistent between versions, then version pinning is necessary.

My professional opinion is that a consistent style, auto-enforceable + auto-fixable, is extremely valuable, because it removes an entire class of code review discussions. I almost never care which style is chosen, as long as it's the same everywhere.

I tried today to apply the full clang-format output and it was A LOT. Doesn't mean it shouldn't be done, but it would need to be done at a carefully chosen time because there will be lots of conflicts on all in-flight topics.

In the best of worlds, the preferred style is exactly what the linter dictates, then these two are equivalent.

Harald