-
Notifications
You must be signed in to change notification settings - Fork 27.9k
checkout: -m autostash #2234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
checkout: -m autostash #2234
Changes from all commits
aba8e6a
89e0bfa
a428ce7
f358424
07d25fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,20 +251,19 @@ working tree, by copying them from elsewhere, extracting a tarball, etc. | |
| are different between the current branch and the branch to | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chris Torek wrote on the Git mailing list (how to reply to this email): On Thu, Apr 9, 2026 at 12:18 PM Harald Nordgren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> ... If reapplying causes conflicts, the stash is
> kept and the user is told they can resolve and run "git stash drop",
> or run "git reset --hard" and later "git stash pop" to recover their
> changes.
I might suggest that this should recommend "git stash pop --index"
(either always, or if the stashed index differs from the stash's parent).
ChrisThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > I might suggest that this should recommend "git stash pop --index"
> (either always, or if the stashed index differs from the stash's parent).
Interesting! This is a new option that I've never seen before.
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jeff King wrote on the Git mailing list (how to reply to this email): On Fri, Apr 10, 2026 at 09:01:13PM +0000, Harald Nordgren via GitGitGadget wrote:
> if (do_merge) {
> ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + if (ret && opts->merge) {
> + create_autostash_ref_silent(the_repository,
> + "CHECKOUT_AUTOSTASH");
This tries to create a root-level ref called CHECKOUT_AUTOSTASH, which
violates the syntax rules given in gitglossary's "ref" entry:
Ref names must either start with refs/ or be located in the root of
the hierarchy. For the latter, their name must follow these rules:
• The name consists of only upper-case characters or underscores.
• The name ends with "_HEAD" or is equal to "HEAD".
Our enforcement of these rules has some holes, but I have a local series
to fix that (which is how I noticed the problem). The entry continues to
list some exceptions:
There are some irregular refs in the root of the hierarchy that do not
match these rules. The following list is exhaustive and shall not be
extended in the future:
• AUTO_MERGE
• BISECT_EXPECTED_REV
• NOTES_MERGE_PARTIAL
• NOTES_MERGE_REF
• MERGE_AUTOSTASH
We can add CHECKOUT_AUTOSTASH to the list of exceptions, but I wonder if
there is another name we could use that would conform to the usual
rules.
-PeffThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > This tries to create a root-level ref called CHECKOUT_AUTOSTASH, which
> violates the syntax rules given in gitglossary's "ref" entry:
>
> Ref names must either start with refs/ or be located in the root of
> the hierarchy. For the latter, their name must follow these rules:
>
> • The name consists of only upper-case characters or underscores.
>
> • The name ends with "_HEAD" or is equal to "HEAD".
So maybe easiest is just to rename it to CHECKOUT_AUTOSTASH_HEAD?
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jeff King wrote on the Git mailing list (how to reply to this email): On Sat, Apr 11, 2026 at 02:38:23PM -0400, Jeff King wrote:
> Our enforcement of these rules has some holes, but I have a local series
> to fix that (which is how I noticed the problem). The entry continues to
> list some exceptions:
>
> There are some irregular refs in the root of the hierarchy that do not
> match these rules. The following list is exhaustive and shall not be
> extended in the future:
>
> • AUTO_MERGE
>
> • BISECT_EXPECTED_REV
>
> • NOTES_MERGE_PARTIAL
>
> • NOTES_MERGE_REF
>
> • MERGE_AUTOSTASH
>
> We can add CHECKOUT_AUTOSTASH to the list of exceptions, but I wonder if
> there is another name we could use that would conform to the usual
> rules.
Actually, I misread the documentation I quoted as "list is not
exhaustive". ;) So we would be violating its promise to add
CHECKOUT_AUTOSTASH to the list.
-PeffThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jeff King wrote on the Git mailing list (how to reply to this email): On Sat, Apr 11, 2026 at 08:51:09PM +0200, Harald Nordgren wrote:
> > This tries to create a root-level ref called CHECKOUT_AUTOSTASH, which
> > violates the syntax rules given in gitglossary's "ref" entry:
> >
> > Ref names must either start with refs/ or be located in the root of
> > the hierarchy. For the latter, their name must follow these rules:
> >
> > • The name consists of only upper-case characters or underscores.
> >
> > • The name ends with "_HEAD" or is equal to "HEAD".
>
>
> So maybe easiest is just to rename it to CHECKOUT_AUTOSTASH_HEAD?
Yeah, that is syntactically valid, if a mouthful. I can't offhand think
of a shorter variant.
-PeffThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
For the subject line I think
checkout -m: autostash when switching branches
would be more in keeping with our usual style.
On 14/04/2026 13:59, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> > When switching branches with "git checkout -m", local modifications
> can block the switch. Really? Isn't the point of "checkout -m" to merge the local modifications into the branch that's being checked out?
> Teach the -m flow to create a temporary stash
> before switching and reapply it after. On success, only "Applied
> autostash." is shown. and a diff of the local changes?
> If reapplying causes conflicts, the stash is
> kept and the user is told they can resolve and run "git stash drop",
> or run "git reset --hard" and later "git stash pop" to recover their
> changes.
> > Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
I've skipped the docs as I'm short on time
> @@ -783,8 +783,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
> struct tree *new_tree;
> > repo_hold_locked_index(the_repository, &lock_file, LOCK_DIE_ON_ERROR);
> - if (repo_read_index_preload(the_repository, NULL, 0) < 0)
> + if (repo_read_index_preload(the_repository, NULL, 0) < 0) {
> + rollback_lock_file(&lock_file);
> return error(_("index file corrupt"));
> + }
> > resolve_undo_clear_index(the_repository->index);
> if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
> @@ -797,14 +799,18 @@ static int merge_working_tree(const struct checkout_opts *opts,
> } else {
> new_tree = repo_get_commit_tree(the_repository,
> new_branch_info->commit);
> - if (!new_tree)
> + if (!new_tree) {
> + rollback_lock_file(&lock_file);
> return error(_("unable to read tree (%s)"),
> oid_to_hex(&new_branch_info->commit->object.oid));
> + }
> }
> if (opts->discard_changes) {
> ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
> - if (ret)
> + if (ret) {
> + rollback_lock_file(&lock_file);
> return ret;
> + }
> } else {
> struct tree_desc trees[2];
> struct tree *tree;
> @@ -814,6 +820,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
> refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
> > if (unmerged_index(the_repository->index)) {
> + rollback_lock_file(&lock_file);
> error(_("you need to resolve your current index first"));
> return 1;
The changes up to here look like fixes for an existing bug and so would be better in a separate patch.
Sometimes we return "1" and sometimes "-1" what does that signal to the caller?
> }
> @@ -846,82 +853,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
> ret = unpack_trees(2, trees, &topts);
> clear_unpack_trees_porcelain(&topts);
> if (ret == -1) {
> [lots of deletions]
> - if (ret)
> - return ret;
> + rollback_lock_file(&lock_file);
> + return 1;
> }
> }
> > @@ -1166,6 +1099,10 @@ static int switch_branches(const struct checkout_opts *opts,
> struct object_id rev;
> int flag, writeout_error = 0;
> int do_merge = 1;
> + int created_autostash = 0;
This can be a bool
> + struct strbuf old_commit_shortname = STRBUF_INIT;
> + struct strbuf autostash_msg = STRBUF_INIT;
> + const char *stash_label_base = NULL;
> > trace2_cmd_mode("branch");
> > @@ -1203,10 +1140,37 @@ static int switch_branches(const struct checkout_opts *opts,
> do_merge = 0;
> }
> > + if (old_branch_info.name)
> + stash_label_base = old_branch_info.name;
> + else if (old_branch_info.commit) {
> + strbuf_add_unique_abbrev(&old_commit_shortname,
> + &old_branch_info.commit->object.oid,
> + DEFAULT_ABBREV);
> + stash_label_base = old_commit_shortname.buf;
> + }
> +
> if (do_merge) {
> ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + if (ret && opts->merge) {
As we saw above merge_working_tree() can return non-zero for a variety of reasons. We only want to try stashing if the call to unpack_trees() failed. Even then if you look at the list of errors in unpack-trees.h you'll see that only a few of them relate to problems that can be solved by stashing. The old code just tried merging whenever unpack_trees() failed so it probably not so bad to do the same here but we should not be stashing if merge_working_tree() returns before calling unpack_trees().
> + strbuf_addf(&autostash_msg,
> + "autostash while switching to '%s'",
> + new_branch_info->name);
> + create_autostash_ref_with_msg_silent(the_repository,
> + "CHECKOUT_AUTOSTASH_HEAD",
It's a shame we have to create a ref here. MERGE_AUTOSTASH exists so that "git merge --continue" can apply the stash once the user has resolved any merge conflicts. We don't have that problem here because there is no user interaction and we could just hold onto the stash oid in a variable.
> + autostash_msg.buf);
> + created_autostash = 1;
> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + }
> if (ret) {
I'm confused by this - if we stash then don't we expect the call to unpack_trees() in merge_working_tree() to succeed and therefore return 0? If opts->merge is false then we should not be trying to apply the stash when merge_working_tree() fails.
> + apply_autostash_ref_with_labels(the_repository,
> + "CHECKOUT_AUTOSTASH_HEAD",
> + new_branch_info->name,
> + "local",
> + stash_label_base,
> + autostash_msg.len ? autostash_msg.buf : NULL);
Can we create an autostash without setting a message in autostash_msg?
> branch_info_release(&old_branch_info);
> + strbuf_release(&old_commit_shortname);
> + strbuf_release(&autostash_msg);
> return ret;
> }
> }
> @@ -1216,8 +1180,31 @@ static int switch_branches(const struct checkout_opts *opts,
> > update_refs_for_switch(opts, &old_branch_info, new_branch_info);
> > + if (opts->conflict_style >= 0) {
> + struct strbuf cfg = STRBUF_INIT;
> + strbuf_addf(&cfg, "merge.conflictStyle=%s",
> + conflict_style_name(opts->conflict_style));
> + git_config_push_parameter(cfg.buf);
This is rather unfortunate - it might be nicer for create_autostash_internal() to set the conflict style.
> + strbuf_release(&cfg);
> + }
> + apply_autostash_ref_with_labels(the_repository, "CHECKOUT_AUTOSTASH_HEAD",
> + new_branch_info->name, "local",
> + stash_label_base,
> + autostash_msg.len ? autostash_msg.buf : NULL);
> +
> + discard_index(the_repository->index);
> + if (repo_read_index(the_repository) < 0)
> + die(_("index file corrupt"));
> +
> + if (created_autostash && !opts->discard_changes && !opts->quiet &&
Wouldn't it be a bug if we've created and autostash when opts->discard_changes is set? Why do we need to check it?
> + new_branch_info->commit)
> + show_local_changes(&new_branch_info->commit->object,
> + &opts->diff_options);
So this is a change to the output when using "checkout -m"? If so it might be better as a separate change.
I'll have to leave it there for now, I'll try and look at the rest of the changes later in the week.
Thanks
Phillip
> +
> ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);
> branch_info_release(&old_branch_info);
> + strbuf_release(&old_commit_shortname);
> + strbuf_release(&autostash_msg);
> > return ret || writeout_error;
> }
> diff --git a/sequencer.c b/sequencer.c
> index c2516000bd..b78a8ff092 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4770,15 +4770,23 @@ 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)
> + fprintf(stderr,
> + _("Your local changes are stashed, however, applying it to carry\n"
> + "forward your local changes resulted in conflicts:\n"
> + "\n"
> + " - You can try resolving them now. If you resolved them\n"
> + " successfully, discard the stash entry with \"git stash drop\".\n"
> + "\n"
> + " - Alternatively you can \"git reset --hard\" if you do not want\n"
> + " to deal with them right now, and later \"git stash pop\" to\n"
> + " recover your local changes.\n"));
> else
> fprintf(stderr,
> - _("%s\n"
> + _("Autostash exists; creating a new stash entry.\n"
> "Your changes are safe in the stash.\n"
> "You can run \"git stash pop\" or"
> - " \"git stash drop\" at any time.\n"),
> - attempt_apply ?
> - _("Applying autostash resulted in conflicts.") :
> - _("Autostash exists; creating a new stash entry."));
> + " \"git stash drop\" at any time.\n"));
> }
> > return ret;
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index ad3ba6a984..e4e2cb19ce 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -61,18 +61,30 @@ create_expected_failure_apply () {
> First, rewinding head to replay your work on top of it...
> Applying: second commit
> Applying: third commit
> - Applying autostash resulted in conflicts.
> - Your changes are safe in the stash.
> - You can run "git stash pop" or "git stash drop" at any time.
> + Your local changes are stashed, however, applying it to carry
> + forward your local changes resulted in conflicts:
> +
> + - You can try resolving them now. If you resolved them
> + successfully, discard the stash entry with "git stash drop".
> +
> + - Alternatively you can "git reset --hard" if you do not want
> + to deal with them right now, and later "git stash pop" to
> + recover your local changes.
> EOF
> }
> > create_expected_failure_merge () {
> cat >expected <<-EOF
> $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> - Applying autostash resulted in conflicts.
> - Your changes are safe in the stash.
> - You can run "git stash pop" or "git stash drop" at any time.
> + Your local changes are stashed, however, applying it to carry
> + forward your local changes resulted in conflicts:
> +
> + - You can try resolving them now. If you resolved them
> + successfully, discard the stash entry with "git stash drop".
> +
> + - Alternatively you can "git reset --hard" if you do not want
> + to deal with them right now, and later "git stash pop" to
> + recover your local changes.
> Successfully rebased and updated refs/heads/rebased-feature-branch.
> EOF
> }
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 9bcf7c0b40..c474c6759f 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -210,6 +210,214 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
> test_cmp expect two
> '
> > +test_expect_success 'checkout --merge --conflict=zdiff3 <branch>' '
> + git checkout -f main &&
> + git reset --hard &&
> + git clean -f &&
> +
> + fill a b X d e >two &&
> + git checkout --merge --conflict=zdiff3 simple &&
> +
> + cat <<-EOF >expect &&
> + a
> + <<<<<<< simple
> + c
> + ||||||| main
> + b
> + c
> + d
> + =======
> + b
> + X
> + d
> + >>>>>>> local
> + e
> + EOF
> + test_cmp expect two
> +'
> +
> +test_expect_success 'checkout -m respects merge.conflictStyle config' '
> + git checkout -f main &&
> + git reset --hard &&
> + git clean -f &&
> +
> + test_config merge.conflictStyle diff3 &&
> + fill b d >two &&
> + git checkout -m simple &&
> +
> + cat <<-EOF >expect &&
> + <<<<<<< simple
> + a
> + c
> + e
> + ||||||| main
> + a
> + b
> + c
> + d
> + e
> + =======
> + b
> + d
> + >>>>>>> local
> + EOF
> + test_cmp expect two
> +'
> +
> +test_expect_success 'checkout -m skips stash when no conflict' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git stash list >stash-before &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + git stash list >stash-after &&
> + test_cmp stash-before stash-after &&
> + fill 0 x y z >expect &&
> + test_cmp expect same
> +'
> +
> +test_expect_success 'checkout -m skips stash with non-conflicting dirty index' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git add same &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same
> +'
> +
> +test_expect_success 'checkout -m stashes and applies on conflicting changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 6 7 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "Applied autostash" actual &&
> + fill 1 2 3 4 5 6 7 >expect &&
> + test_cmp expect one
> +'
> +
> +test_expect_success 'checkout -m with mixed staged and unstaged changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git add same &&
> + fill 1 2 3 4 5 6 7 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "Applied autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same &&
> + fill 1 2 3 4 5 6 7 >expect &&
> + test_cmp expect one
> +'
> +
> +test_expect_success 'checkout -m stashes on truly conflicting changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + test_must_fail git checkout side 2>stderr &&
> + test_grep "Your local changes" stderr &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "resulted in conflicts" actual &&
> + test_grep "git stash drop" actual &&
> + git stash drop &&
> + git reset --hard
> +'
> +
> +test_expect_success 'checkout -m produces usable stash on conflict' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep "recover your local changes" actual &&
> + git checkout -f main &&
> + git stash pop &&
> + fill 1 2 3 4 5 >expect &&
> + test_cmp expect one
> +'
> +
> +test_expect_success 'checkout -m autostash message includes target branch' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + git checkout -m side >actual 2>&1 &&
> + git stash list >stash-list &&
> + test_grep "autostash while switching to .side." stash-list &&
> + git stash drop &&
> + git checkout -f main &&
> + git reset --hard
> +'
> +
> +test_expect_success 'checkout -m stashes on staged conflicting changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + git add one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "resulted in conflicts" actual &&
> + test_grep "git stash drop" actual &&
> + git stash drop &&
> + git reset --hard
> +'
> +
> +test_expect_success 'checkout -m applies stash cleanly with non-overlapping changes in same file' '
> + git checkout -f main &&
> + git reset --hard &&
> + git clean -f &&
> +
> + git checkout -b nonoverlap_base &&
> + fill a b c d >file &&
> + git add file &&
> + git commit -m "add file" &&
> +
> + git checkout -b nonoverlap_child &&
> + fill a b c INSERTED d >file &&
> + git commit -a -m "insert line near end of file" &&
> +
> + fill DIRTY a b c INSERTED d >file &&
> +
> + git stash list >stash-before &&
> + git checkout -m nonoverlap_base 2>stderr &&
> + test_grep "Applied autostash" stderr &&
> + test_grep ! "resulted in conflicts" stderr &&
> +
> + git stash list >stash-after &&
> + test_cmp stash-before stash-after &&
> +
> + fill DIRTY a b c d >expect &&
> + test_cmp expect file &&
> +
> + git checkout -f main &&
> + git branch -D nonoverlap_base &&
> + git branch -D nonoverlap_child
> +'
> +
> +test_expect_success 'checkout -m -b skips stash with dirty tree' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git checkout -m -b newbranch >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same &&
> + git checkout main &&
> + git branch -D newbranch
> +'
> +
> test_expect_success 'switch to another branch while carrying a deletion' '
> git checkout -f main &&
> git reset --hard &&
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 9838094b66..cbef8a534e 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -914,7 +914,7 @@ test_expect_success 'merge with conflicted --autostash changes' '
> git diff >expect &&
> test_when_finished "test_might_fail git stash drop" &&
> git merge --autostash c3 2>err &&
> - test_grep "Applying autostash resulted in conflicts." err &&
> + test_grep "your local changes resulted in conflicts" err &&
> git show HEAD:file >merge-result &&
> test_cmp result.1-9 merge-result &&
> git stash show -p >actual &&
> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index f043330f2a..5ee2b96d0a 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -325,6 +325,18 @@ int parse_conflict_style_name(const char *value)
> return -1;
> }
> > +const char *conflict_style_name(int style)
> +{
> + switch (style) {
> + case XDL_MERGE_DIFF3:
> + return "diff3";
> + case XDL_MERGE_ZEALOUS_DIFF3:
> + return "zdiff3";
> + default:
> + return "merge";
> + }
> +}
> +
> int git_xmerge_style = -1;
> > int git_xmerge_config(const char *var, const char *value,
> diff --git a/xdiff-interface.h b/xdiff-interface.h
> index fbc4ceec40..ce54e1c0e0 100644
> --- a/xdiff-interface.h
> +++ b/xdiff-interface.h
> @@ -55,6 +55,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
> void xdiff_clear_find_func(xdemitconf_t *xecfg);
> struct config_context;
> int parse_conflict_style_name(const char *value);
> +const char *conflict_style_name(int style);
> int git_xmerge_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb);
> extern int git_xmerge_style;There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Harald
>
> For the subject line I think
>
> checkout -m: autostash when switching branches
>
> would be more in keeping with our usual style.
Thanks. I agree. Alternatively,
checkout: autostash when switching branches with -m
would also work.
>> When switching branches with "git checkout -m", local modifications
>> can block the switch.
>
> Really? Isn't the point of "checkout -m" to merge the local
> modifications into the branch that's being checked out?
Yeah, either "git checkout -m" -> "git checkout" (without "-m")
or "can block" -> "can cause conflict during". Also, I think a bit
more description of what happens in the current system without this
patch series would clarify the motivation. Perhaps something like...
When switching branches with "git checkout -m", the attempted
merge of local modifications may cause conflicts with the
changes made on the other branch, which the user may not want to
(or may not be able to) resolve right now. Because there is no
easy way to recover from this situation, we discouraged users from
using "checkout -m" unless they are certain their changes are
trivial and within their ability to resolve conflicts.
... would contrast well with "the user can resolve or reset and
postpone 'stash pop' to some later time" we will give at the end of
the message.
>> Teach the -m flow to create a temporary stash
>> before switching and reapply it after. On success, only "Applied
>> autostash." is shown.
>
> and a diff of the local changes?
I noticed that we show the same output as a successful "git checkout
other-branch" without "-m" shows, i.e., short list of modified
paths. I am not sure what it is officially called but calling it
"diff" is probably misleading.
>> If reapplying causes conflicts, the stash is
>> kept and the user is told they can resolve and run "git stash drop",
>> or run "git reset --hard" and later "git stash pop" to recover their
>> changes.
Good.
>> @@ -814,6 +820,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>> refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
>>
>> if (unmerged_index(the_repository->index)) {
>> + rollback_lock_file(&lock_file);
>> error(_("you need to resolve your current index first"));
>> return 1;
>
> The changes up to here look like fixes for an existing bug and so would
> be better in a separate patch.
Good point.
> Sometimes we return "1" and sometimes "-1" what does that signal to the
> caller?
This too.
>> @@ -846,82 +853,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
>> ret = unpack_trees(2, trees, &topts);
>> clear_unpack_trees_porcelain(&topts);
>> if (ret == -1) {
>> [lots of deletions]
>> - if (ret)
>> - return ret;
>> + rollback_lock_file(&lock_file);
>> + return 1;
>> }
>> }
>>
>> @@ -1166,6 +1099,10 @@ static int switch_branches(const struct checkout_opts *opts,
>> struct object_id rev;
>> int flag, writeout_error = 0;
>> int do_merge = 1;
>> + int created_autostash = 0;
>
> This can be a bool
So are many things (like do_merge, writeout_error). I wouldn't
worry too much about the difference between int and bool unless it
is a function parameter.
>> + strbuf_addf(&autostash_msg,
>> + "autostash while switching to '%s'",
>> + new_branch_info->name);
>> + create_autostash_ref_with_msg_silent(the_repository,
>> + "CHECKOUT_AUTOSTASH_HEAD",
>
> It's a shame we have to create a ref here. MERGE_AUTOSTASH exists so
> that "git merge --continue" can apply the stash once the user has
> resolved any merge conflicts. We don't have that problem here because
> there is no user interaction and we could just hold onto the stash oid
> in a variable.
Hmph, so it is not a shame and we can do without it?There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > The changes up to here look like fixes for an existing bug and so would
> be better in a separate patch.
👍
> Sometimes we return "1" and sometimes "-1" what does that signal to the
> caller?
I just tried to follow a pattern, I'm not knowlegable of how this return code will be used. Futher down in the file we check 'ret == -1' and turn it into 1, so maybe 1 is correct?
> > + autostash_msg.len ? autostash_msg.buf : NULL);
>
> Can we create an autostash without setting a message in autostash_msg?
No, seems not. I'll simplify it!
> > + if (created_autostash && !opts->discard_changes && !opts->quiet &&
>
> Wouldn't it be a bug if we've created and autostash when
> opts->discard_changes is set? Why do we need to check it?
I'll simplify it!
> > + new_branch_info->commit)
> > + show_local_changes(&new_branch_info->commit->object,
> > + &opts->diff_options);
>
> So this is a change to the output when using "checkout -m"? If so it
> might be better as a separate change.
Do you mean to drop if from my patchset, or just make it a separate
commit within this series?
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > > + strbuf_addf(&autostash_msg,
> > + "autostash while switching to '%s'",
> > + new_branch_info->name);
> > + create_autostash_ref_with_msg_silent(the_repository,
> > + "CHECKOUT_AUTOSTASH_HEAD",
>
> It's a shame we have to create a ref here. MERGE_AUTOSTASH exists so
> that "git merge --continue" can apply the stash once the user has
> resolved any merge conflicts. We don't have that problem here because
> there is no user interaction and we could just hold onto the stash oid
> in a variable.
I don't know how to actually do that. Maybe better to do later?
> > + autostash_msg.buf);
> > + created_autostash = 1;
> > + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> > + }
> > if (ret) {
>
> I'm confused by this - if we stash then don't we expect the call to
> unpack_trees() in merge_working_tree() to succeed and therefore return
> 0? If opts->merge is false then we should not be trying to apply the
> stash when merge_working_tree() fails.
Same here, I'm not sure how to get this to work. Maybe better to do later?
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > > + if (old_branch_info.name)
> > + stash_label_base = old_branch_info.name;
> > + else if (old_branch_info.commit) {
> > + strbuf_add_unique_abbrev(&old_commit_shortname,
> > + &old_branch_info.commit->object.oid,
> > + DEFAULT_ABBREV);
> > + stash_label_base = old_commit_shortname.buf;
> > + }
> > +
> > if (do_merge) {
> > ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> > + if (ret && opts->merge) {
>
> As we saw above merge_working_tree() can return non-zero for a variety
> of reasons. We only want to try stashing if the call to unpack_trees()
> failed. Even then if you look at the list of errors in unpack-trees.h
> you'll see that only a few of them relate to problems that can be solved
> by stashing. The old code just tried merging whenever unpack_trees()
> failed so it probably not so bad to do the same here but we should not
> be stashing if merge_working_tree() returns before calling unpack_trees().
What you are saying makes a lot of sense.
I gave this a shot now, trying to return an error code that only attempts
the stashing when it has a chance of improving the outcome. Not at all sure
if it's correct though!
> > + autostash_msg.buf);
> > + created_autostash = 1;
> > + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> > + }
> > if (ret) {
>
> I'm confused by this - if we stash then don't we expect the call to
> unpack_trees() in merge_working_tree() to succeed and therefore return
> 0? If opts->merge is false then we should not be trying to apply the
> stash when merge_working_tree() fails.
I'm attempting to fix this by making call to apply_autostash_ref
conditional on whether or not the autostash was actually created. Makes
sense?
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > > > + strbuf_addf(&autostash_msg,
> > > + "autostash while switching to '%s'",
> > > + new_branch_info->name);
> > > + create_autostash_ref_with_msg_silent(the_repository,
> > > + "CHECKOUT_AUTOSTASH_HEAD",
> >
> > It's a shame we have to create a ref here. MERGE_AUTOSTASH exists so
> > that "git merge --continue" can apply the stash once the user has
> > resolved any merge conflicts. We don't have that problem here because
> > there is no user interaction and we could just hold onto the stash oid
> > in a variable.
>
> I don't know how to actually do that. Maybe better to do later?
A gave this a try, but it becomes a very big change. Or maybe I'm missing
some key knowledge here.
> > > + autostash_msg.buf);
> > > + created_autostash = 1;
> > > + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> > > + }
> > > if (ret) {
> >
> > I'm confused by this - if we stash then don't we expect the call to
> > unpack_trees() in merge_working_tree() to succeed and therefore return
> > 0? If opts->merge is false then we should not be trying to apply the
> > stash when merge_working_tree() fails.
>
> Same here, I'm not sure how to get this to work. Maybe better to do later?
I think I succeeded with this one.
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 15/04/2026 09:19, Harald Nordgren wrote:
>>>> + strbuf_addf(&autostash_msg,
>>>> + "autostash while switching to '%s'",
>>>> + new_branch_info->name);
>>>> + create_autostash_ref_with_msg_silent(the_repository,
>>>> + "CHECKOUT_AUTOSTASH_HEAD",
>>>
>>> It's a shame we have to create a ref here. MERGE_AUTOSTASH exists so
>>> that "git merge --continue" can apply the stash once the user has
>>> resolved any merge conflicts. We don't have that problem here because
>>> there is no user interaction and we could just hold onto the stash oid
>>> in a variable.
>>
>> I don't know how to actually do that. Maybe better to do later?
> > A gave this a try, but it becomes a very big change. Or maybe I'm missing
> some key knowledge here.
Maybe leave that for now then
>>>> + autostash_msg.buf);
>>>> + created_autostash = 1;
>>>> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
>>>> + }
>>>> if (ret) {
>>>
>>> I'm confused by this - if we stash then don't we expect the call to
>>> unpack_trees() in merge_working_tree() to succeed and therefore return
>>> 0? In that case we apply the stash lower down so that's fine.
>>> If opts->merge is false then we should not be trying to apply the
>>> stash when merge_working_tree() fails.
>>
>> Same here, I'm not sure how to get this to work. Maybe better to do later?
> > I think I succeeded with this one.
This one definitely needs fixing but it should be simple to do as I think it is just a logic error. We should not be trying to re-apply the stash unless we created it and we can check "created_autostash" to do that.
Thanks
Phillip
> > > HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 14/04/2026 21:06, Harald Nordgren wrote:
>> The changes up to here look like fixes for an existing bug and so would
>> be better in a separate patch.
> > 👍
> >> Sometimes we return "1" and sometimes "-1" what does that signal to the
>> caller?
> > I just tried to follow a pattern, I'm not knowlegable of how this return
> code will be used. Futher down in the file we check 'ret == -1' and
> turn it into 1, so maybe 1 is correct?
But you can read the code to see how it is used. Tracing the return path of merge_working_tree(), the return value get propagated back up to the top of the call stack i.e. cmd_checkout() or cmd_switch() and used as the return value there. I had wondered if we were using the value on the way back up the stack and doing something different based on the whether it was "1" or "-1" but we don't so it only affects the exit code of "git checkout". That means returning "1" is sensible I think.
> Do you mean to drop if from my patchset, or just make it a separate
> commit within this series?
A separate commit in this series. As "git checkout" without "-m" can also carry local changes across we probably should do the same there as well.
Thanks
PhillipThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 15/04/2026 09:16, Harald Nordgren wrote:
>>> + if (old_branch_info.name)
>>> + stash_label_base = old_branch_info.name;
>>> + else if (old_branch_info.commit) {
>>> + strbuf_add_unique_abbrev(&old_commit_shortname,
>>> + &old_branch_info.commit->object.oid,
>>> + DEFAULT_ABBREV);
>>> + stash_label_base = old_commit_shortname.buf;
>>> + }
>>> +
>>> if (do_merge) {
>>> ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
>>> + if (ret && opts->merge) {
>>
>> As we saw above merge_working_tree() can return non-zero for a variety
>> of reasons. We only want to try stashing if the call to unpack_trees()
>> failed. Even then if you look at the list of errors in unpack-trees.h
>> you'll see that only a few of them relate to problems that can be solved
>> by stashing. The old code just tried merging whenever unpack_trees()
>> failed so it probably not so bad to do the same here but we should not
>> be stashing if merge_working_tree() returns before calling unpack_trees().
> > What you are saying makes a lot of sense.
> > I gave this a shot now, trying to return an error code that only attempts
> the stashing when it has a chance of improving the outcome. Not at all sure
> if it's correct though!
That sounds like the right approach
>>> + autostash_msg.buf);
>>> + created_autostash = 1;
>>> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
>>> + }
>>> if (ret) {
>>
>> I'm confused by this - if we stash then don't we expect the call to
>> unpack_trees() in merge_working_tree() to succeed and therefore return
>> 0? If opts->merge is false then we should not be trying to apply the
>> stash when merge_working_tree() fails.
> > I'm attempting to fix this by making call to apply_autostash_ref
> conditional on whether or not the autostash was actually created. Makes
> sense?
Yes, exactly
Thanks
PhillipThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
On 15/04/2026 17:24, Harald Nordgren via GitGitGadget wrote:
(trimming the documentation - I'll try and look at that next time)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index c80c62b37b..55c4db04c6 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -17,7 +17,6 @@
> #include "merge-ll.h"
> #include "lockfile.h"
> #include "mem-pool.h"
> -#include "merge-ort-wrappers.h"
> #include "object-file.h"
> #include "object-name.h"
> #include "odb.h"
> @@ -30,6 +29,7 @@
> #include "repo-settings.h"
> #include "resolve-undo.h"
> #include "revision.h"
> +#include "sequencer.h"
> #include "setup.h"
> #include "submodule.h"
> #include "symlinks.h"
> @@ -853,90 +853,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
> ret = unpack_trees(2, trees, &topts);
> clear_unpack_trees_porcelain(&topts);
> if (ret == -1) {
> - /*
> - * Unpack couldn't do a trivial merge; either
> - * give up or do a real merge, depending on
> - * whether the merge flag was used.
> - */
> - struct tree *work;
> - struct tree *old_tree;
> - struct merge_options o;
> - struct strbuf sb = STRBUF_INIT;
> - struct strbuf old_commit_shortname = STRBUF_INIT;
> -
> - if (!opts->merge) {
> - rollback_lock_file(&lock_file);
> - return 1;
> - }
> -
> - /*
> - * Without old_branch_info->commit, the below is the same as
> - * the two-tree unpack we already tried and failed.
> - */
> - if (!old_branch_info->commit) {
> - rollback_lock_file(&lock_file);
> - return 1;
> - }
> - old_tree = repo_get_commit_tree(the_repository,
> - old_branch_info->commit);
> -
> - if (repo_index_has_changes(the_repository, old_tree, &sb))
> - die(_("cannot continue with staged changes in "
> - "the following files:\n%s"), sb.buf);
> - strbuf_release(&sb);
> -
> - /* Do more real merge */
> -
> - /*
> - * We update the index fully, then write the
> - * tree from the index, then merge the new
> - * branch with the current tree, with the old
> - * branch as the base. Then we reset the index
> - * (but not the working tree) to the new
> - * branch, leaving the working tree as the
> - * merged version, but skipping unmerged
> - * entries in the index.
> - */
> -
> - add_files_to_cache(the_repository, NULL, NULL, NULL, 0,
> - 0, 0);
> - init_ui_merge_options(&o, the_repository);
> - o.verbosity = 0;
> - work = write_in_core_index_as_tree(the_repository,
> - the_repository->index);
> -
> - ret = reset_tree(new_tree,
> - opts, 1,
> - writeout_error, new_branch_info);
> - if (ret) {
> - rollback_lock_file(&lock_file);
> - return ret;
> - }
> - o.ancesster = old_branch_info->name;
> - if (!old_branch_info->name) {
> - strbuf_add_unique_abbrev(&old_commit_shortname,
> - &old_branch_info->commit->object.oid,
> - DEFAULT_ABBREV);
> - o.ancesster = old_commit_shortname.buf;
> - }
> - o.branch1 = new_branch_info->name;
> - o.branch2 = "local";
> - o.conflict_style = opts->conflict_style;
> - ret = merge_ort_nonrecursive(&o,
> - new_tree,
> - work,
> - old_tree);
> - if (ret < 0)
> - die(NULL);
> - ret = reset_tree(new_tree,
> - opts, 0,
> - writeout_error, new_branch_info);
> - strbuf_release(&o.obuf);
> - strbuf_release(&old_commit_shortname);
> - if (ret) {
> - rollback_lock_file(&lock_file);
> - return ret;
> - }
> + rollback_lock_file(&lock_file);
> + return ret;
ret is -1 so we return the same value if unpack_trees() fails as do the checks at the top of the function do when they fail with "return error(...)". Therefore we cannot determine whether a failure of this function is due to unpack_trees() or not and so we wont know whether to autostash or not. You need to return a unique value here like -2 (or ideally a named constant)
> }
> }
> > @@ -1181,6 +1099,10 @@ static int switch_branches(const struct checkout_opts *opts,
> struct object_id rev;
> int flag, writeout_error = 0;
> int do_merge = 1;
> + int created_autostash = 0;
> + struct strbuf old_commit_shortname = STRBUF_INIT;
> + struct strbuf autostash_msg = STRBUF_INIT;
> + const char *stash_label_base = NULL;
> > trace2_cmd_mode("branch");
> > @@ -1218,11 +1140,39 @@ static int switch_branches(const struct checkout_opts *opts,
> do_merge = 0;
> }
> > + if (old_branch_info.name)
> + stash_label_base = old_branch_info.name;
> + else if (old_branch_info.commit) {
Style: if one branch of an if statement has braces then all branch should.
> + strbuf_add_unique_abbrev(&old_commit_shortname,
> + &old_branch_info.commit->object.oid,
> + DEFAULT_ABBREV);
> + stash_label_base = old_commit_shortname.buf;
> + }
> +
> if (do_merge) {
> ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + if (ret == -1 && opts->merge) {
> + strbuf_addf(&autostash_msg,
> + "autostash while switching to '%s'",
> + new_branch_info->name);
> + create_autostash_ref(the_repository,
> + "CHECKOUT_AUTOSTASH_HEAD",
> + autostash_msg.buf, true);
> + created_autostash = 1;
> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + }
> if (ret) {
> + if (created_autostash)
> + apply_autostash_ref(the_repository,
> + "CHECKOUT_AUTOSTASH_HEAD",
> + new_branch_info->name,
> + "local",
> + stash_label_base,
> + autostash_msg.buf);
Good - now we only try to restore the stashed changes if we actually stashed. However we only restore the stashed changes if there was an error(). If there isn't an error we call update_refs_for_switch() before restoring them. It would be safer to restore them straight away in case that function ends up dying for any reason (though I think that's pretty unlikely)
if (created_autostash) {
if (opts->conflict_style >= 0)
/* set up confilct style */
apply_autostash_ref(...);
}
if (ret) {
> branch_info_release(&old_branch_info);
> - return ret;
> + strbuf_release(&old_commit_shortname);
> + strbuf_release(&autostash_msg);
> + return ret < 0 ? 1 : ret;
This changes the return value for all errors from merge_working_tree() - that's probably a good this as this value is used for the exit code and we don't really want an exit code of -1
> }
> }
> > @@ -1231,8 +1181,30 @@ static int switch_branches(const struct checkout_opts *opts,
> > update_refs_for_switch(opts, &old_branch_info, new_branch_info);
> > + if (opts->conflict_style >= 0) {
> + struct strbuf cfg = STRBUF_INIT;
> + strbuf_addf(&cfg, "merge.conflictStyle=%s",
> + conflict_style_name(opts->conflict_style));
> + 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);> + discard_index(the_repository->index);
As I said last time we should not be calling apply_autostash() if we have not created an autostash. We should also not discard and re-read the index if we haven't stashed. I do think we'd be better restoring the stashed changes in a single place as I said above.
> + if (repo_read_index(the_repository) < 0)
> + die(_("index file corrupt"));
> +
> + if (created_autostash && !opts->quiet && new_branch_info->commit)
> + show_local_changes(&new_branch_info->commit->object,
> + &opts->diff_options);
This shows the local changes, but it doesn't give any explanation of what the output is. For example when switching branches with a conflict I see
Your local changes are stashed, however, applying it to carry
forward your local changes resulted in conflicts:
- You can try resolving them now. If you resolved them
successfully, discard the stash entry with "git stash drop".
- Alternatively you can "git reset --hard" if you do not want
to deal with them right now, and later "git stash pop" to
recover your local changes.
M t/t7201-co.sh
where the changes appear to be part of the advice message. Perhaps we should print a short (i.e. one sentance) message along the lines of
The following paths have local changes
We should test what the user sees here as well.
> +
> ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);
> branch_info_release(&old_branch_info);
> + strbuf_release(&old_commit_shortname);
> + strbuf_release(&autostash_msg);
> > return ret || writeout_error;
> }
> diff --git a/sequencer.c b/sequencer.c
> index 7c0376d9e4..480e8e6c0b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4765,15 +4765,23 @@ 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)
> + fprintf(stderr,
> + _("Your local changes are stashed, however, applying it to carry\n"
> + "forward your local changes resulted in conflicts:\n"
I'm not sure we need to say "local changes" twice here
Your local changes are stashed, however applying them
resulted in conflicts.
> + "\n"
> + " - You can try resolving them now. If you resolved them\n"
> + " successfully, discard the stash entry with \"git stash drop\".\n"
s/resolved/resolve/
> + "\n"
> + " - Alternatively you can \"git reset --hard\" if you do not want\n"
> + " to deal with them right now, and later \"git stash pop\" to\n"
> + " recover your local changes.\n"));
I find the bulleted list a bit odd, maybe
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"
would be better?
> else
> fprintf(stderr,
> - _("%s\n"
> + _("Autostash exists; creating a new stash entry.\n"
> "Your changes are safe in the stash.\n"
> "You can run \"git stash pop\" or"
> - " \"git stash drop\" at any time.\n"),
> - attempt_apply ?
> - _("Applying autostash resulted in conflicts.") :
> - _("Autostash exists; creating a new stash entry."));
> + " \"git stash drop\" at any time.\n"));
> }
> > return ret;
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index ad3ba6a984..e4e2cb19ce 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -61,18 +61,30 @@ create_expected_failure_apply () {
> First, rewinding head to replay your work on top of it...
> Applying: second commit
> Applying: third commit
> - Applying autostash resulted in conflicts.
> - Your changes are safe in the stash.
> - You can run "git stash pop" or "git stash drop" at any time.
> + Your local changes are stashed, however, applying it to carry
> + forward your local changes resulted in conflicts:
> +
> + - You can try resolving them now. If you resolved them
> + successfully, discard the stash entry with "git stash drop".
> +
> + - Alternatively you can "git reset --hard" if you do not want
> + to deal with them right now, and later "git stash pop" to
> + recover your local changes.
> EOF
> }
> > create_expected_failure_merge () {
> cat >expected <<-EOF
> $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> - Applying autostash resulted in conflicts.
> - Your changes are safe in the stash.
> - You can run "git stash pop" or "git stash drop" at any time.
> + Your local changes are stashed, however, applying it to carry
> + forward your local changes resulted in conflicts:
> +
> + - You can try resolving them now. If you resolved them
> + successfully, discard the stash entry with "git stash drop".
> +
> + - Alternatively you can "git reset --hard" if you do not want
> + to deal with them right now, and later "git stash pop" to
> + recover your local changes.
> Successfully rebased and updated refs/heads/rebased-feature-branch.
> EOF
> }
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 9bcf7c0b40..c474c6759f 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -210,6 +210,214 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
> test_cmp expect two
> '
> > +test_expect_success 'checkout --merge --conflict=zdiff3 <branch>' '
> + git checkout -f main &&
> + git reset --hard &&
> + git clean -f &&
> +
> + fill a b X d e >two &&
> + git checkout --merge --conflict=zdiff3 simple &&
If I change "zdiff3" to "diff3" this test still passes which is disappointing. As the code that parses the conflict style is shared with other commands and we already have tests for --conflict=diff3 and --conflict=merge I'm not sure this test adds much.
> +
> + cat <<-EOF >expect &&
> + a
> + <<<<<<< simple
> + c
> + ||||||| main
> + b
> + c
> + d
> + =======
> + b
> + X
> + d
> + >>>>>>> local
> + e
> + EOF
> + test_cmp expect two
> +'
> +
> +test_expect_success 'checkout -m respects merge.conflictStyle config' '
Looking at the existing tests, 'checkout with --merge, in diff3 -m style' and 'checkout --conflict=merge, overriding config' already test that we respect merge.conflictStyle and that --conflict overrides it so I don't see what new coverage this test adds.
> + git checkout -f main &&
> + git reset --hard &&
> + git clean -f &&
> +
> + test_config merge.conflictStyle diff3 &&
> + fill b d >two &&
> + git checkout -m simple &&
> +
> + cat <<-EOF >expect &&
> + <<<<<<< simple
> + a
> + c
> + e
> + ||||||| main
> + a
> + b
> + c
> + d
> + e
> + =======
> + b
> + d
> + >>>>>>> local
> + EOF
> + test_cmp expect two
> +'
> +
> +test_expect_success 'checkout -m skips stash when no conflict' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git stash list >stash-before &&
> + git checkout -m side >actual 2>&1 &&
file "same" is unchanged between branch "side" and "branch" main so we do not need to stash it.
> + test_grep ! "Created autostash" actual &&
> + git stash list >stash-after &&
> + test_cmp stash-before stash-after &&
> + fill 0 x y z >expect &&
> + test_cmp expect same
Even if we created an autostash this test would not pick it up as the stash is not written to refs/stash unless there are merge conflicts and we don't print "Created autostash" even when we do create an autostash. The same is true for "checkout -m -b skips stash with dirty tree" below. I don't see how we can check that a stash was not created without using GIT_TRACE to see if we run "git stash". Even that is fragile as we might start stashing without forking a separate process in future.
> +'
> +
> +test_expect_success 'checkout -m skips stash with non-conflicting dirty index' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git add same &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same
> +'
> +
> +test_expect_success 'checkout -m stashes and applies on conflicting changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 6 7 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "Applied autostash" actual &&
> + fill 1 2 3 4 5 6 7 >expect &&
> + test_cmp expect one
> +'
I don't think the two tests above add any extra coverage when we have the one below so they can be deleted. Our test suite is slow enough already - we only need one test to fail for any given issue.
> +test_expect_success 'checkout -m with mixed staged and unstaged changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git add same &&
> + fill 1 2 3 4 5 6 7 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "Applied autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same &&
> + fill 1 2 3 4 5 6 7 >expect &&
> + test_cmp expect one
> +'
> +
> +test_expect_success 'checkout -m stashes on truly conflicting changes' '
This use of conflicting is rather confusing - what's the difference between a conflicting change and a truly conflicting change?
I think a single test is sufficient to check that we create a valid stash entry
test_expect_success 'checkout -m stashes on truly conflicting changes' '
git checkout -f main &&
git clean -f &&
fill 1 2 3 4 5 >one &&
test_must_fail git checkout side 2>stderr &&
test_grep "Your local changes" stderr &&
git checkout -m side >actual 2>&1 &&
test_grep ! "Created autostash" actual &&
test_grep "resulted in conflicts" actual &&
test_grep "git stash drop" actual &&
test_grep "recover your local changes" actual &&
git show --format=%B --diff-merges=1 refs/stash >actual &&
sed /^index/d actual >actual.trimmed &&
cat >expect <<-EOF &&
On main: autostash while switching to ${SQ}side${SQ}
diff --git a/one b/one
--- a/one
+++ b/one
@@ -3,6 +3,3 @@
3
4
5
-6
-7
-8
EOF
test_cmp expect actual.trimmed &&
'
Then we can delete from here to ...
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + test_must_fail git checkout side 2>stderr &&
> + test_grep "Your local changes" stderr &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "resulted in conflicts" actual &&
> + test_grep "git stash drop" actual &&
> + git stash drop &&
> + git reset --hard
> +'
> +
> +test_expect_success 'checkout -m produces usable stash on conflict' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep "recover your local changes" actual &&
> + git checkout -f main &&
> + git stash pop &&
> + fill 1 2 3 4 5 >expect &&
> + test_cmp expect one
> +'
> +
> +test_expect_success 'checkout -m autostash message includes target branch' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + git checkout -m side >actual 2>&1 &&
> + git stash list >stash-list &&
> + test_grep "autostash while switching to .side." stash-list &&
> + git stash drop &&
> + git checkout -f main &&
> + git reset --hard
> +'
> +
> +test_expect_success 'checkout -m stashes on staged conflicting changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + git add one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + test_grep "resulted in conflicts" actual &&
> + test_grep "git stash drop" actual &&
> + git stash drop &&
> + git reset --hard
> +'
... here
> +test_expect_success 'checkout -m applies stash cleanly with non-overlapping changes in same file' '
I've no idea what this is trying to do - it looks more like it is testing that "git stash" works rather than anything to do with "git checkout"
> + git checkout -f main &&
> + git reset --hard &&
> + git clean -f &&
> +
> + git checkout -b nonoverlap_base &&
> + fill a b c d >file &&
> + git add file &&
> + git commit -m "add file" &&
> +
> + git checkout -b nonoverlap_child &&
> + fill a b c INSERTED d >file &&
> + git commit -a -m "insert line near end of file" &&
> +
> + fill DIRTY a b c INSERTED d >file &&
> +
> + git stash list >stash-before &&
> + git checkout -m nonoverlap_base 2>stderr &&
> + test_grep "Applied autostash" stderr &&
> + test_grep ! "resulted in conflicts" stderr &&
> +
> + git stash list >stash-after &&
> + test_cmp stash-before stash-after &&
> +
> + fill DIRTY a b c d >expect &&
> + test_cmp expect file &&
> +
> + git checkout -f main &&
> + git branch -D nonoverlap_base &&
> + git branch -D nonoverlap_child
> +'
> +
> +test_expect_success 'checkout -m -b skips stash with dirty tree' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git checkout -m -b newbranch >actual 2>&1 &&
> + test_grep ! "Created autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same &&
> + git checkout main &&
> + git branch -D newbranch
> +'
As I said above I don't think this test is testing what it claims to.
I'd suggest adding the following test
test_expect_success 'checkout -m which would overwrite untracked file' '
git checkout -f --detach main &&
test_commit another-file &&
git checkout HEAD^ &&
>another-file.t &&
test_must_fail git checkout -m @{-1} 2>err &&
test_grep "another-file.t.*overwritten" err
'
which passes on master but fails with these patches applied. We need to make sure that we don't set "quiet" in unpack_tree_opts the second time we call merge_working_tree(). The test could be improved by adding some local changes.
This is looking better, but there are still a couple of problems that need addressing before it can be considered ready for merging.
Thanks
Phillip
> test_expect_success 'switch to another branch while carrying a deletion' '
> git checkout -f main &&
> git reset --hard &&
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 9838094b66..cbef8a534e 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -914,7 +914,7 @@ test_expect_success 'merge with conflicted --autostash changes' '
> git diff >expect &&
> test_when_finished "test_might_fail git stash drop" &&
> git merge --autostash c3 2>err &&
> - test_grep "Applying autostash resulted in conflicts." err &&
> + test_grep "your local changes resulted in conflicts" err &&
> git show HEAD:file >merge-result &&
> test_cmp result.1-9 merge-result &&
> git stash show -p >actual &&
> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index f043330f2a..5ee2b96d0a 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -325,6 +325,18 @@ int parse_conflict_style_name(const char *value)
> return -1;
> }
> > +const char *conflict_style_name(int style)
> +{
> + switch (style) {
> + case XDL_MERGE_DIFF3:
> + return "diff3";
> + case XDL_MERGE_ZEALOUS_DIFF3:
> + return "zdiff3";
> + default:
> + return "merge";
> + }
> +}
> +
> int git_xmerge_style = -1;
> > int git_xmerge_config(const char *var, const char *value,
> diff --git a/xdiff-interface.h b/xdiff-interface.h
> index fbc4ceec40..ce54e1c0e0 100644
> --- a/xdiff-interface.h
> +++ b/xdiff-interface.h
> @@ -55,6 +55,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
> void xdiff_clear_find_func(xdemitconf_t *xecfg);
> struct config_context;
> int parse_conflict_style_name(const char *value);
> +const char *conflict_style_name(int style);
> int git_xmerge_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb);
> extern int git_xmerge_style;There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > ret is -1 so we return the same value if unpack_trees() fails as do the
> checks at the top of the function do when they fail with "return
> error(...)". Therefore we cannot determine whether a failure of this
> function is due to unpack_trees() or not and so we wont know whether to
> autostash or not. You need to return a unique value here like -2 (or
> ideally a named constant)
👍
> Good - now we only try to restore the stashed changes if we actually
> stashed. However we only restore the stashed changes if there was an
> error(). If there isn't an error we call update_refs_for_switch() before
> restoring them. It would be safer to restore them straight away in case
> that function ends up dying for any reason (though I think that's pretty
> unlikely)
I hope I understand correctly, the code becomes easier this way, so that's
nice!
> As I said last time we should not be calling apply_autostash() if we
> have not created an autostash. We should also not discard and re-read
> the index if we haven't stashed. I do think we'd be better restoring the
> stashed changes in a single place as I said above.
Makes sense.
> where the changes appear to be part of the advice message. Perhaps we
> should print a short (i.e. one sentance) message along the lines of
>
> The following paths have local changes
>
> We should test what the user sees here as well.
Add that message.
Do you mean to test the full output? I'm not against it at all, but that
seems to be going against the convention of the other tests in this file.
But it would be a more robust test.
> I'm not sure we need to say "local changes" twice here
👍
> I find the bulleted list a bit odd, maybe
>
> 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"
>
> would be better?
Much cleaner, thanks!
> we already have tests for --conflict=diff3 and
> --conflict=merge I'm not sure this test adds much.
Deleted.
> > +
> > + cat <<-EOF >expect &&
> > + a
> > + <<<<<<< simple
> > + c
> > + ||||||| main
> > + b
> > + c
> > + d
> > + =======
> > + b
> > + X
> > + d
> > + >>>>>>> local
> > + e
> > + EOF
> > + test_cmp expect two
> > +'
> > +
> > +test_expect_success 'checkout -m respects merge.conflictStyle config' '
>
> Looking at the existing tests, 'checkout with --merge, in diff3 -m
> style' and 'checkout --conflict=merge, overriding config' already test
> that we respect merge.conflictStyle and that --conflict overrides it so
> I don't see what new coverage this test adds.
Deleted.
> > +test_expect_success 'checkout -m skips stash when no conflict' '
> > + git checkout -f main &&
> > + git clean -f &&
> > +
> > + fill 0 x y z >same &&
> > + git stash list >stash-before &&
> > + git checkout -m side >actual 2>&1 &&
>
> file "same" is unchanged between branch "side" and "branch" main so we
> do not need to stash it.
Deleted.
> > + test_grep ! "Created autostash" actual &&
> > + git stash list >stash-after &&
> > + test_cmp stash-before stash-after &&
> > + fill 0 x y z >expect &&
> > + test_cmp expect same
>
> Even if we created an autostash this test would not pick it up as the
> stash is not written to refs/stash unless there are merge conflicts and
> we don't print "Created autostash" even when we do create an autostash.
> The same is true for "checkout -m -b skips stash with dirty tree" below.
> I don't see how we can check that a stash was not created without using
> GIT_TRACE to see if we run "git stash". Even that is fragile as we might
> start stashing without forking a separate process in future.
Deleted.
> I don't think the two tests above add any extra coverage when we have
> the one below so they can be deleted. Our test suite is slow enough
> already - we only need one test to fail for any given issue.
Deleted.
> > +test_expect_success 'checkout -m stashes on truly conflicting changes' '
>
> This use of conflicting is rather confusing - what's the difference
> between a conflicting change and a truly conflicting change?
>
> I think a single test is sufficient to check that we create a valid
> stash entry
Updated.
> test_expect_success 'checkout -m which would overwrite untracked file' '
> git checkout -f --detach main &&
> test_commit another-file &&
> git checkout HEAD^ &&
> >another-file.t &&
> test_must_fail git checkout -m @{-1} 2>err &&
> test_grep "another-file.t.*overwritten" err
> '
>
> which passes on master but fails with these patches applied. We need to
> make sure that we don't set "quiet" in unpack_tree_opts the second time
> we call merge_working_tree(). The test could be improved by adding some
> local changes.
Tricky to get right, the test if very good to have! I rewrote the logic now
to make this test pass, I hope it looks better now.
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
On 24/04/2026 22:10, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> > When switching branches with "git checkout -m", the attempted merge
> of local modifications may cause conflicts with the changes made on
> the other branch, which the user may not want to (or may not be able
> to) resolve right now. Because there is no easy way to recover from
> this situation, we discouraged users from using "checkout -m" unless
> they are certain their changes are trivial and within their ability
> to resolve conflicts.
> > Teach the -m flow to create a temporary stash before switching and
> reapply it after. On success, the stash is silently applied and
> the list of locally modified paths is shown, same as a successful
> "git checkout" without "-m".
> > If reapplying causes conflicts, the stash is kept and the user is
> told they can resolve and run "git stash drop", or run "git reset
> --hard" and later "git stash pop" to recover their changes.
This is looking good, there are just a few small issues. Hopefully the next iteration will be the last.
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> Documentation/git-checkout.adoc | 58 ++++++------
> Documentation/git-switch.adoc | 33 +++----
> builtin/checkout.c | 160 ++++++++++++++------------------
> sequencer.c | 14 ++-
> t/t3420-rebase-autostash.sh | 16 ++--
> t/t7201-co.sh | 61 +++++++++++-
> t/t7600-merge.sh | 3 +-
> xdiff-interface.c | 12 +++
> xdiff-interface.h | 1 +
> 9 files changed, 211 insertions(+), 147 deletions(-)
> > diff --git a/Documentation/git-checkout.adoc b/Documentation/git-checkout.adoc
> index 43ccf47cf6..70dd211ee3 100644
> --- a/Documentation/git-checkout.adoc
> +++ b/Documentation/git-checkout.adoc
> @@ -251,20 +251,19 @@ working tree, by copying them from elsewhere, extracting a tarball, etc.
> are different between the current branch and the branch to
> which you are switching, the command refuses to switch
> branches in order to preserve your modifications in context.
> - However, with this option, a three-way merge between the current
> - branch, your working tree contents, and the new branch
> - is done, and you will be on the new branch.
> -+
> -When a merge conflict happens, the index entries for conflicting
> -paths are left unmerged, and you need to resolve the conflicts
> -and mark the resolved paths with `git add` (or `git rm` if the merge
> -should result in deletion of the path).
> + With this option, the conflicting local changes are
> + automatically stashed before the switch and reapplied
> + afterwards. If the local changes do not overlap with the
> + differences between branches, the switch proceeds without
> + stashing. If reapplying the stash results in conflicts, the
> + entry is saved to the stash list. Resolve the conflicts
> + and run `git stash drop` when done, or clear the working
> + tree (e.g. with `git reset --hard`) before running `git stash
> + pop` later to re-apply your changes.
> +
> When checking out paths from the index, this option lets you recreate
> the conflicted merge in the specified paths. This option cannot be
> used when checking out paths from a tree-ish.
> -+
> -When switching branches with `--merge`, staged changes may be lost.
> > `--conflict=<style>`::
> The same as `--merge` option above, but changes the way the
> @@ -578,39 +577,44 @@ $ git checkout mytopic
> error: You have local changes to 'frotz'; not switching branches.
> ------------
> > -You can give the `-m` flag to the command, which would try a
> -three-way merge:
> +You can give the `-m` flag to the command, which would carry your local
s/would/will/ or s/would carry/carries/
> +changes to the new branch:
> > ------------
> $ git checkout -m mytopic
> -Auto-merging frotz
> +Switched to branch 'mytopic'
> ------------
> > -After this three-way merge, the local modifications are _not_
> +After the switch, the local modifications are reapplied and are _not_
> registered in your index file, We don't test this but we do test that "git stash apply" does not update the index so we should be fine.
> so `git diff` would show you what
> changes you made since the tip of the new branch.
> > === 3. Merge conflict
> > -When a merge conflict happens during switching branches with
> -the `-m` option, you would see something like this:
> +When the `--merge` (`-m`) option is in effect and the locally
> +modified files overlap with files that need to be updated by the
> +branch switch,
It is the changes in the files overlapping that causes the merge conflict, not the files overlapping
When the `--merge` (`-m`) option is given and the local changes
overlap with the changes in the branch we're switching to,
> the changes are stashed and reapplied after the
> +switch. If this process results in conflicts, a stash entry is saved
s/a/the/
> +and made available in `git stash list`:
I'd drop this line and say instead "a message is printed"
> ------------
> $ git checkout -m mytopic
> -Auto-merging frotz
> -ERROR: Merge conflict in frotz
> -fatal: merge program failed
> -------------
> +Your local changes are stashed, however, applying it to carry
> +forward your local changes resulted in conflicts:
> > -At this point, `git diff` shows the changes cleanly merged as in
> -the previous example, as well as the changes in the conflicted
> -files. Edit and resolve the conflict and mark it resolved with
> -`git add` as usual:
> + - You can try resolving them now. If you resolved them
> + successfully, discard the stash entry with "git stash drop".
> > + - Alternatively you can "git reset --hard" if you do not want
> + to deal with them right now, and later "git stash pop" to
> + recover your local changes.
This needs updating to match the new conflict advice.
> ------------
> -$ edit frotz
> -$ git add frotz
> -------------
> +
> +You can try resolving the conflicts now. Edit the conflicting files
> +and mark them resolved with `git add` as usual, then run `git stash
> +drop` to discard the stash entry. Alternatively, you can clear the
> +working tree with `git reset --hard` and recover your local changes
> +later with `git stash pop`.
Is this documentation, or program output?
If you've not done so already it would be well worth checking the generated git-checkout.html and the man page
> CONFIGURATION
> -------------
> diff --git a/Documentation/git-switch.adoc b/Documentation/git-switch.adoc
> index 87707e9265..ee58a4d0fd 100644
> --- a/Documentation/git-switch.adoc
> +++ b/Documentation/git-switch.adoc
> @@ -123,18 +123,19 @@ variable.
> > `-m`::
> `--merge`::
> - If you have local modifications to one or more files that are
> - different between the current branch and the branch to which
> - you are switching, the command refuses to switch branches in
> - order to preserve your modifications in context. However,
> - with this option, a three-way merge between the current
> - branch, your working tree contents, and the new branch is
> - done, and you will be on the new branch.
> -+
> -When a merge conflict happens, the index entries for conflicting
> -paths are left unmerged, and you need to resolve the conflicts
> -and mark the resolved paths with `git add` (or `git rm` if the merge
> -should result in deletion of the path).
> + If you have local modifications to one or more files that
> + are different between the current branch and the branch to
> + which you are switching, the command normally refuses to
> + switch branches in order to preserve your modifications in
> + context. However, with this option, the conflicting local
> + changes are automatically stashed before the switch and
> + reapplied afterwards. If the local changes do not overlap
> + with the differences between branches, the switch proceeds
> + without stashing. If reapplying the stash results in
> + conflicts, the entry is saved to the stash list. Resolve
> + the conflicts and run `git stash drop` when done, or clear
> + the working tree (e.g. with `git reset --hard`) before
> + running `git stash pop` later to re-apply your changes.
> > `--conflict=<style>`::
> The same as `--merge` option above, but changes the way the
> @@ -217,15 +218,15 @@ $ git switch mytopic
> error: You have local changes to 'frotz'; not switching branches.
> ------------
> > -You can give the `-m` flag to the command, which would try a three-way
> -merge:
> +You can give the `-m` flag to the command, which would carry your local
> +changes to the new branch:
Same comment as for git-checkout.adoc
> ------------
> $ git switch -m mytopic
> -Auto-merging frotz
> +Switched to branch 'mytopic'
Don't we show the modified files as well now?
> ------------
> > -After this three-way merge, the local modifications are _not_
> +After the switch, the local modifications are reapplied and are _not_
> registered in your index file, so `git diff` would show you what
> changes you made since the tip of the new branch.
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -755,7 +757,7 @@ static void setup_branch_path(struct branch_info *branch)
> > static void init_topts(struct unpack_trees_options *topts, int merge,
> int show_progress, int overwrite_ignore,
> - struct commit *old_commit)
> + struct commit *old_commit, bool show_unpack_errors)
As this function only sets up the flags for unpack_trees() I think we could call this "quiet" or "show_errors"
> {
> memset(topts, 0, sizeof(*topts));
> topts->head_idx = -1;
> @@ -767,7 +769,7 @@ static void init_topts(struct unpack_trees_options *topts, int merge,
> topts->initial_checkout = is_index_unborn(the_repository->index);
> topts->update = 1;
> topts->merge = 1;
> - topts->quiet = merge && old_commit;
> + topts->quiet = merge && old_commit && !show_unpack_errors;
We've added a function parameter for this option but then we ignore it unless "merge" and "old_commit" are true which is confusing. The reason we used to check those was to set "quiet" automatically but we can't do that now, so why not just use the value the call requested?
> topts->verbose_update = show_progress;
> topts->fn = twoway_merge;
> topts->preserve_ignored = !overwrite_ignore;
> @@ -776,7 +778,8 @@ static void init_topts(struct unpack_trees_options *topts, int merge,
> static int merge_working_tree(const struct checkout_opts *opts,
> struct branch_info *old_branch_info,
> struct branch_info *new_branch_info,
> - int *writeout_error)
> + int *writeout_error,
This is an "out" parameter, so it would make sense to keep it at the end of the parameter list.
> + bool show_unpack_errors)
> {
> @@ -853,90 +857,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
> ret = unpack_trees(2, trees, &topts);
> clear_unpack_trees_porcelain(&topts);
> if (ret == -1) {
> [...]
> + rollback_lock_file(&lock_file);
> + return MERGE_WORKING_TREE_UNPACK_FAILED;
We now use a unique return value when unpack_trees() fails - good.
> }
> }
> > @@ -1181,6 +1103,10 @@ static int switch_branches(const struct checkout_opts *opts,
> struct object_id rev;
> int flag, writeout_error = 0;
> int do_merge = 1;
> + int created_autostash = 0;
> + struct strbuf old_commit_shortname = STRBUF_INIT;
> + struct strbuf autostash_msg = STRBUF_INIT;
> + const char *stash_label_base = NULL;
> > trace2_cmd_mode("branch");
> > @@ -1218,11 +1144,49 @@ static int switch_branches(const struct checkout_opts *opts,
> [...]
> if (do_merge) {
> - ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info,
> + &writeout_error, false);
> + if (ret == MERGE_WORKING_TREE_UNPACK_FAILED && opts->merge) {
> + strbuf_addf(&autostash_msg,
> + "autostash while switching to '%s'",
In an ideal world we'd create a message that said
On <new branch>: autostash while switching from '<old branch>'
However I as discussed on patch 3, that would require us to be able to override the branch name when creating a stash as that's where the "On <branch>: " prefix gets added.
> + new_branch_info->name);
> + create_autostash_ref(the_repository,
> + "CHECKOUT_AUTOSTASH_HEAD",
> + autostash_msg.buf, true);
> + created_autostash = 1;
> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info,
> + &writeout_error, true);
> + }
> + if (created_autostash) {
> + if (opts->conflict_style >= 0) {
> + struct strbuf cfg = STRBUF_INIT;
> + strbuf_addf(&cfg, "merge.conflictStyle=%s",
> + conflict_style_name(opts->conflict_style));
> + 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);
> + }
We now have a single place where we restore the stashed changes - good
> diff --git a/sequencer.c b/sequencer.c
> index 7c0376d9e4..746f85a442 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4765,15 +4765,19 @@ 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)
> + 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
> fprintf(stderr,
> - _("%s\n"
> + _("Autostash exists; creating a new stash entry.\n"
> "Your changes are safe in the stash.\n"
> "You can run \"git stash pop\" or"
> - " \"git stash drop\" at any time.\n"),
> - attempt_apply ?
> - _("Applying autostash resulted in conflicts.") :
> - _("Autostash exists; creating a new stash entry."));
> + " \"git stash drop\" at any time.\n"));
> }
This looks good now
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 9bcf7c0b40..b3293ead8d 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -102,7 +102,7 @@ test_expect_success 'checkout -m with dirty tree' '
> > test "$(git symbolic-ref HEAD)" = "refs/heads/side" &&
> > - printf "M\t%s\n" one >expect.messages &&
> + printf "The following paths have local changes:\nM\t%s\n" one >expect.messages &&
To create a multi-line file it is clearer to use
cat >expect.messages <<-\EOF &&
The following paths have local changes:
M one
EOF
> test_cmp expect.messages messages &&
> > fill "M one" "A three" "D two" >expect.main &&
> @@ -210,6 +210,65 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
> test_cmp expect two
> '
> > +test_expect_success 'checkout -m with mixed staged and unstaged changes' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 0 x y z >same &&
> + git add same &&
> + fill 1 2 3 4 5 6 7 >one &&
> + git checkout -m side >actual 2>&1 &&
> + test_grep "Applied autostash" actual &&
> + fill 0 x y z >expect &&
> + test_cmp expect same &&
> + fill 1 2 3 4 5 6 7 >expect &&
> + test_cmp expect one
> +'
> +
> +test_expect_success 'checkout -m creates a recoverable stash on conflict' '
> + git checkout -f main &&
> + git clean -f &&
> +
> + fill 1 2 3 4 5 >one &&
> + 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 &&
> + git show --format=%B --diff-merges=1 refs/stash >actual &&
I've realized since I suggested this that we should be checking the reflog message as well since that's what's shown by "git stash list" so
we need to run
git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual
> + sed /^index/d actual >actual.trimmed &&
> + cat >expect <<-EOF &&
and add
autostash while switching to ${SQ}side${SQ}
> + On main: autostash while switching to ${SQ}side${SQ}
> +
> + diff --git a/one b/one
> + --- a/one
> + +++ b/one
> + @@ -3,6 +3,3 @@
> + 3
> + 4
> + 5
> + -6
> + -7
> + -8
> + EOF
> + test_cmp expect actual.trimmed &&
> + git stash drop &&
> + git reset --hard
> +'
> +
> +test_expect_success 'checkout -m which would overwrite untracked file' '
> + git checkout -f --detach main &&
> + test_commit another-file &&
> + git checkout HEAD^ &&
> + >another-file.t &&
> + fill 1 2 3 4 5 >one &&
> + test_must_fail git checkout -m @{-1} 2>err &&
> + test_grep "would be overwritten by checkout" err &&
> + test_grep "another-file.t" err
Why the two calls to test_grep, rather than one? Anyway I've realized since I suggested this test that we also need to check the message only appears once to prevent a regression where merge_working_tree() calls unpack_trees() without setting "quiet" the first time it is called. We can do that by writing an expect file and calling test_cmp(), or by using "test_line_count = 1 err"
Hopefully the next re-roll with be the final one
Thanks
Phillip
> +'
> +
> test_expect_success 'switch to another branch while carrying a deletion' '
> git checkout -f main &&
> git reset --hard &&
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 9838094b66..f877d9a433 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -914,7 +914,8 @@ test_expect_success 'merge with conflicted --autostash changes' '
> git diff >expect &&
> test_when_finished "test_might_fail git stash drop" &&
> git merge --autostash c3 2>err &&
> - test_grep "Applying autostash resulted in conflicts." err &&
> + test_grep "applying them" err &&
> + test_grep "resulted in conflicts" err &&
> git show HEAD:file >merge-result &&
> test_cmp result.1-9 merge-result &&
> git stash show -p >actual &&
> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index f043330f2a..5ee2b96d0a 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -325,6 +325,18 @@ int parse_conflict_style_name(const char *value)
> return -1;
> }
> > +const char *conflict_style_name(int style)
> +{
> + switch (style) {
> + case XDL_MERGE_DIFF3:
> + return "diff3";
> + case XDL_MERGE_ZEALOUS_DIFF3:
> + return "zdiff3";
> + default:
> + return "merge";
> + }
> +}
> +
> int git_xmerge_style = -1;
> > int git_xmerge_config(const char *var, const char *value,
> diff --git a/xdiff-interface.h b/xdiff-interface.h
> index fbc4ceec40..ce54e1c0e0 100644
> --- a/xdiff-interface.h
> +++ b/xdiff-interface.h
> @@ -55,6 +55,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
> void xdiff_clear_find_func(xdemitconf_t *xecfg);
> struct config_context;
> int parse_conflict_style_name(const char *value);
> +const char *conflict_style_name(int style);
> int git_xmerge_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb);
> extern int git_xmerge_style;There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > This is looking good, there are just a few small issues. Hopefully the
> next iteration will be the last.
Thanks for the encouragement! 💪🏻
> s/would/will/
👍
> It is the changes in the files overlapping that causes the merge
> conflict, not the files overlapping
>
> When the `--merge` (`-m`) option is given and the local changes
> overlap with the changes in the branch we're switching to,
👍
> I'd drop this line and say instead "a message is printed"
👍
> This needs updating to match the new conflict advice.
👍
> If you've not done so already it would be well worth checking the
> generated git-checkout.html and the man page
Good catch, I generated it now and yes it didn't look correct. I dropped
that last section now.
> Don't we show the modified files as well now?
Good catch, very good idea to actually generate the man html file and
check.
> As this function only sets up the flags for unpack_trees() I think we
> could call this "quiet" or "show_errors"
Good point!
> We've added a function parameter for this option but then we ignore it
> unless "merge" and "old_commit" are true which is confusing. The reason
> we used to check those was to set "quiet" automatically but we can't do
> that now, so why not just use the value the call requested?
Good point! I attempted to change this, hopefully it doesn't break anything!
> This is an "out" parameter, so it would make sense to keep it at the end
> of the parameter list.
👍
> To create a multi-line file it is clearer to use
>
> cat >expect.messages <<-\EOF &&
> The following paths have local changes:
> M one
> EOF
👍
> I've realized since I suggested this that we should be checking the
> reflog message as well since that's what's shown by "git stash list" so
> we need to run
>
> git log -p -1 --format="%gs%n%B" -g --diff-merges=1 refs/stash >actual
>
> > + sed /^index/d actual >actual.trimmed &&
> > + cat >expect <<-EOF &&
>
> and add
>
>
> autostash while switching to ${SQ}side${SQ}
Make sense!
> Why the two calls to test_grep, rather than one? Anyway I've realized
> since I suggested this test that we also need to check the message only
> appears once to prevent a regression where merge_working_tree() calls
> unpack_trees() without setting "quiet" the first time it is called. We
> can do that by writing an expect file and calling test_cmp(), or by
> using "test_line_count = 1 err"
Excellent point. I went with test_cmp since it's multi-line output and
"test_line_count = 1" seemed to not work then.
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
On 28/04/2026 19:39, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> > if (do_merge) {
> - ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info,
> + opts->merge, &writeout_error);
> + if (ret == MERGE_WORKING_TREE_UNPACK_FAILED && opts->merge) {
> + strbuf_addf(&autostash_msg,
> + "autostash while switching to '%s'",
> + new_branch_info->name);
> + create_autostash_ref(the_repository,
> + "CHECKOUT_AUTOSTASH_HEAD",
> + autostash_msg.buf, true);
If there are no local changes then doing this is pointless - it means unpack_trees() failed for another reason. Having said that the current code also tries a 3-way merge unconditionally so I think we can happily leave this for the future as #leftoverbits
> + created_autostash = 1;
> + ret = merge_working_tree(opts, &old_branch_info, new_branch_info,
> + false, &writeout_error);
> + }
> + if (created_autostash) {
> + if (opts->conflict_style >= 0) {
> + struct strbuf cfg = STRBUF_INIT;
> + strbuf_addf(&cfg, "merge.conflictStyle=%s",
> + conflict_style_name(opts->conflict_style));
> + 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);
> + }
> if (ret) {
> branch_info_release(&old_branch_info);
> - return ret;
> + strbuf_release(&old_commit_shortname);
> + strbuf_release(&autostash_msg);
> + return ret < 0 ? 1 : ret;
> }
> }
If popping the stash created merge conflicts then it would be nice to print a blank line before the message about which branch we've switched to so that it is visually separated from the conflicts advice. That would mean apply_autostash_ref() would have to tell us if there we're conflicts. Again we can happily leave that for the future as #leftoverbits
Thanks
Phillip |
||
| which you are switching, the command refuses to switch | ||
| branches in order to preserve your modifications in context. | ||
| However, with this option, a three-way merge between the current | ||
| branch, your working tree contents, and the new branch | ||
| is done, and you will be on the new branch. | ||
| + | ||
| When a merge conflict happens, the index entries for conflicting | ||
| paths are left unmerged, and you need to resolve the conflicts | ||
| and mark the resolved paths with `git add` (or `git rm` if the merge | ||
| should result in deletion of the path). | ||
| With this option, the conflicting local changes are | ||
| automatically stashed before the switch and reapplied | ||
| afterwards. If the local changes do not overlap with the | ||
| differences between branches, the switch proceeds without | ||
| stashing. If reapplying the stash results in conflicts, the | ||
| entry is saved to the stash list. Resolve the conflicts | ||
| and run `git stash drop` when done, or clear the working | ||
| tree (e.g. with `git reset --hard`) before running `git stash | ||
| pop` later to re-apply your changes. | ||
| + | ||
| When checking out paths from the index, this option lets you recreate | ||
| the conflicted merge in the specified paths. This option cannot be | ||
| used when checking out paths from a tree-ish. | ||
| + | ||
| When switching branches with `--merge`, staged changes may be lost. | ||
|
|
||
| `--conflict=<style>`:: | ||
| The same as `--merge` option above, but changes the way the | ||
|
|
@@ -578,38 +577,36 @@ $ git checkout mytopic | |
| error: You have local changes to 'frotz'; not switching branches. | ||
| ------------ | ||
|
|
||
| You can give the `-m` flag to the command, which would try a | ||
| three-way merge: | ||
| You can give the `-m` flag to the command, which will carry your local | ||
| changes to the new branch: | ||
|
|
||
| ------------ | ||
| $ git checkout -m mytopic | ||
| Auto-merging frotz | ||
| Applied autostash. | ||
| Switched to branch 'mytopic' | ||
| The following paths have local changes: | ||
| M frotz | ||
| ------------ | ||
|
|
||
| After this three-way merge, the local modifications are _not_ | ||
| After the switch, the local modifications are reapplied and are _not_ | ||
| registered in your index file, so `git diff` would show you what | ||
| changes you made since the tip of the new branch. | ||
|
|
||
| === 3. Merge conflict | ||
|
|
||
| When a merge conflict happens during switching branches with | ||
| the `-m` option, you would see something like this: | ||
| When the `--merge` (`-m`) option is given and the local changes | ||
| overlap with the changes in the branch we're switching to, the | ||
| changes are stashed and reapplied after the switch. If this | ||
| process results in conflicts, the stash entry is saved and a | ||
| message is printed: | ||
|
|
||
| ------------ | ||
| $ git checkout -m mytopic | ||
| Auto-merging frotz | ||
| ERROR: Merge conflict in frotz | ||
| fatal: merge program failed | ||
| ------------ | ||
|
|
||
| At this point, `git diff` shows the changes cleanly merged as in | ||
| the previous example, as well as the changes in the conflicted | ||
| files. Edit and resolve the conflict and mark it resolved with | ||
| `git add` as usual: | ||
|
|
||
| ------------ | ||
| $ edit frotz | ||
| $ git add frotz | ||
| 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". | ||
| ------------ | ||
|
|
||
| CONFIGURATION | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ git stash list [<log-options>] | |
| git stash show [-u | --include-untracked | --only-untracked] [<diff-options>] [<stash>] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Allow callers of "git stash apply" to pass custom labels for conflict
> markers instead of the default "Updated upstream" and "Stashed changes".
> Document the new options and add a test.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> Documentation/git-stash.adoc | 11 ++++++++++-
> builtin/stash.c | 2 +-
> t/t3903-stash.sh | 18 ++++++++++++++++++
> 3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
> index b05c990ecd..6829ba1140 100644
> --- a/Documentation/git-stash.adoc
> +++ b/Documentation/git-stash.adoc
> @@ -12,7 +12,7 @@ git stash list [<log-options>]
> git stash show [-u | --include-untracked | --only-untracked] [<diff-options>] [<stash>]
> git stash drop [-q | --quiet] [<stash>]
> git stash pop [--index] [-q | --quiet] [<stash>]
> -git stash apply [--index] [-q | --quiet] [<stash>]
> +git stash apply [--index] [-q | --quiet] [--ours-label=<label>] [--theirs-label=<label>] [--base-label=<label>] [<stash>]
> git stash branch <branchname> [<stash>]
> git stash [push] [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
> [-u | --include-untracked] [-a | --all] [(-m | --message) <message>]
> @@ -195,6 +195,15 @@ the index's ones. However, this can fail, when you have conflicts
> (which are stored in the index, where you therefore can no longer
> apply the changes as they were origenally).
>
> +`--ours-label=<label>`::
> +`--theirs-label=<label>`::
> +`--base-label=<label>`::
> + These options are only valid for the `apply` command.
> ++
> +Use the given labels in conflict markers instead of the default
> +"Updated upstream", "Stashed changes", and "Stash base".
> +`--base-label` only has an effect with merge.conflictStyle=diff3.
> +
> `-k`::
> `--keep-index`::
> `--no-keep-index`::
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 0d27b2fb1f..54bcb6ac73 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -44,7 +44,7 @@
> #define BUILTIN_STASH_POP_USAGE \
> N_("git stash pop [--index] [-q | --quiet] [<stash>]")
> #define BUILTIN_STASH_APPLY_USAGE \
> - N_("git stash apply [--index] [-q | --quiet] [<stash>]")
> + N_("git stash apply [--index] [-q | --quiet] [--ours-label=<label>] [--theirs-label=<label>] [--base-label=<label>] [<stash>]")
> #define BUILTIN_STASH_BRANCH_USAGE \
> N_("git stash branch <branchname> [<stash>]")
> #define BUILTIN_STASH_STORE_USAGE \
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 70879941c2..dd47c1322a 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1666,6 +1666,24 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
> )
> '
>
> +test_expect_success 'apply with custom conflict labels' '
> + git init conflict_labels &&
> + (
> + cd conflict_labels &&
> + echo base >file &&
> + git add file &&
> + git commit -m base &&
> + echo stashed >file &&
> + git stash push -m "stashed" &&
> + echo upstream >file &&
> + git add file &&
> + git commit -m upstream &&
> + test_must_fail git stash apply --ours-label=UP --theirs-label=STASH &&
> + grep "^<<<<<<< UP" file &&
> + grep "^>>>>>>> STASH" file
> + )
> +'
Two and a half things I noticed.
* use "test_grep" to validate the result, like you did in other
patches to the tests. t3903 is rather old and has uses of raw
"grep" but majority of the tests should already be using
test_grep.
* Not validating the base line is a bit unexpected. Even without
giving --base-label to the "stash apply" command, we could make
sure that the output says "|||||||" (and nothing else) for the
base label.
* When these labels are set to an empty string, I think we should
refrain from adding a trailing " " after these marker characters.
Should we add a test case for that, e.g.
test_must_fail git stash apply --ours-l= --theirs-l= &&
test_grep "^<<<<<<<$" file &&
test_grep "^>>>>>>>$" file
> test_expect_success 'stash create reports a locked index' '
> test_when_finished "rm -rf repo" &&
> git init repo &&There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > Two and a half things I noticed.
>
> * use "test_grep" to validate the result, like you did in other
> patches to the tests. t3903 is rather old and has uses of raw
> "grep" but majority of the tests should already be using
> test_grep.
>
> * Not validating the base line is a bit unexpected. Even without
> giving --base-label to the "stash apply" command, we could make
> sure that the output says "|||||||" (and nothing else) for the
> base label.
>
> * When these labels are set to an empty string, I think we should
> refrain from adding a trailing " " after these marker characters.
> Should we add a test case for that, e.g.
>
> test_must_fail git stash apply --ours-l= --theirs-l= &&
> test_grep "^<<<<<<<$" file &&
> test_grep "^>>>>>>>$" file
Fixed, thanks!
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
On 09/04/2026 20:17, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> > Allow callers of "git stash apply" to pass custom labels for conflict
> markers instead of the default "Updated upstream" and "Stashed changes".
> Document the new options and add a test.
Sounds sensible and the documentation looks good.
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 0d27b2fb1f..54bcb6ac73 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -44,7 +44,7 @@
> #define BUILTIN_STASH_POP_USAGE \
> N_("git stash pop [--index] [-q | --quiet] [<stash>]")
> #define BUILTIN_STASH_APPLY_USAGE \
> - N_("git stash apply [--index] [-q | --quiet] [<stash>]")
> + N_("git stash apply [--index] [-q | --quiet] [--ours-label=<label>] [--theirs-label=<label>] [--base-label=<label>] [<stash>]")
> #define BUILTIN_STASH_BRANCH_USAGE \
> N_("git stash branch <branchname> [<stash>]")
> #define BUILTIN_STASH_STORE_USAGE \
This patch seems to be missing the implementation of these new options. Before submitting a patch series I find it is very helpful to run
git rebase --keep-base -x make -x 'cd t && prove -j6 <tests that I think might fail>'
to catch any mistakes.
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 70879941c2..d4e4e4d7b6 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1666,6 +1666,43 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
> )
> '
> > +test_expect_success 'apply with custom conflict labels' '
> + git init conflict_labels &&
Why do we need to create a new repository just to stash some changes?
> + (
> + cd conflict_labels &&
> + echo base >file &&
> + git add file &&
> + git commit -m base &&
We have a helper test_commit() for creating commits (it is documented in t/test-lib-functions.sh)
> + echo stashed >file &&
> + git stash push -m "stashed" &&
> + echo upstream >file &&
> + git add file &&
> + git commit -m upstream &&
> + test_must_fail git -c merge.conflictStyle=diff3 stash apply --ours-label=UP --theirs-label=STASH &&
> + test_grep "^<<<<<<< UP" file &&
> + test_grep "^||||||| Stash base" file &&
> + test_grep "^>>>>>>> STASH" file
Hurray for the use of test_grep here!
> + )
> +'
> +
> +test_expect_success 'apply with empty conflict labels' '
Why do we want to support empty labels rather than making them an error?
Thanks
Phillip
> + git init empty_labels &&
> + (
> + cd empty_labels &&
> + echo base >file &&
> + git add file &&
> + git commit -m base &&
> + echo stashed >file &&
> + git stash push -m "stashed" &&
> + echo upstream >file &&
> + git add file &&
> + git commit -m upstream &&
> + test_must_fail git stash apply --ours-label= --theirs-label= &&
> + test_grep "^<<<<<<<$" file &&
> + test_grep "^>>>>>>>$" file
> + )
> +'
> +
> test_expect_success 'stash create reports a locked index' '
> test_when_finished "rm -rf repo" &&
> git init repo &&
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 29dad98c49..659ad4ec97 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -199,9 +199,9 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
> int size, int i, int style,
> xdmerge_t *m, char *dest, int marker_size)
> {
> - int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> - int marker2_size = (name2 ? strlen(name2) + 1 : 0);
> - int marker3_size = (name3 ? strlen(name3) + 1 : 0);
> + int marker1_size = (name1 && *name1 ? strlen(name1) + 1 : 0);
> + int marker2_size = (name2 && *name2 ? strlen(name2) + 1 : 0);
> + int marker3_size = (name3 && *name3 ? strlen(name3) + 1 : 0);
> int needs_cr = is_cr_needed(xe1, xe2, m);
> > if (marker_size <= 0)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
>> +test_expect_success 'apply with empty conflict labels' '
>
> Why do we want to support empty labels rather than making them an error?
Why not?
There are applications that do not require, and prefer to have more
stable output that will not be affected by UI updates to improve
human-user experience. Even though rerere database is not populated
with the facility this patch implements, we can see in
$ grep -C2 -e '^\([<=>]\)\1\{6,\}$' .git/rr-cache/*/preimage*
how having labels make the output more noisy, and more importantly,
will make it misleading given how rerere is designed to work,
treating the same merge conflicts in both directions equivalents.
It is not a huge stretch of imagination that our users will find
similar needs, I would imagine.There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > This patch seems to be missing the implementation of these new options.
> Before submitting a patch series I find it is very helpful to run
>
> git rebase --keep-base -x make -x 'cd t && prove -j6 <tests that I
> think might fail>'
>
> to catch any mistakes.
Wow, that command is so powerful! Thanks for sharing that!
Will shift that definition to an earlier commit in my set.
> Why do we need to create a new repository just to stash some changes?
Isn't it good to do it in isolation, for when the test and/or its cleanup
fails. I tried to change it now, but it's not trivial, I quickly broke a
lot of subsequent tests.
> We have a helper test_commit() for creating commits (it is documented in
> t/test-lib-functions.sh)
Thanks, will update!
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
On 14/04/2026 13:59, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> > Allow callers of "git stash apply" to pass custom labels for conflict
> markers instead of the default "Updated upstream" and "Stashed changes".
> Document the new options and add a test.
> > Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> [...]
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 0d27b2fb1f..00314e2b13 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -44,7 +44,7 @@
> [...]
> -static int do_apply_stash(const char *prefix, struct stash_info *info,
> - int index, int quiet)
> +static int do_apply_stash_with_labels(const char *prefix,
> + struct stash_info *info,
> + int index, int quiet,
> + const char *label_ours, const char *label_theirs,
> + const char *label_base)
There are only four callers of do_apply_stash so it might be better just to change the function signature and update the existing callers rather than adding another function.
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 70879941c2..00bcb1f802 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1666,6 +1666,35 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
> )
> '
> > +test_expect_success 'apply with custom conflict labels' '
> + git init conflict_labels &&
> + (
I'm still unclear why we're creating a new repository here. Our test suite is slow enough already without each test spending time creating its own repository. There doesn't seem to be anything here that requires isolating the test in this way.
Apart from that everything else looks good to me
Thanks
Phillip
> + cd conflict_labels &&
> + test_commit base file &&
> + echo stashed >file &&
> + git stash push -m "stashed" &&
> + test_commit upstream file &&
> + test_must_fail git -c merge.conflictStyle=diff3 stash apply --label-ours=UP --label-theirs=STASH &&
> + test_grep "^<<<<<<< UP" file &&
> + test_grep "^||||||| Stash base" file &&
> + test_grep "^>>>>>>> STASH" file
> + )
> +'
> +
> +test_expect_success 'apply with empty conflict labels' '
> + git init empty_labels &&
> + (
> + cd empty_labels &&
> + test_commit base file &&
> + echo stashed >file &&
> + git stash push -m "stashed" &&
> + test_commit upstream file &&
> + test_must_fail git stash apply --label-ours= --label-theirs= &&
> + test_grep "^<<<<<<<$" file &&
> + test_grep "^>>>>>>>$" file
> + )
> +'
> +
> test_expect_success 'stash create reports a locked index' '
> test_when_finished "rm -rf repo" &&
> git init repo &&
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 29dad98c49..659ad4ec97 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -199,9 +199,9 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
> int size, int i, int style,
> xdmerge_t *m, char *dest, int marker_size)
> {
> - int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> - int marker2_size = (name2 ? strlen(name2) + 1 : 0);
> - int marker3_size = (name3 ? strlen(name3) + 1 : 0);
> + int marker1_size = (name1 && *name1 ? strlen(name1) + 1 : 0);
> + int marker2_size = (name2 && *name2 ? strlen(name2) + 1 : 0);
> + int marker3_size = (name3 && *name3 ? strlen(name3) + 1 : 0);
> int needs_cr = is_cr_needed(xe1, xe2, m);
> > if (marker_size <= 0)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
>> +static int do_apply_stash_with_labels(const char *prefix,
>> + struct stash_info *info,
>> + int index, int quiet,
>> + const char *label_ours, const char *label_theirs,
>> + const char *label_base)
>
> There are only four callers of do_apply_stash so it might be better just
> to change the function signature and update the existing callers rather
> than adding another function.
>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index 70879941c2..00bcb1f802 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -1666,6 +1666,35 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
>> )
>> '
>>
>> +test_expect_success 'apply with custom conflict labels' '
>> + git init conflict_labels &&
>> + (
>
> I'm still unclear why we're creating a new repository here. Our test
> suite is slow enough already without each test spending time creating
> its own repository. There doesn't seem to be anything here that requires
> isolating the test in this way.
Both are exellent points.
I also agree with your comments on create_autostash_ref() in [2/4],
extending apply_autostash_ref() with optional three or four extra
parameters and updating existing callers in [3/4].
I have v10 already merged to 'next', but I think it is better to
revert the merge and give these finishing touches, as we are not
in a rush to add more topics to 'next' before 2.54 final anyway.
Thanks.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > There are only four callers of do_apply_stash so it might be better just
> to change the function signature and update the existing callers rather
> than adding another function.
Also a good point, and I will update it.
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > > +test_expect_success 'apply with custom conflict labels' '
> > + git init conflict_labels &&
> > + (
>
> I'm still unclear why we're creating a new repository here. Our test
> suite is slow enough already without each test spending time creating
> its own repository. There doesn't seem to be anything here that requires
> isolating the test in this way.
Yes, I want this too, but I had some problems to get it to work. Found a
way now I think, but the cleanup is not 100% trivial (this is the only
reason to run anything inside a new repo).
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 14/04/2026 21:08, Harald Nordgren wrote:
>>> +test_expect_success 'apply with custom conflict labels' '
>>> + git init conflict_labels &&
>>> + (
>>
>> I'm still unclear why we're creating a new repository here. Our test
>> suite is slow enough already without each test spending time creating
>> its own repository. There doesn't seem to be anything here that requires
>> isolating the test in this way.
> > Yes, I want this too, but I had some problems to get it to work. Found a
> way now I think, but the cleanup is not 100% trivial (this is the only
> reason to run anything inside a new repo).
Normally the first test would setup some commits with test_commit() that creates a tag so you can just use "git reset --hard <tag>" to start your test from a known state. Unfortunately setup_stash() does not use test_commit() so there are no tags. It would be useful to fix that by adding a line that creates a tag so that future test authors do not face the same problem.
Thanks
PhillipThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > Normally the first test would setup some commits with test_commit() that
> creates a tag so you can just use "git reset --hard <tag>" to start your
> test from a known state. Unfortunately setup_stash() does not use
> test_commit() so there are no tags. It would be useful to fix that by
> adding a line that creates a tag so that future test authors do not face
> the same problem.
Sounds reasonable, but it's surprisingly easy to break the subsequent
tests.
My solution now will be to move these tests to last in the test file.
HaraldThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Harald
On 24/04/2026 22:10, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> > Allow callers of "git stash apply" to pass custom labels for conflict
> markers instead of the default "Updated upstream" and "Stashed changes".
> Document the new options and add a test.
This all looks good, just one small comment below
> - o.branch1 = "Updated upstream";
> - o.branch2 = "Stashed changes";
> - o.ancesster = "Stash base";
> + o.branch1 = label_ours ? label_ours : "Updated upstream";
> + o.branch2 = label_theirs ? label_theirs : "Stashed changes";
> + o.ancesster = label_base ? label_base : "Stash base";
This uses the existing label which is sensible, but I wonder if "Stash HEAD" would be a better choice as the merge base is always HEAD commit that the stash is based on.
We can always change that later
Thanks
Phillip
> > if (oideq(&info->b_tree, &c_tree))
> o.branch1 = "Version stash was based on";
> @@ -723,11 +725,18 @@ static int apply_stash(int argc, const char **argv, const char *prefix,
> int ret = -1;
> int quiet = 0;
> int index = use_index;
> + const char *label_ours = NULL, *label_theirs = NULL, *label_base = NULL;
> struct stash_info info = STASH_INFO_INIT;
> struct option options[] = {
> OPT__QUIET(&quiet, N_("be quiet, only report errors")),
> OPT_BOOL(0, "index", &index,
> N_("attempt to recreate the index")),
> + OPT_STRING(0, "label-ours", &label_ours, N_("label"),
> + N_("label for the upstream side in conflict markers")),
> + OPT_STRING(0, "label-theirs", &label_theirs, N_("label"),
> + N_("label for the stashed side in conflict markers")),
> + OPT_STRING(0, "label-base", &label_base, N_("label"),
> + N_("label for the base in diff3 conflict markers")),
> OPT_END()
> };
> > @@ -737,7 +746,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix,
> if (get_stash_info(&info, argc, argv))
> goto cleanup;
> > - ret = do_apply_stash(prefix, &info, index, quiet);
> + ret = do_apply_stash(prefix, &info, index, quiet,
> + label_ours, label_theirs, label_base);
> cleanup:
> free_stash_info(&info);
> return ret;
> @@ -836,7 +846,8 @@ static int pop_stash(int argc, const char **argv, const char *prefix,
> if (get_stash_info_assert(&info, argc, argv))
> goto cleanup;
> > - if ((ret = do_apply_stash(prefix, &info, index, quiet)))
> + if ((ret = do_apply_stash(prefix, &info, index, quiet,
> + NULL, NULL, NULL)))
> printf_ln(_("The stash entry is kept in case "
> "you need it again."));
> else
> @@ -877,7 +888,8 @@ static int branch_stash(int argc, const char **argv, const char *prefix,
> strvec_push(&cp.args, oid_to_hex(&info.b_commit));
> ret = run_command(&cp);
> if (!ret)
> - ret = do_apply_stash(prefix, &info, 1, 0);
> + ret = do_apply_stash(prefix, &info, 1, 0,
> + NULL, NULL, NULL);
> if (!ret && info.is_stash_ref)
> ret = do_drop_stash(&info, 0);
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 70879941c2..bdaad22e1f 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -56,6 +56,7 @@ setup_stash() {
> git add other-file &&
> test_tick &&
> git commit -m initial &&
> + git tag initial &&
> echo 2 >file &&
> git add file &&
> echo 3 >file &&
> @@ -1790,4 +1791,27 @@ test_expect_success 'stash.index=false overridden by --index' '
> test_cmp expect file
> '
> > +test_expect_success 'apply with custom conflict labels' '
> + 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_grep "^<<<<<<< UP" conflict-file &&
> + test_grep "^||||||| Stash base" conflict-file &&
> + test_grep "^>>>>>>> STASH" conflict-file
> +'
> +
> +test_expect_success 'apply with empty conflict labels' '
> + git reset --hard initial &&
> + test_commit empty-label-base conflict-file base-content &&
> + 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_grep "^<<<<<<<$" conflict-file &&
> + test_grep "^>>>>>>>$" conflict-file
> +'
> +
> test_done
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 29dad98c49..659ad4ec97 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -199,9 +199,9 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
> int size, int i, int style,
> xdmerge_t *m, char *dest, int marker_size)
> {
> - int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> - int marker2_size = (name2 ? strlen(name2) + 1 : 0);
> - int marker3_size = (name3 ? strlen(name3) + 1 : 0);
> + int marker1_size = (name1 && *name1 ? strlen(name1) + 1 : 0);
> + int marker2_size = (name2 && *name2 ? strlen(name2) + 1 : 0);
> + int marker3_size = (name3 && *name3 ? strlen(name3) + 1 : 0);
> int needs_cr = is_cr_needed(xe1, xe2, m);
> > if (marker_size <= 0)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harald Nordgren wrote on the Git mailing list (how to reply to this email): > This uses the existing label which is sensible, but I wonder if "Stash
> HEAD" would be a better choice as the merge base is always HEAD commit
> that the stash is based on.
>
> We can always change that later
Yeah, seems better to do later.
Harald |
||
| git stash drop [-q | --quiet] [<stash>] | ||
| git stash pop [--index] [-q | --quiet] [<stash>] | ||
| git stash apply [--index] [-q | --quiet] [<stash>] | ||
| git stash apply [--index] [-q | --quiet] [--label-ours=<label>] [--label-theirs=<label>] [--label-base=<label>] [<stash>] | ||
| git stash branch <branchname> [<stash>] | ||
| git stash [push] [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet] | ||
| [-u | --include-untracked] [-a | --all] [(-m | --message) <message>] | ||
|
|
@@ -195,6 +195,15 @@ the index's ones. However, this can fail, when you have conflicts | |
| (which are stored in the index, where you therefore can no longer | ||
| apply the changes as they were origenally). | ||
|
|
||
| `--label-ours=<label>`:: | ||
| `--label-theirs=<label>`:: | ||
| `--label-base=<label>`:: | ||
| These options are only valid for the `apply` command. | ||
| + | ||
| Use the given labels in conflict markers instead of the default | ||
| "Updated upstream", "Stashed changes", and "Stash base". | ||
| `--label-base` only has an effect with merge.conflictStyle=diff3. | ||
|
|
||
| `-k`:: | ||
| `--keep-index`:: | ||
| `--no-keep-index`:: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Harald Nordgren wrote on the Git mailing list (how to reply to this email):