From: Elijah Newren <newren@gmail.com>
To: Tao Klerks via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Tao Klerks <tao@klerks.biz>
Subject: Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved
Date: Tue, 2 May 2023 08:55:39 -0700 [thread overview]
Message-ID: <CABPp-BH8A=CnO3_UWXDegb87VTNEX8s+=CefB90m1_vjBZ_+Fw@mail.gmail.com> (raw)
In-Reply-To: <pull.1527.git.1683008869804.gitgitgadget@gmail.com>
On Mon, May 1, 2023 at 11:57 PM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tao Klerks <tao@klerks.biz>
>
> 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 <tao@klerks.biz>
> ---
> RFC: switch: allow same-commit switch during merge if conflicts resolved
>
> RFC V1: proposing a limited-scope change as per mailing discussion
> https://lore.kernel.org/git/CAPMMpoht4FWnv-WuSM3+Z2R4HhwFY+pahJ6zirFU-BD5r34B7Q@mail.gmail.com/T/#t
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1527%2FTaoK%2Ftao-checkout-during-merge-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1527/TaoK/tao-checkout-during-merge-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1527
>
> branch.c | 7 ++++-
> branch.h | 6 +++++
> builtin/checkout.c | 64 ++++++++++++++++++++++++++++++++++++++++++----
> t/t2060-switch.sh | 18 ++++++++++++-
> 4 files changed, 88 insertions(+), 7 deletions(-)
>
> 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
next prev parent reply other threads:[~2023-05-02 15:55 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 6:27 [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved Tao Klerks via GitGitGadget
2023-05-02 15:55 ` Elijah Newren [this message]
2023-05-02 16:50 ` Junio C Hamano
2023-05-03 0:34 ` Elijah Newren
2023-05-04 5:01 ` Tao Klerks
2023-05-05 5:06 ` Tao Klerks
2023-05-07 2:57 ` Elijah Newren
2023-05-07 2:48 ` Elijah Newren
2023-05-07 22:01 ` Felipe Contreras
2023-05-08 8:30 ` Tao Klerks
2023-05-08 16:13 ` Felipe Contreras
2023-05-08 16:58 ` Tao Klerks
2023-05-08 19:18 ` Junio C Hamano
2023-05-09 1:55 ` Felipe Contreras
2023-05-08 10:44 ` Tao Klerks
2023-05-11 7:06 ` Elijah Newren
2023-05-21 20:08 ` Tao Klerks
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABPp-BH8A=CnO3_UWXDegb87VTNEX8s+=CefB90m1_vjBZ_+Fw@mail.gmail.com' \
--to=newren@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=tao@klerks.biz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).