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

Commit Message

Tao Klerks May 2, 2023, 6:27 a.m. UTC

From: Tao Klerks <[e-mail]>

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 <[e-mail]>
---
    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/[e-mail]/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(-)


base-commit: 950264636c68591989456e3ba0a5442f93152c1a

Comments

Elijah Newren May 2, 2023, 3:55 p.m. UTC | #1

On Mon, May 1, 2023 at 11:57 PM Tao Klerks via GitGitGadget
<[e-mail]> wrote:
>
> From: Tao Klerks <[e-mail]>
>
> 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/

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

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

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

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

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

> 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 <[e-mail]>
> ---
>     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/[e-mail]/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(-)
>
> diff --git a/branch.c b/branch.c
> index 5aaf073dce1..8cc7fabe599 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -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)
> diff --git a/branch.h b/branch.h
> index ef56103c050..c73d98b8766 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -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.
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 21a4335abb0..cae54af997b 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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) {
> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
> index 5a7caf958c3..9c80d469b6b 100755
> --- a/t/t2060-switch.sh
> +++ b/t/t2060-switch.sh
> @@ -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

Junio C Hamano May 2, 2023, 4:50 p.m. UTC | #2

Elijah Newren <[e-mail]> writes:

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

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

It is pretty much what I wanted to say about why we haven't done
this in <https://lore.kernel.org/git/xmqqpm7k6ojz.fsf@gitster.g/>,
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.

Thanks.

Elijah Newren May 3, 2023, 12:34 a.m. UTC | #3

On Tue, May 2, 2023 at 9:50 AM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> > 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.
>
> > 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...
> >
> >> 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,
>
> It is pretty much what I wanted to say about why we haven't done
> this in <https://lore.kernel.org/git/xmqqpm7k6ojz.fsf@gitster.g/>,
> 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.

He does tie it specifically to "is-this-a-merge-operation" (and
actually doesn't check for conflicts at all since there are existing
checks he leaves untouched).  That certainly prevents some problems,
but doesn't address my concerns.

I think the usecase Tao presents has multiple simple workarounds, and
I'm worried that the particular proposal might paint us into a corner.

Personally, I think that before we consider a
merge-specific-if-no-conflicts exception, someone should evaluate all
the cases where exceptions could or should be allowed, get a
documented story about them, and then if a consistent-ish UI is
possible then propose patches to start taking us down this path.

Tao Klerks May 4, 2023, 5:01 a.m. UTC | #4

On Tue, May 2, 2023 at 5:55 PM Elijah Newren <[e-mail]> wrote:
>
> On Mon, May 1, 2023 at 11:57 PM Tao Klerks via GitGitGadget
> <[e-mail]> wrote:
> >
> > From: Tao Klerks <[e-mail]>
> >
> > 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/

Thx!

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

Yep, this paragraph is straight-up wrong - safety is an entirely
separate concern from usability & meeting reasonable expectations.

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

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

I'm not convinced a simple rule that prevents a natural majority-case
workflow is better than a complex rule that allows one majority-case
workflow while prohibiting others.

My claim, which I understand is quite tenuous, is that the situation
where you find yourself legitimately thinking "wow, I just did a lot
of work, but I'm not sure it's *right*. I should test it on a new
branch *before* committing to this branch" is significantly more
likely to arise with "git merge", where what you are merging in is
often others' work, in a single step, than with those other commands.

(ignoring rebase, which may be as common, but is a completely
different workflow, where "let me save it on a new branch before I
share with others" doesn't make as much sense; in most workflows you
don't rebase shared branches)

It makes sense to say that prohibiting something that will hurt you
saves you a headache, but I think we agree that changing to a
same-commit branch during a resolved-index merge as enabled here *does
not* hurt you. And the only reasonable alternative (committing the the
original branch, *then* creating a new testing branch, and resetting
the original branch) is very non-obvious.

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

Expert users are not the targeted audience of these changes. Users
used to the natural git pattern of "I have some local uncommitted
changes, I'm not quite sure about them, so let me create a new branch
and commit them there, so I can validate them properly, and then I'll
bring them to the original branch" are.

>
> More importantly, though...
>
> > 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 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.

>
> 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 argument is that making things a little better is better than not
making them better at all (although yes, of course, ideally we would
also cover those other cases long-term, if they make logical sense). I
understand that's easy for me to say of course, as the dilletante who
pops in to make a tiny change and leaves the team on the hook to
resolve the edge-cases / address broader consistency in the long term.

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

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

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

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

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

Thanks,
Tao

Tao Klerks May 5, 2023, 5:06 a.m. UTC | #5

On Thu, May 4, 2023 at 7:01 AM Tao Klerks <[e-mail]> wrote:
>
> 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)
> [...]

Having reviewed the commit by Nguyễn Thái Ngọc Duy that introduced the
"can_switch_when_in_progress" boolean in 2019 (as part of the broader
introduction of "git switch" and "git restore"), it looks like I
should change my proposal here. I thought it made sense to continue to
support --force because we can, but I now think this is wrong,
because:
1. the fact that --force does not work in git switch is *intentional*
2. even though making it work for "merging" states would be trivial,
making it work during rebase would not be
3. the blocking of --force is not a significant inconvenience, as the
error message clearly tells you what to do (--quit), to continue on
your way

So I will set about trying to understand how to make this one-line
change work. I already see that some tests rely on "git checkout -f"
bulldozing through an ongoing merge, so those tests will need to be
adjusted at least.

Are there any recommendations or processes around breaking changes for
the git project anywhere? The specific behaviors that we would be
changing here appear to be undocumented (I've looked through
https://git-scm.com/docs/git-checkout at least and find no mention or
expectation that switching during a merge, or rebase, etc is
supported; nor do I see any explicit mention in
https://git-scm.com/docs/git-switch that it is UNsupported)


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?

Elijah Newren May 7, 2023, 2:48 a.m. UTC | #6

Hi Tao,

On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
>
[...]
> > 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.

The implementation was done that way, not because _anyone_ thought
that was the right design, but because Duy wanted to introduce a new
command without touching the existing command, so that he only had to
deal with review comments about the new command.  Two people commented
on it at the time, one of them me, saying that we should just change
checkout and fix it.  But it was decided to leave it for later...

> 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, and I've brought it
up a few times.  Junio elsewhere in this thread agrees we should just
change it.

Besides, this is typically the way backward incompatibilities are
handled in Git: First make something a warning, then make it an error,
then wait a while, then change the thing (e.g. git push defaults).
For simpler cases, you can jump straight to an error, especially when
not giving an error can hurt users and is unlikely to be what users
meant anyway.  This is a case where it can really hurt.

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

I'd be okay with an override.

[...]
> >
> > More importantly, though...
> >
> > > 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 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.

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

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.

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

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
`--force`d.

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

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

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

Absolutely.

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

That's fine too.

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

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

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

Elijah Newren May 7, 2023, 2:57 a.m. UTC | #7

On Thu, May 4, 2023 at 10:06 PM Tao Klerks <[e-mail]> wrote:
>
[...]
> 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?

Honestly, I'd prefer to just change them to --abort.

--quit is for very unusual expert situations (I did the operation,
forgot I was in the middle, did all kinds of funny resets and tweaks
and who-knows-what, and then later discovered there was an in-progress
operation I had forgotten, but I decided I liked my totally munged
state better and want to keep it while somehow marking the operation
as over.[1])  I think recommending it to users is a bit of a
disservice.  If someone feels strongly about keeping it, I'd argue for
having both --abort and --quit, with --abort more prominent.

But my first vote would be for changing these to mention --abort.  And
adding some scary warnings to the places where --quit is documented,
to recommend users consider --abort instead.


[1] That might sound like an exaggeration, but I think that's exactly
how it was advertised originally: 9512177b682 ("rebase: add --quit to
cleanup rebase, leave everything else untouched", 2016-11-12)

Felipe Contreras May 7, 2023, 10:01 p.m. UTC | #8

Elijah Newren wrote:
> On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:

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

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.

Tao Klerks May 8, 2023, 8:30 a.m. UTC | #9

On Mon, May 8, 2023 at 12:01 AM Felipe Contreras
<[e-mail]> wrote:
>
> Elijah Newren wrote:
> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
>
> > > 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
>
> 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 :)

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

What *I* want is a generally safe system, in which you don't have to
be an expert to avoid causing yourself problems, especially losing
work.

The specific example that motivated my wanting to change "git
checkout" here was the case of a normal (non-expert, non-novice) user
who is used to doing "git checkout -b
actually-those-changes-I-made-shouldnt-go-on-the-branch-I-was-working-on-yet".
In their day-to-day work, that action will always have achieved
exactly what they wanted. The day they make exactly the same
invocation just before they commit a merge, it will do something
completely different and confusing - it will destroy a merge state,
and result in the commit, shortly after, being a regular non-merge
commot. Leveraging that committed tree in a merge commit *is* indeed
an expert action, and most novice and maybe intermediate users will
instead find themselves cursing git, and starting the merge again from
scratch - if they even notice the problem. If they don't notice the
problem, then they will instead have a new and fascinating source of
merge conflicts at some future time.

In general I *will* be willing to make things a little harder for
experts in favor of novices - absolutely.

That said, I don't believe the (new) change proposed here strips away
*useful* options from experts at all. When I said "safe-and-limiting",
I meant it in the most literal way - that there are some operations
that could be performed before and would achieve certain outcomes that
won't be possible afterwards. What I didn't mean to imply is that
those options are *valuable* to anyone - even to experts.

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

I will provide a table of behaviors with a proposed patch in a few
days, but basically the main behavior we're taking away is a
one-command behavior of "switch branch and remove the (merge,
cherry-pick) in-progress state". The explicit equivalent is and will
continue to be "git [merge|cherry-pick] --quit && git checkout" -
leaving the in-progress merge changes in the index, but switching to
the specified branch.

My expectation is that this is not something even expert users find
themselves doing... ever. But I would like to know about it if I'm
wrong of course!

Something that I *do* see quite a lot in the test suite, and is
prompting my turn-about on "--force" support, is "git checkout -f
whatever" as a shorthand for "just get my worktree to the state of
that branch, regardless of the current ongoing operation".

This shorthand happens to *fail to work correctly* during a rebase
(clearing of the rebasing state was never implemented), but I believe
that has more to do with priorities and scope of changes than
intentional "let's set an extra-confusing trap for rebase" reasons.
The resulting state, where you have switched to the requested branch
and discarded any local changes, but are still in a rebase,
potentially with pending rebase sequence steps to complete, is not one
that I can see even expert users making constructive use of.

Generally, the "allow 'checkout --force' to destroy in-progress
operation states" behavior looks like an expert shortcut worth
preserving, and improving/fixing in the case of rebase.

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

It is true that the (updated) proposal closes the doors on *specific*
behaviors as a single command, requiring them to instead be spread
across two commands. However, I believe that those are effectively
*unused behaviors*, and that the increase in consistency and safety,
for all users, by far outweighs the cost of this particular break in
backwards compatibility.

I am, again, very interested in anything I might be missing!

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

I absolutely understand and agree that breaking backwards
compatibility *for no reason* is never the right thing - and I take
note that being clear about exactly what the reasons are, and what the
costs are, *before* talking about doing it and asking for opinions, is
advisable and something that I failed to do sensibly here.

Thanks again for the feedback, please let me know if you know of any
useful expert use cases that I *am* missing in this updated proposal.

Tao Klerks May 8, 2023, 10:44 a.m. UTC | #10

On Sun, May 7, 2023 at 4:48 AM Elijah Newren <[e-mail]> wrote:
>
> On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
> >
> > 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.
>

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.

> > > 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.
>
> 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 :)

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

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

> > > 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'"
>
> 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
> `--force`d.

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.

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

>
> > > > 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
>
> Absolutely.
>
> > (except still allowing --force at least on a merging state)
>
> That's fine too.
>
> > * 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.
>
> > * 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!

Felipe Contreras May 8, 2023, 4:13 p.m. UTC | #11

Tao Klerks wrote:
> On Mon, May 8, 2023 at 12:01 AM Felipe Contreras
> <[e-mail]> wrote:
> > Elijah Newren wrote:
> > > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
> >
> > > > 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
> >
> > 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".

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

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

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

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

But that's not an excuse to break user experience.

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

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.

Cheers.

Tao Klerks May 8, 2023, 4:58 p.m. UTC | #12

On Mon, May 8, 2023 at 6:13 PM Felipe Contreras
<[e-mail]> wrote:
>
> Tao Klerks wrote:
> > On Mon, May 8, 2023 at 12:01 AM Felipe Contreras
> > <[e-mail]> wrote:
> > > Elijah Newren wrote:
> > > > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
> > >
> > > > > 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
> > >
> > > 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".
>
> Yes, but that's something for the Git community to fix.
>
> 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)

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

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

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

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... On the other hand, the "we shouldn't break existing
(scripted/automated) uses" argument seems much more convincing, and
more in line with what I was fishing for in my first question about a
"breaking changes process".

I haven't found any use cases that I could imagine anyone credibly
automating against, but I did find some tests in the suite that were
doing (in my opinion) "the wrong thing" and need to be modified:

```
# fails for some reason other than conflicts, eg commit hook
git cherry-pick XXXXX

# previously succeeded, removing cherry-pick state but leaving modified index;
# will now newly fail with "you need to --abort first"
git checkout main

# cleans up modified index state
git reset --hard
```

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

Making this behave correctly, after my planned changes, is very
simple: replace "git checkout && git reset --hard" with "git checkout
-f", or even just with "git cherry-pick --abort". But it is still a
change in behavior that *could* cause breakage if anyone implemented a
corner-case cleanup process in the same way those particular tests
did. I believe this particular example is vanishingly unlikely,
*because it doesn't deal with conflicts*. If the cherry-pick had left
any conflicted files, then the checkout would have failed.

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.

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. I wonder, for example, whether there is any recommendation that
automation users be willing and able to downgrade git temporarily, or
not. That would be one way to make the risk of this kind of
"corner-case breakage" more acceptable.

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

I will do my best :)

Junio C Hamano May 8, 2023, 7:18 p.m. UTC | #13

Tao Klerks <[e-mail]> writes:

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

If a new behaviour is hidden behind a new option nobody has heard of
before, you would not risk breaking anybody who wrote their scripts
long time ago and have relied on them the way they currently work,
and the new option would not have to be removed at all.  I think the
"switch" was written exactly for such a transition so that folks who
wanted a different behaviour do not have to break existing users of
"checkout".

Do we still mark "switch" as experimental in bold red letters in the
documentation?  Then it is not too late to improve the end-user
experimence with the command without worrying about too much about
backward compatibility.

Felipe Contreras May 9, 2023, 1:55 a.m. UTC | #14

Tao Klerks wrote:
> On Mon, May 8, 2023 at 6:13 PM Felipe Contreras
> <[e-mail]> wrote:
> > Tao Klerks wrote:
> > > On Mon, May 8, 2023 at 12:01 AM Felipe Contreras
> > > <[e-mail]> wrote:
> > > > Elijah Newren wrote:
> > > > > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
> > > >
> > > > > > 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
> > > >
> > > > 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".
> >
> > Yes, but that's something for the Git community to fix.
> >
> > 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)

Yes, it was rhetorical.

That being said, if you feel like sharing that opinion off the record,
I'm interested in hearing it.

> > > - 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`.
> 
> Ah, I see your point - git switch requires you to be more verbose in
> this case, specifying an extra --detach.

Yes, because it's meant for more novice users.

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

Sure, they are different, but they are related.

If somebody has only one week of expertice with git, I think it's safe
to say they don't rely on the current behavior that much.

On the other hand somebody who has 15 years of experience with git has a
higher chance of relying on the current behavior.

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

And you may be right, I'm not going to argue against such claim.

But this is an argument from ignorance fallacy. The last time I argued
"I cannot imagine how X might be the case" turned out X was the case.

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

Indeed.

Once again: I'm not saying this is going to break user expectations,
because it might not. I'm saying this *might* break user expectations,
but we could still roll the dice and find out.

Ultimately this is not my decision, it's the decision of the maintainer.

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

Agreed, which is why I'm not suggesting this work has to be done, but
the maintainer might (I've often done what I consider unnecessary work
just because of that reason).

All I'm saying is that because the git project puts a premium on
preserving backwards compatibility (as any decent software project
should), then:

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

Cheers.

Elijah Newren May 11, 2023, 7:06 a.m. UTC | #15

On Mon, May 8, 2023 at 3:44 AM Tao Klerks <[e-mail]> wrote:
>
> On Sun, May 7, 2023 at 4:48 AM Elijah Newren <[e-mail]> wrote:
> >
> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
> > >
> > > 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.
> >
>
> 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.

Doh, sorry, brain fart on my part forgetting the checkout/switch
already have a "--force".  Replace "--force" in my email with "an
override" such as "--ignore-in-progress".

Tao Klerks May 21, 2023, 8:08 p.m. UTC | #16

FWIW, it looks like I bit off a lot more than I could chew when I
offered/proposed to fix "git checkout --force <ref>" across all the
in-progress operation types (and make it available in "git switch",
and then disallow non-force "git checkout" with in-progress
operations), especially given the 1-2 hours/week I'm managing to
spend.

I'm having enough troubles understanding all the ins & outs of the
current behavior that it will likely take me a few weeks to have any
proposed changes.

I will revive this thread at that time, if I manage to propose anything useful.

On Mon, May 8, 2023 at 12:44 PM Tao Klerks <[e-mail]> wrote:
>
> On Sun, May 7, 2023 at 4:48 AM Elijah Newren <[e-mail]> wrote:
> >
> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <[e-mail]> wrote:
> > >
> > > 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.
> >
>
> 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.
>
> > > > 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.
> >
> > 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 :)
>
> >
> > 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
>
> >
> > 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.
>
> > > > 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'"
> >
> > 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
> > `--force`d.
>
> 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.
>
> >
> > > > > 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.
> > > >
> > >
> > > 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.
> >
> > > 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.
>
> >
> > > > > 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
> >
> > Absolutely.
> >
> > > (except still allowing --force at least on a merging state)
> >
> > That's fine too.
> >
> > > * 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.
> >
> > > * 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!

Patch

diff --git a/branch.c b/branch.c
index 5aaf073dce1..8cc7fabe599 100644
--- a/branch.c
+++ b/branch.c
@@ -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)
diff --git a/branch.h b/branch.h
index ef56103c050..c73d98b8766 100644
--- a/branch.h
+++ b/branch.h
@@ -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.
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 21a4335abb0..cae54af997b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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) {
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 5a7caf958c3..9c80d469b6b 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -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 &&