RFC: switch: allow same-commit switch during merge if conflicts resolved

From: Tao Klerks <[email protected]>

When a "git checkout" branch-switch operation runs during a merge, the in-progress merge metadata is removed. The refusal to maintain the merge matadata makes sense if the new commit is different to the original commit,

because a merge operation against a different commit could have turned out differently / maintaining the merge relationship would be misleading. It is still a difficult-to-understand behavior for new users, however, as they would expect a switch (to a new or same-commit branch at least) to allow committing local changes "faithfully", as in the case of regular non-merge local changes.

"git switch" introduces a little more safety, by refusing to switch if there

is a merge in progress - or a number of other operations such as rebase, cherry-pick, or "git am". This is less of a nasty surprise than the merge metadata/state being silently discarded, but is still not very helpful, when a user has a complex merge resolved, and wishes to commit it to a new branch for testing.

Change the behavior of "git switch" and "git checkout" to no longer delete merge metadata, nor prohibit the switch, if a merge is in progress and the commit being switched to is the same commit the HEAD was previously set to.

Also add a warning when the merge metadata is deleted (in case of a "git checkout" to another commit) to let the user know the merge state was lost, and that "git switch" would prevent this.

Also add a warning when the merge metadata is preserved (same commit), to let the user know the commit message prepared for the merge may still refer to the previous branch.

Add tests to verify the exception works correctly, and to verify that --force always wipes out in-progress merge metadata when it is discarding worktree changes.

Signed-off-by Tao Klerks <[email protected]>

---
RFC: switch: allow same-commit switch during merge if conflicts resolved

RFC V1: proposing a limited-scope change as per mailing discussion
https://lore.kernel.org/git/CAPMMpoht4FWnv-WuSM3+Z2R4HhwFY+pahJ6zirFU-BD5r34B7Q@mail.gmail.com/T/#t

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1527%2FTaoK%2Ftao-checkout-during-merge-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1527/TaoK/tao-checkout-during-merge-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1527

 branch.c           |  7 ++++-
 branch.h           |  6 +++++
 builtin/checkout.c | 64 ++++++++++++++++++++++++++++++++++++++++++----
 t/t2060-switch.sh  | 18 ++++++++++++-
 4 files changed, 88 insertions(+), 7 deletions(-)

branch.c +6 -1

index 5aaf073dce1..8cc7fabe599 100644
@@ -812,10 +812,15 @@ void remove_merge_branch_state(struct repository *r)
 }
 
 void remove_branch_state(struct repository *r, int verbose)
+{
+       remove_branch_state_except_merge(r, verbose);
+       remove_merge_branch_state(r);
+}
+
+void remove_branch_state_except_merge(struct repository *r, int verbose)
 {
        sequencer_post_commit_cleanup(r, verbose);
        unlink(git_path_squash_msg(r));
-       remove_merge_branch_state(r);
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)

branch.h +6 -0

index ef56103c050..c73d98b8766 100644
@@ -135,6 +135,12 @@ void remove_merge_branch_state(struct repository *r);
  */
 void remove_branch_state(struct repository *r, int verbose);
 
+/*
+ * Remove information about the state of working on the current
+ * branch, *except* merge state.
+ */
+void remove_branch_state_except_merge(struct repository *r, int verbose);
+
 /*
  * Configure local branch "local" as downstream to branch "remote"
  * from remote "origin".  Used by git branch --set-upstream.

builtin/checkout.c +59 -5

index 21a4335abb0..cae54af997b 100644
@@ -985,7 +985,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
                                delete_reflog(old_branch_info->path);
                }
        }
-       remove_branch_state(the_repository, !opts->quiet);
+
+       remove_branch_state_except_merge(the_repository, !opts->quiet);
+       if (opts->force || old_branch_info->commit != new_branch_info->commit) {
+               remove_merge_branch_state(the_repository);
+       }
        strbuf_release(&msg);
        if (!opts->quiet &&
            (new_branch_info->path || (!opts->force_detach && !strcmp(new_branch_info->name, "HEAD"))))

@@ -1098,6 +1102,36 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne
        release_revisions(&revs);
 }
 
+/*
+ * Check whether we're in a merge, and if so warn - about the ongoing merge and surprising merge
+ * message if the merge state will be preserved, and about the destroyed merge state otherwise.
+ */
+static void merging_checkout_warning(const char *name, struct commit *old_commit,
+                                     struct commit *new_commit)
+{
+       struct wt_status_state state;
+       memset(&state, 0, sizeof(state));
+       wt_status_get_state(the_repository, &state, 0);
+
+       if (!state.merge_in_progress)
+       {
+               wt_status_state_free_buffers(&state);
+               return;
+       }
+
+       if (old_commit == new_commit)
+               warning(_("switching while merge-in-progress (without changing commit).\n"
+                         "An auto-generated commit message may still refer to the previous\n"
+                         "branch.\n"));
+       else
+               warning(_("switching to a different commit while while merge-in-progress;\n"
+                         "merge metadata was removed. To avoid accidentally losing merge,\n"
+                         "metadata in this way, please use \"git switch\" instead of\n"
+                         "\"git checkout\".\n"));
+
+       wt_status_state_free_buffers(&state);
+}
+
 static int switch_branches(const struct checkout_opts *opts,
                           struct branch_info *new_branch_info)
 {

@@ -1153,6 +1187,9 @@ static int switch_branches(const struct checkout_opts *opts,
        if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
                orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
 
+       if (!opts->quiet && !opts->force)
+               merging_checkout_warning(old_branch_info.name, old_branch_info.commit, new_branch_info->commit);
+
        update_refs_for_switch(opts, &old_branch_info, new_branch_info);
 
        ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);

@@ -1445,15 +1482,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)
        exit(code);
 }
 
-static void die_if_some_operation_in_progress(void)
+static void die_if_some_incompatible_operation_in_progress(struct commit *new_commit)
 {
+       /*
+        * Note: partially coordinated logic in related function
+        * merging_checkout_warning(), checking for merge_in_progress
+        * and old_commit != new_commit to issue warnings. Issuing those
+        * warnings here would be simpler to implement, but would make the
+        * language more complex to account for common situations where the
+        * switch still won't happen (namely working tree merge failure).
+        */
+
        struct wt_status_state state;
+       struct branch_info old_branch_info = { 0 };
+       struct object_id rev;
+       int flag;
 
        memset(&state, 0, sizeof(state));
        wt_status_get_state(the_repository, &state, 0);
+       memset(&old_branch_info, 0, sizeof(old_branch_info));
+       old_branch_info.path = resolve_refdup("HEAD", 0, &rev, &flag);
+       if (old_branch_info.path)
+               old_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1);
 
-       if (state.merge_in_progress)
-               die(_("cannot switch branch while merging\n"
+       if (state.merge_in_progress && old_branch_info.commit != new_commit)
+               die(_("cannot switch to a different commit while merging\n"
                      "Consider \"git merge --quit\" "
                      "or \"git worktree add\"."));
        if (state.am_in_progress)

@@ -1476,6 +1529,7 @@ static void die_if_some_operation_in_progress(void)
                warning(_("you are switching branch while bisecting"));
 
        wt_status_state_free_buffers(&state);
+       branch_info_release(&old_branch_info);
 }
 
 static int checkout_branch(struct checkout_opts *opts,

@@ -1536,7 +1590,7 @@ static int checkout_branch(struct checkout_opts *opts,
                die_expecting_a_branch(new_branch_info);
 
        if (!opts->can_switch_when_in_progress)
-               die_if_some_operation_in_progress();
+               die_if_some_incompatible_operation_in_progress(new_branch_info->commit);
 
        if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
            !opts->ignore_other_worktrees) {

t/t2060-switch.sh +17 -1

index 5a7caf958c3..9c80d469b6b 100755
@@ -111,13 +111,29 @@ test_expect_success 'guess and create branch' '
        test_cmp expected actual
 '
 
-test_expect_success 'not switching when something is in progress' '
+test_expect_success 'not switching to a different commit when something is in progress' '
        test_when_finished rm -f .git/MERGE_HEAD &&
        # fake a merge-in-progress
        cp .git/HEAD .git/MERGE_HEAD &&
        test_must_fail git switch -d @^
 '
 
+test_expect_success 'switching to same-commit when merge is in progress succeeds' '
+       test_when_finished rm -f .git/MERGE_HEAD &&
+       # fake a merge-in-progress
+       cp .git/HEAD .git/MERGE_HEAD &&
+       git switch -d @ &&
+       # confirm the merge-in-progress is still there
+       test -e .git/MERGE_HEAD
+'
+test_expect_success 'switching with --force removes merge state' '
+       test_when_finished rm -f .git/MERGE_HEAD &&
+       # fake a merge-in-progress
+       cp .git/HEAD .git/MERGE_HEAD &&
+       git switch --force -d @ &&
+       # confirm the merge-in-progress is removed
+       test ! -e .git/MERGE_HEAD
+'
 test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
        # default config does not copy tracking info
        git switch -c foo-no-inherit foo &&

base-commit: 950264636c68591989456e3ba0a5442f93152c1a --
gitgitgadget

By the way, it was a problem that git-checkout wasn't updated to have the same safety that git-switch has. We should fix that. (It's on my todo list, along with adding other prevent-erroneous-command-while-in-middle-of-other-operation cases.)

I'm worried this is likely to lead us into confusing UI mismatches, and makes it harder to understand the appropriate rules of what can and cannot be done. A very simple "no switching branches in the middle of operations" is a very simple rule, and saves users from lots of headaches.

Granted, expert users may understand that with the commit being the same, there is no issue. But expert users can use git update-ref to tweak HEAD, or edit .git/HEAD directly, and accept the consequences. Why do we need to confuse the UI for the sake of expert users who already have an escape hatch?

More importantly, though...

Quoted 3 lines

Change the behavior of "git switch" and "git checkout" to no longer delete merge metadata, nor prohibit the switch, if a merge is in progress and the commit being switched to is the same commit the HEAD was previously set to.

Even if there are conflicts? For rebases, cherry-picks, ams, and reverts too? (Does allowing this during rebases and whatnot mean that --abort becomes really funny? Does it mean that some commits are applied to one branch, and all commits are applied to another? What about autostashes? Does it interact weirdly with --update-refs? etc.)

I think this change is premature unless it discusses all these cases,

Quoted 4 lines · Elijah Newren

By the way, it was a problem that git-checkout wasn't updated to have the same safety that git-switch has. We should fix that. (It's on my todo list, along with adding other prevent-erroneous-command-while-in-middle-of-other-operation cases.)

Yes.

Quoted 18 lines

I'm worried this is likely to lead us into confusing UI mismatches, and makes it harder to understand the appropriate rules of what can and cannot be done. A very simple "no switching branches in the middle of operations" is a very simple rule, and saves users from lots of headaches.

Granted, expert users may understand that with the commit being the same, there is no issue. But expert users can use git update-ref to tweak HEAD, or edit .git/HEAD directly, and accept the consequences. Why do we need to confuse the UI for the sake of expert users who already have an escape hatch?

More importantly, though...

Quoted 3 lines

Change the behavior of "git switch" and "git checkout" to no longer delete merge metadata, nor prohibit the switch, if a merge is in progress and the commit being switched to is the same commit the HEAD was previously set to.

Quoted 8 lines

Even if there are conflicts? For rebases, cherry-picks, ams, and reverts too? (Does allowing this during rebases and whatnot mean that --abort becomes really funny? Does it mean that some commits are applied to one branch, and all commits are applied to another? What about autostashes? Does it interact weirdly with --update-refs? etc.)

I think this change is premature unless it discusses all these cases,

It is pretty much what I wanted to say about why we haven't done this in <https://lore.kernel.org/git/[email protected]/>,
so it makes two of us ;-). I didn't look at Tao's RFC patch but if the way it determines "we are in a middle of conflicted merge and we'll allow switching to the same commit only in this case" were "the index has an unmerged entry", then it is an overly broad test and the consequences of allowing the switch for these other merge-y operations that are ongoing must be evaluated.

Quoted 6 lines · Tao Klerks via GitGitGadget

From: Tao Klerks <[email protected]>

When a "git checkout" branch-switch operation runs during a merge, the in-progress merge metadata is removed. The refusal to maintain the merge matadata makes sense if the new commit is different to the original commit,

s/matadata/metadata/

Quoted 9 lines

because a merge operation against a different commit could have turned out differently / maintaining the merge relationship would be misleading. It is still a difficult-to-understand behavior for new users, however, as they would expect a switch (to a new or same-commit branch at least) to allow committing local changes "faithfully", as in the case of regular non-merge local changes.

"git switch" introduces a little more safety, by refusing to switch if there

s/little/lot/ or s/a little//

By the way, it was a problem that git-checkout wasn't updated to have the same safety that git-switch has. We should fix that. (It's on my todo list, along with adding other prevent-erroneous-command-while-in-middle-of-other-operation cases.)

Quoted 5 lines

is a merge in progress - or a number of other operations such as rebase, cherry-pick, or "git am". This is less of a nasty surprise than the merge metadata/state being silently discarded, but is still not very helpful, when a user has a complex merge resolved, and wishes to commit it to a new branch for testing.

I'm worried this is likely to lead us into confusing UI mismatches, and makes it harder to understand the appropriate rules of what can and cannot be done. A very simple "no switching branches in the middle of operations" is a very simple rule, and saves users from lots of headaches.

Granted, expert users may understand that with the commit being the same, there is no issue. But expert users can use git update-ref to tweak HEAD, or edit .git/HEAD directly, and accept the consequences. Why do we need to confuse the UI for the sake of expert users who already have an escape hatch?

More importantly, though...

Quoted 3 lines

Change the behavior of "git switch" and "git checkout" to no longer delete merge metadata, nor prohibit the switch, if a merge is in progress and the commit being switched to is the same commit the HEAD was previously set to.

Even if there are conflicts? For rebases, cherry-picks, ams, and reverts too? (Does allowing this during rebases and whatnot mean that --abort becomes really funny? Does it mean that some commits are applied to one branch, and all commits are applied to another? What about autostashes? Does it interact weirdly with --update-refs? etc.)

I think this change is premature unless it discusses all these cases, because UI backward-compatibility requirements means we can't rip this out later if we add it, and any change here is going to lead to questions about either inconsistencies in the UI for other operations (why can't I also switch branches if there are conflicts? why can't I also switch branches to a same-commit branch during a rebase?) or crazy problems we've introduced (git rebase --abort only aborted changes to one of the branches I modified?? Which of the three branches -- the one I started on, the one I was rebasing, or the one I switched to in the middle -- is my autostash now found on??) by opening this can of worms.

My first gut guess is that switching with conflicts would be just as safe as this is, and any users who likes your change is going to complain if we don't allow it during conflicts.

But I think it'd take
a fair amount of work to figure out if it's safe during rebase/cherry-pick/am/revert (is it only okay on the very first patch of a series? And only if non-interactive? And only without --autostash and --update-refs? etc.), and whether the ending set of rules feels horribly inconsistent or feels fine to support.

Quoted 4 lines

Also add a warning when the merge metadata is deleted (in case of a "git checkout" to another commit) to let the user know the merge state was lost, and that "git switch" would prevent this.

If we're touching this area, we should employ the right fix rather than a half measure. As I mentioned above, this should be an error with the operation prevented -- just like switch behaves.

Quoted 3 lines

Also add a warning when the merge metadata is preserved (same commit), to let the user know the commit message prepared for the merge may still refer to the previous branch.

So, it's not entirely safe even when the commit of the target branch matches HEAD? Is that perhaps reason to just leave this for expert users to use the update-refs workaround?

Please let me know whether you would be comfortable with a patch that:
* Fixed checkout to be more restrictive (except still allowing --force at least on a merging state)
[...]

ASIDE: I realized today that the warnings in die_if_some_operation_in_progress() suggest "--quit" (potentially leaving a conflicted index) and do not mention "--abort". Is there any objection to beefing up these messages a bit to offer both options?

Quoted 5 lines

By the way, it was a problem that git-checkout wasn't updated to have the same safety that git-switch has. We should fix that. (It's on my todo list, along with adding other prevent-erroneous-command-while-in-middle-of-other-operation cases.)

This surprises me because the difference in "safety" is very explicitly expressed and implemented in an option "can_switch_when_in_progress", which is driven purely by "checkout vs switch", and determines whether the validations in die_if_some_operation_in_progress() apply - dying on merge, am, rebase cherry-pick, or revert.

If we are comfortable changing the behavior of branch checkout to be safe-and-limiting like switch, then that should be almost as simple as removing that condition.

The wrinkle is that I believe "--force"
should still be allowed at least in the cases where it is safe (whereas currently switch does *not* allow even --force if a merge is in progress, and this proposed patch accidentally "fixed" that for the same-commit case only).

Quoted 7 lines

More importantly, though...

Quoted 3 lines

Change the behavior of "git switch" and "git checkout" to no longer delete merge metadata, nor prohibit the switch, if a merge is in progress and the commit being switched to is the same commit the HEAD was previously set to.

Quoted 6 lines

Even if there are conflicts? For rebases, cherry-picks, ams, and reverts too? (Does allowing this during rebases and whatnot mean that --abort becomes really funny? Does it mean that some commits are applied to one branch, and all commits are applied to another? What about autostashes? Does it interact weirdly with --update-refs? etc.)

I believe this question was resolved later in the thread. The proposal is to allow the simplest case of merge only, for resolved (unconflicted) indexes only. If the change were to make sense I could update this message to be clearer that none of those other operations or situations are impacted by this change.

Quoted 3 lines

My first gut guess is that switching with conflicts would be just as safe as this is, and any users who likes your change is going to complain if we don't allow it during conflicts.

In principle I believe so too, I just haven't checked whether the tree-merge process attempts to do anything for a same-commit switch, and if it does, whether the presence of conflict data "bothers" it in any way / causes it to do the wrong thing, eg remove it.

If verifying this and opening up the "pending conflicts" case meets the consistency itch, I'm happy to explore this area and (try to) expand the scope of the fix/exemption.

Quoted 6 lines

But I think it'd take
a fair amount of work to figure out if it's safe during rebase/cherry-pick/am/revert (is it only okay on the very first patch of a series? And only if non-interactive? And only without --autostash and --update-refs? etc.), and whether the ending set of rules feels horribly inconsistent or feels fine to support.

I agree this gets complicated - I haven't thought or explored through most of these, but I have confirmed that switching branch in the middle of a *rebase* is very confusing: your rebase continues on the new HEAD, as you continue to commit, your rebased commits get committed to the branch you switched to, but at the end when you *complete* the rebase, the original ref you were rebasing still ends up being pointed to the new HEAD - so you end up with *both* the branch you were rebasing, and the branch you switched to along the way, pointing to the same head commit.

I understand how that works in terms of git's internal logic, but as a user of rebase, if I tried to switch (to a new branch) in the middle, I would be intending to say "I got scared of the changes I'm making here, I want the that is ref pointed to the new commit graph at the end of the process to be this new ref, instead of the ref I originally started on".

Supporting that usecase, for rebase, sounds to me like it should be done by something completely different to "git switch". The most helpful behavior I can think of here would be that a "git switch" attempt would say "cannot switch branch in the middle of a rebase. to continue your rebase and create a new branch, use 'git rebase --make-new-branch NEWBRANCHNAME" instead of 'git switch'"

Quoted 4 lines

Quoted 3 lines

Also add a warning when the merge metadata is deleted (in case of a "git checkout" to another commit) to let the user know the merge state was lost, and that "git switch" would prevent this.

Quoted 4 lines

If we're touching this area, we should employ the right fix rather than a half measure. As I mentioned above, this should be an error with the operation prevented -- just like switch behaves.

My understanding, given the code organization, was that we wanted to preserve current (funky) behavior for backwards-compatibility purposes.

If we're comfortable changing behavior here, I am happy to change the patch (while keeping/allowing the --force exemption, which *should* still destroy the merge state).

Quoted 8 lines

Quoted 3 lines

Also add a warning when the merge metadata is preserved (same commit), to let the user know the commit message prepared for the merge may still refer to the previous branch.

So, it's not entirely safe even when the commit of the target branch matches HEAD? Is that perhaps reason to just leave this for expert users to use the update-refs workaround?

It is *safe*, it's just that one aspect of the outcome is *potentially confusing*. You really did do the merge on the original branch. The merge message is the same as it would be if you committed, created a new branch, and reset the original branch.

(and just to note - the reasonable workaround is to commit the merge on the current "wrong" branch, create the other branch, and then reset the original branch, as Chris Torek shows on StackOverflow; not to teach people all about update-refs)

Thanks so much for taking the time to go through all this!

Please let me know whether you would be comfortable with a patch that:
* Fixed checkout to be more restrictive

(except still allowing --force at least on a merging state)

* More explicitly noted that we are relaxing things for merge only, none of the other in-progress states that currently prevent switch

* Also worked with outstanding conflicts in the index (verifying that this is safe)

Quoted 3 lines · Tao Klerks

If we are comfortable changing the behavior of branch checkout to be safe-and-limiting like switch, then that should be almost as simple as removing that condition.

I've never heard a dissenting vote against this

Quoted 6 lines · Elijah Newren

Quoted 3 lines · Tao Klerks

If we are comfortable changing the behavior of branch checkout to be safe-and-limiting like switch, then that should be almost as simple as removing that condition.

Quoted 1 line

I've never heard a dissenting vote against this

Here is my dissenting vote: I'm against this change.

If I want to use a high-level command meant for novices, I use git switch. If instead I simply want to switch to a different commit and I want git to shut up about it, then I use git checkout.

You want to strip away the options for experts, in search for what?

If there was a way of doing:

git -c core.iknowwhatimdoing=true checkout $whatever

Then I wouldn't oppose such change.

But this is not the proposal. The proposal is to break backwards compatibility for expert users with no way to retain the existing behavior.

Generally, breaking backwards compatibility for no reason is frowned upon.

Quoted 7 lines · Felipe Contreras

Quoted 6 lines · Elijah Newren

Quoted 3 lines · Tao Klerks

If we are comfortable changing the behavior of branch checkout to be safe-and-limiting like switch, then that should be almost as simple as removing that condition.

Quoted 7 lines

Quoted 1 line

I've never heard a dissenting vote against this

Here is my dissenting vote: I'm against this change.

If I want to use a high-level command meant for novices, I use git switch. If instead I simply want to switch to a different commit and I want git to shut up about it, then I use git checkout.

Thank you for your perspective on the relationship between these commands.

I don't fully share this perspective, in two ways:
- In my experience most novices don't see or know about "git switch" at all - the vast majority of the internet is still stuck on "git checkout", as are existing users. Google search result counts are of course a poor metric of anything, but compare 100k for "git switch" to 2.4M for "git checkout".

- As far as I can tell, "git switch" and "git restore" have exactly the same power and expressiveness (except specifically the lack of "git switch --force" support for bulldozing ongoing merges) - they are just as much "expert" tools as "git checkout"; the main way they differ is that they are clearer about what they're doing / what they're for.

I'd love to see "git checkout" deprecated one day, although I'm not sure I'll live to see it happen :)

Quoted 5 lines

If there was a way of doing:

git -c core.iknowwhatimdoing=true checkout $whatever

Then I wouldn't oppose such change.

I know I keep wavering back and forth on this, my apologies for my
inconstancy: *I once again think adding support for "--force" (to checkout and switch) with ongoing operations makes sense.*

This does not achieve exactly what you seem to be suggesting above, for two reasons:
1. It could not be implicit in config, but rather would need to be explicit in the command
2. The outcome of using --force is not exactly the same as "git checkout" without it (but that's a good thing)

I would (and will) argue that not achieving exactly what you propose *is OK* because the behavior of "git checkout", without "--force", when there is a (merge, rebase, cherry-pick, am, bisect) operation in course, especially the way that behavior differs from when "--force" is specified, is *not useful* - even to expert users.

Quoted 17 lines · Tao Klerks

Quoted 7 lines · Felipe Contreras

Quoted 6 lines · Elijah Newren

Quoted 3 lines · Tao Klerks

If we are comfortable changing the behavior of branch checkout to be safe-and-limiting like switch, then that should be almost as simple as removing that condition.

Quoted 7 lines

Quoted 1 line

I've never heard a dissenting vote against this

Here is my dissenting vote: I'm against this change.

If I want to use a high-level command meant for novices, I use git switch. If instead I simply want to switch to a different commit and I want git to shut up about it, then I use git checkout.

Quoted 8 lines

Thank you for your perspective on the relationship between these commands.

I don't fully share this perspective, in two ways:
- In my experience most novices don't see or know about "git switch" at all - the vast majority of the internet is still stuck on "git checkout", as are existing users. Google search result counts are of course a poor metric of anything, but compare 100k for "git switch" to 2.4M for "git checkout".

Yes, but that's something for the Git community to fix.

Why can't the git developers communicate effectively with the user base?

Quoted 7 lines

- As far as I can tell, "git switch" and "git restore" have exactly the same power and expressiveness (except specifically the lack of "git switch --force" support for bulldozing ongoing merges) - they are just as much "expert" tools as "git checkout"; the main way they differ is that they are clearer about what they're doing / what they're for.

That is not true, you can't do git switch master^0 because that would be potentially confusing to new users, but you can do the same with git checkout.

Quoted 18 lines

Quoted 6 lines

If there was a way of doing:

git -c core.iknowwhatimdoing=true checkout $whatever

Then I wouldn't oppose such change.

I know I keep wavering back and forth on this, my apologies for my
inconstancy: *I once again think adding support for "--force" (to checkout and switch) with ongoing operations makes sense.*

This does not achieve exactly what you seem to be suggesting above, for two reasons:
1. It could not be implicit in config, but rather would need to be explicit in the command
2. The outcome of using --force is not exactly the same as "git checkout" without it (but that's a good thing)

Quoted 5 lines

I would (and will) argue that not achieving exactly what you propose *is OK* because the behavior of "git checkout", without "--force", when there is a (merge, rebase, cherry-pick, am, bisect) operation in course, especially the way that behavior differs from when "--force" is specified, is *not useful* - even to expert users.

OK. That may be the case.

But it wouldn't be the first time some operation is considered not useful, and then it turns out people did in fact use it.

I would be much more confortable if there was a way to retain the current behavior, but if we are 99.99% positive nobody is actually relying on this behavior, we could chose to roll the die and see what happens (hopefully nobody will shout).

But if that's the case, I think this is something that should be a conscious decision that is extremely clear in the commit message.

The question, I understand, is whether there should be a "git -c core.suckycheckoutstatemanagement=true checkout" option *just in case*, so any affected automation users could set it, fix their affected automated processes, and then remove it, before we finally remove the "core.suckycheckoutstatemanagement" option in a subsequent release.

Quoted 29 lines · Felipe Contreras

Quoted 17 lines · Tao Klerks

Quoted 7 lines · Felipe Contreras

Quoted 6 lines · Elijah Newren

Quoted 3 lines · Tao Klerks

If we are comfortable changing the behavior of branch checkout to be safe-and-limiting like switch, then that should be almost as simple as removing that condition.

Quoted 7 lines

Quoted 1 line

I've never heard a dissenting vote against this

Here is my dissenting vote: I'm against this change.

If I want to use a high-level command meant for novices, I use git switch. If instead I simply want to switch to a different commit and I want git to shut up about it, then I use git checkout.

Quoted 8 lines

Thank you for your perspective on the relationship between these commands.

I don't fully share this perspective, in two ways:
- In my experience most novices don't see or know about "git switch" at all - the vast majority of the internet is still stuck on "git checkout", as are existing users. Google search result counts are of course a poor metric of anything, but compare 100k for "git switch" to 2.4M for "git checkout".

Yes, but that's something for the Git community to fix.

Quoted 1 line

Why can't the git developers communicate effectively with the user base?

Emm... I'm going to take that as a rhetorical question, since you've been around these parts for a lot longer than I have :)

(I have opinions, but they are not pertinent to this thread, and I don't have meaningful solutions)

Quoted 7 lines

Quoted 6 lines

- As far as I can tell, "git switch" and "git restore" have exactly the same power and expressiveness (except specifically the lack of "git switch --force" support for bulldozing ongoing merges) - they are just as much "expert" tools as "git checkout"; the main way they differ is that they are clearer about what they're doing / what they're for.

Quoted 3 lines

That is not true, you can't do git switch master^0 because that would be potentially confusing to new users, but you can do the same with git checkout.

Ah, I see your point - git switch requires you to be more verbose in this case, specifying an extra --detach.

Quoted 25 lines

Quoted 17 lines

Quoted 5 lines

If there was a way of doing:

git -c core.iknowwhatimdoing=true checkout $whatever

Then I wouldn't oppose such change.

I know I keep wavering back and forth on this, my apologies for my
inconstancy: *I once again think adding support for "--force" (to checkout and switch) with ongoing operations makes sense.*

This does not achieve exactly what you seem to be suggesting above, for two reasons:
1. It could not be implicit in config, but rather would need to be explicit in the command
2. The outcome of using --force is not exactly the same as "git checkout" without it (but that's a good thing)

Quoted 5 lines

I would (and will) argue that not achieving exactly what you propose *is OK* because the behavior of "git checkout", without "--force", when there is a (merge, rebase, cherry-pick, am, bisect) operation in course, especially the way that behavior differs from when "--force" is specified, is *not useful* - even to expert users.

OK. That may be the case.

Quoted 7 lines

But it wouldn't be the first time some operation is considered not useful, and then it turns out people did in fact use it.

I would be much more confortable if there was a way to retain the current behavior, but if we are 99.99% positive nobody is actually relying on this behavior, we could chose to roll the die and see what happens (hopefully nobody will shout).

It sounds like you're distinguishing here between "options for experts" (which should be valuable to warrant influencing the long-term design) and "behavior that users and systems may have come to rely on".

As I've argued here, I believe that the current behavior is not *useful*, and thus a "but the expert users" argument doesn't sway me at all...

I can't imagine this pattern being used in real-life automation, but like anyone my imagination is limited.

Here is precisely where I don't know how to judge "breakage risk and value of being able to revert behavior without downgrading git" vs "complexity of implementation and communication". Obviously I would prefer not to do a bunch of valueless work implementing and supporting an option that no-one would ever use, and removing it a couple months later.

Quoted 2 lines

But if that's the case, I think this is something that should be a conscious decision that is extremely clear in the commit message.

Quoted 6 lines · Tao Klerks

I believe this question was resolved later in the thread. The proposal is to allow the simplest case of merge only, for resolved (unconflicted) indexes only. If the change were to make sense I could update this message to be clearer that none of those other operations or situations are impacted by this change.

As I mentioned to Junio, I understood fully that your implementation limited the changes to this one case. That did not resolve my concerns, it merely obviated some other bigger ones that I didn't raise.

However, making it only available via a --force override (and then perhaps also limiting it to just some operations), would resolve my concerns.

Quoted 9 lines

Quoted 3 lines

My first gut guess is that switching with conflicts would be just as safe as this is, and any users who likes your change is going to complain if we don't allow it during conflicts.

In principle I believe so too, I just haven't checked whether the tree-merge process attempts to do anything for a same-commit switch, and if it does, whether the presence of conflict data "bothers" it in any way / causes it to do the wrong thing, eg remove it.

Quoted 3 lines

If verifying this and opening up the "pending conflicts" case meets the consistency itch, I'm happy to explore this area and (try to) expand the scope of the fix/exemption.

If this behavior is behind a --force flag rather than the default behavior, then I think there's much more leniency for a partial solution.

That said, I do still think it'd be nice to handle this case for consistency, so if you're willing to take a look, that'd be great. If you are interested, here's a pointer: Stolee's commit 313677627a8 ("checkout: add simple check for 'git checkout -b'", 2019-08-29) might be of interest here. Essentially, when switching to a same-commit branch, you can short-circuit most of the work and basically just update HEAD. (In his case, he was creating _and_ switching to a branch, and he was essentially just trying to short-circuit the reading and writing of the index since he knew there would be no changes, but the same basic logic almost certainly applies to this broader case -- no index changes are needed, so the existence of conflicts shouldn't matter.)

If you don't want to handle that case, though, you should probably think about what kind of message to give the user if they try to --force the checkout and they have conflicts. They'd probably deserve a longer explanation due to the inconsistency.

Quoted 24 lines

Quoted 6 lines

But I think it'd take
a fair amount of work to figure out if it's safe during rebase/cherry-pick/am/revert (is it only okay on the very first patch of a series? And only if non-interactive? And only without --autostash and --update-refs? etc.), and whether the ending set of rules feels horribly inconsistent or feels fine to support.

I agree this gets complicated - I haven't thought or explored through most of these, but I have confirmed that switching branch in the middle of a *rebase* is very confusing: your rebase continues on the new HEAD, as you continue to commit, your rebased commits get committed to the branch you switched to, but at the end when you *complete* the rebase, the original ref you were rebasing still ends up being pointed to the new HEAD - so you end up with *both* the branch you were rebasing, and the branch you switched to along the way, pointing to the same head commit.

I understand how that works in terms of git's internal logic, but as a user of rebase, if I tried to switch (to a new branch) in the middle, I would be intending to say "I got scared of the changes I'm making here, I want the that is ref pointed to the new commit graph at the end of the process to be this new ref, instead of the ref I originally started on".

Quoted 6 lines

Supporting that usecase, for rebase, sounds to me like it should be done by something completely different to "git switch". The most helpful behavior I can think of here would be that a "git switch" attempt would say "cannot switch branch in the middle of a rebase. to continue your rebase and create a new branch, use 'git rebase --make-new-branch NEWBRANCHNAME" instead of 'git switch'"

That all sounds reasonable.

But you know someone is going to try it anyway during a rebase/cherry-pick/revert. If we start letting --force override during a merge, we should do something to address that inconsistency for users. It doesn't need to be something big; we could likely address it by just specifically checking for the --force case during a rebase/cherry-pick/revert and providing an even more detailed error message in that case that spells out why the operation cannot be --forced.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Also add a warning when the merge metadata is deleted (in case of a "git checkout" to another commit) to let the user know the merge state was lost, and that "git switch" would prevent this.

Quoted 8 lines

Quoted 4 lines

If we're touching this area, we should employ the right fix rather than a half measure. As I mentioned above, this should be an error with the operation prevented -- just like switch behaves.

My understanding, given the code organization, was that we wanted to preserve current (funky) behavior for backwards-compatibility purposes.

I totally understand how you'd reach that conclusion. I would probably come to the same one reading the code for the first time. But, as it turns out, that's not how things happened.

Quoted 3 lines

If we're comfortable changing behavior here, I am happy to change the patch (while keeping/allowing the --force exemption, which *should* still destroy the merge state).

Yaay!

Quoted 21 lines

Quoted 9 lines

Quoted 4 lines

Also add a warning when the merge metadata is preserved (same commit), to let the user know the commit message prepared for the merge may still refer to the previous branch.

So, it's not entirely safe even when the commit of the target branch matches HEAD? Is that perhaps reason to just leave this for expert users to use the update-refs workaround?

It is *safe*, it's just that one aspect of the outcome is *potentially confusing*. You really did do the merge on the original branch. The merge message is the same as it would be if you committed, created a new branch, and reset the original branch.

(and just to note - the reasonable workaround is to commit the merge on the current "wrong" branch, create the other branch, and then reset the original branch, as Chris Torek shows on StackOverflow; not to teach people all about update-refs)

Quoted 4 lines

Thanks so much for taking the time to go through all this!

Please let me know whether you would be comfortable with a patch that:
* Fixed checkout to be more restrictive

Absolutely.

Quoted 1 line

(except still allowing --force at least on a merging state)

That's fine too.

Quoted 2 lines

* More explicitly noted that we are relaxing things for merge only, none of the other in-progress states that currently prevent switch

That wouldn't resolve any of my concerns; it was totally clear to me the first time.

Quoted 2 lines

* Also worked with outstanding conflicts in the index (verifying that this is safe)

In combination with --force, I think that would be very nice.

Quoted 9 lines · Elijah Newren

Quoted 6 lines · Tao Klerks

I believe this question was resolved later in the thread. The proposal is to allow the simplest case of merge only, for resolved (unconflicted) indexes only. If the change were to make sense I could update this message to be clearer that none of those other operations or situations are impacted by this change.

Quoted 9 lines

As I mentioned to Junio, I understood fully that your implementation limited the changes to this one case. That did not resolve my concerns, it merely obviated some other bigger ones that I didn't raise.

However, making it only available via a --force override (and then perhaps also limiting it to just some operations), would resolve my concerns.

Hmm, I think there is confusion here.

My proposal was (and now, again, is) to add support for "--force" to "git switch", and to keep and improve that existing support for "git checkout" (where it is in my opinion broken during a rebase), but that proposal was mostly-unrelated to my main goal and proposal for supporting same-commit switches in the first place:

A same-commit switch (*without* --force) serves the use-case of *completing a merge on another branch*. This is, as far as I can tell only *useful* for merges:
* during a rebase, switching in the middle (to the same commit,
without --force) won't achieve anything useful; your rebase is still in progress, any previously rebased commits in the sequence are lost, and if you continue the rebase you'll end up with a very strange and likely-surprising partial rebase state)
* during a cherry-pick, it's just "not very useful" - it's not bad
like rebase, because in-progress cherry-pick metadata is destroyed
* during am, and bisect I'm not sure, I haven't tested yet.

The reason this in-progress is *valuable* for merges (in a way that it is not for those other states) is that the merge metadata not only says what you're in the middle of, but also contains additional useful information about what you've done so far, which you want to have be a part of what you commit in the end - the identity of the commit you were merging in.

Supporting switch with --force, and having it implicitly destroy in-progress operation metadata, has value in that it makes it easier to break backwards compatibility of "git checkout" without impacting users' or tests' workflows; it helps make a change to make checkout safer; but it does not help with my other (/main?) objective of making it easy and intuitive to switch to another same-commit branch, to be able to commit your in-progress merge on another branch and avoid committing it where you started.

Hence, if/when we add support for same-commit switching during merge (and potentially other operations, if that makes sense), it should *not* take "--force", which has a substantially different purpose and meaning.

Quoted 9 lines · Elijah Newren

Quoted 6 lines · Tao Klerks

I believe this question was resolved later in the thread. The proposal is to allow the simplest case of merge only, for resolved (unconflicted) indexes only. If the change were to make sense I could update this message to be clearer that none of those other operations or situations are impacted by this change.

Quoted 9 lines

As I mentioned to Junio, I understood fully that your implementation limited the changes to this one case. That did not resolve my concerns, it merely obviated some other bigger ones that I didn't raise.

However, making it only available via a --force override (and then perhaps also limiting it to just some operations), would resolve my concerns.

Hmm, I think there is confusion here.

My proposal was (and now, again, is) to add support for "--force" to "git switch", and to keep and improve that existing support for "git checkout" (where it is in my opinion broken during a rebase), but that proposal was mostly-unrelated to my main goal and proposal for supporting same-commit switches in the first place:

A same-commit switch (*without* --force) serves the use-case of *completing a merge on another branch*. This is, as far as I can tell only *useful* for merges:
* during a rebase, switching in the middle (to the same commit,
without --force) won't achieve anything useful; your rebase is still in progress, any previously rebased commits in the sequence are lost, and if you continue the rebase you'll end up with a very strange and likely-surprising partial rebase state)
* during a cherry-pick, it's just "not very useful" - it's not bad
like rebase, because in-progress cherry-pick metadata is destroyed
* during am, and bisect I'm not sure, I haven't tested yet.

The reason this in-progress is *valuable* for merges (in a way that it is not for those other states) is that the merge metadata not only says what you're in the middle of, but also contains additional useful information about what you've done so far, which you want to have be a part of what you commit in the end - the identity of the commit you were merging in.

Supporting switch with --force, and having it implicitly destroy in-progress operation metadata, has value in that it makes it easier to break backwards compatibility of "git checkout" without impacting users' or tests' workflows; it helps make a change to make checkout safer; but it does not help with my other (/main?) objective of making it easy and intuitive to switch to another same-commit branch, to be able to commit your in-progress merge on another branch and avoid committing it where you started.

Hence, if/when we add support for same-commit switching during merge (and potentially other operations, if that makes sense), it should *not* take "--force", which has a substantially different purpose and meaning.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

My first gut guess is that switching with conflicts would be just as safe as this is, and any users who likes your change is going to complain if we don't allow it during conflicts.

In principle I believe so too, I just haven't checked whether the tree-merge process attempts to do anything for a same-commit switch, and if it does, whether the presence of conflict data "bothers" it in any way / causes it to do the wrong thing, eg remove it.

Quoted 7 lines

Quoted 3 lines

If verifying this and opening up the "pending conflicts" case meets the consistency itch, I'm happy to explore this area and (try to) expand the scope of the fix/exemption.

If this behavior is behind a --force flag rather than the default behavior, then I think there's much more leniency for a partial solution.

But if it were behind "--force", it wouldn't work :)

Quoted 13 lines

That said, I do still think it'd be nice to handle this case for consistency, so if you're willing to take a look, that'd be great. If you are interested, here's a pointer: Stolee's commit 313677627a8 ("checkout: add simple check for 'git checkout -b'", 2019-08-29) might be of interest here. Essentially, when switching to a same-commit branch, you can short-circuit most of the work and basically just update HEAD. (In his case, he was creating _and_ switching to a branch, and he was essentially just trying to short-circuit the reading and writing of the index since he knew there would be no changes, but the same basic logic almost certainly applies to this broader case -- no index changes are needed, so the existence of conflicts shouldn't matter.)

Will look, thx

Quoted 6 lines

If you don't want to handle that case, though, you should probably think about what kind of message to give the user if they try to --force the checkout and they have conflicts. They'd probably deserve a longer explanation due to the inconsistency.

--force implicitly and intentionally discards the conflicts.

Quoted 33 lines

Quoted 24 lines

Quoted 6 lines

But I think it'd take
a fair amount of work to figure out if it's safe during rebase/cherry-pick/am/revert (is it only okay on the very first patch of a series? And only if non-interactive? And only without --autostash and --update-refs? etc.), and whether the ending set of rules feels horribly inconsistent or feels fine to support.

I agree this gets complicated - I haven't thought or explored through most of these, but I have confirmed that switching branch in the middle of a *rebase* is very confusing: your rebase continues on the new HEAD, as you continue to commit, your rebased commits get committed to the branch you switched to, but at the end when you *complete* the rebase, the original ref you were rebasing still ends up being pointed to the new HEAD - so you end up with *both* the branch you were rebasing, and the branch you switched to along the way, pointing to the same head commit.

I understand how that works in terms of git's internal logic, but as a user of rebase, if I tried to switch (to a new branch) in the middle, I would be intending to say "I got scared of the changes I'm making here, I want the that is ref pointed to the new commit graph at the end of the process to be this new ref, instead of the ref I originally started on".

Quoted 6 lines

Supporting that usecase, for rebase, sounds to me like it should be done by something completely different to "git switch". The most helpful behavior I can think of here would be that a "git switch" attempt would say "cannot switch branch in the middle of a rebase. to continue your rebase and create a new branch, use 'git rebase --make-new-branch NEWBRANCHNAME" instead of 'git switch'"

That all sounds reasonable.

Quoted 8 lines

But you know someone is going to try it anyway during a rebase/cherry-pick/revert. If we start letting --force override during a merge, we should do something to address that inconsistency for users. It doesn't need to be something big; we could likely address it by just specifically checking for the --force case during a rebase/cherry-pick/revert and providing an even more detailed error message in that case that spells out why the operation cannot be --forced.

The behavior of "--force" is already clear - it resets your worktree to the state of the branch you are switching to. It also (or should but doesn't, in the case of rebase) destroys in-progress operation state/metadata.

That said, I understand and agree that there should be a difference between a generic error "there is an operation in progress, you need to '--abort'" for the operation types that can and should not benefit from a same-commit exception, and the operation(s) that do get a same-commit exception when it doesn't apply (when you're trying to switch commit). If the same-commit does end up behind some parameter, there should be yet another message for a same-commit branch switch operation when the new needed parameter is not specified.

Quoted 18 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Also add a warning when the merge metadata is deleted (in case of a "git checkout" to another commit) to let the user know the merge state was lost, and that "git switch" would prevent this.

Quoted 8 lines

Quoted 4 lines

If we're touching this area, we should employ the right fix rather than a half measure. As I mentioned above, this should be an error with the operation prevented -- just like switch behaves.

My understanding, given the code organization, was that we wanted to preserve current (funky) behavior for backwards-compatibility purposes.

I totally understand how you'd reach that conclusion. I would probably come to the same one reading the code for the first time. But, as it turns out, that's not how things happened.

Quoted 5 lines

Quoted 3 lines

If we're comfortable changing behavior here, I am happy to change the patch (while keeping/allowing the --force exemption, which *should* still destroy the merge state).

Yaay!

As suggested in my recent response to Felipe, I will create a separate patch (series) for the git checkout safety enhancements and related --force support enhancements.

Quoted 35 lines

Quoted 21 lines

Quoted 9 lines

Quoted 4 lines

Also add a warning when the merge metadata is preserved (same commit), to let the user know the commit message prepared for the merge may still refer to the previous branch.

So, it's not entirely safe even when the commit of the target branch matches HEAD? Is that perhaps reason to just leave this for expert users to use the update-refs workaround?

It is *safe*, it's just that one aspect of the outcome is *potentially confusing*. You really did do the merge on the original branch. The merge message is the same as it would be if you committed, created a new branch, and reset the original branch.

(and just to note - the reasonable workaround is to commit the merge on the current "wrong" branch, create the other branch, and then reset the original branch, as Chris Torek shows on StackOverflow; not to teach people all about update-refs)

Quoted 4 lines

Thanks so much for taking the time to go through all this!

Please let me know whether you would be comfortable with a patch that:
* Fixed checkout to be more restrictive

Absolutely.

Quoted 1 line

(except still allowing --force at least on a merging state)

That's fine too.

Quoted 2 lines

* More explicitly noted that we are relaxing things for merge only, none of the other in-progress states that currently prevent switch

Quoted 7 lines

That wouldn't resolve any of my concerns; it was totally clear to me the first time.

Quoted 2 lines

* Also worked with outstanding conflicts in the index (verifying that this is safe)

In combination with --force, I think that would be very nice.

I need this to work without --force, for the reasons noted above, *but I will split this into two patch series to avoid further confusion!*

Thanks so much for your help!