{"thread":{"id":"59681","subject":"[PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","startedAt":"2023-05-02T06:27:57Z","lastAt":"2023-05-21T20:09:00Z","messageCount":17,"participants":["Tao Klerks via GitGitGadget","Elijah Newren","Junio C Hamano","Tao Klerks","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"476412","messageId":"pull.1527.git.1683008869804.gitgitgadget@gmail.com","threadId":"59681","inReplyTo":null,"subject":"[PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-02T06:27:49Z","receivedAt":"2023-05-02T06:27:57Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nWhen a \"git checkout\" branch-switch operation runs during a merge, the\nin-progress merge metadata is removed. The refusal to maintain the merge\nmatadata makes sense if the new commit is different to the original commit,\nbecause a merge operation against a different commit could have turned out\ndifferently / maintaining the merge relationship would be misleading. It is\nstill a difficult-to-understand behavior for new users, however, as they\nwould expect a switch (to a new or same-commit branch at least) to allow\ncommitting local changes \"faithfully\", as in the case of regular non-merge\nlocal changes.\n\n\"git switch\" introduces a little more safety, by refusing to switch if there\nis a merge in progress - or a number of other operations such as rebase,\ncherry-pick, or \"git am\". This is less of a nasty surprise than the merge\nmetadata/state being silently discarded, but is still not very helpful, when\na user has a complex merge resolved, and wishes to commit it to a new branch\nfor testing.\n\nChange the behavior of \"git switch\" and \"git checkout\" to no longer delete\nmerge metadata, nor prohibit the switch, if a merge is in progress and the\ncommit being switched to is the same commit the HEAD was previously set to.\n\nAlso add a warning when the merge metadata is deleted (in case of a\n\"git checkout\" to another commit) to let the user know the merge state\nwas lost, and that \"git switch\" would prevent this.\n\nAlso add a warning when the merge metadata is preserved (same commit),\nto let the user know the commit message prepared for the merge may still\nrefer to the previous branch.\n\nAdd tests to verify the exception works correctly, and to verify that\n--force always wipes out in-progress merge metadata when it is discarding\nworktree changes.\n\nSigned-off-by: Tao Klerks \n---\n RFC: switch: allow same-commit switch during merge if conflicts resolved\n \n RFC V1: proposing a limited-scope change as per mailing discussion\n https://lore.kernel.org/git/CAPMMpoht4FWnv-WuSM3+Z2R4HhwFY+pahJ6zirFU-BD5r34B7Q@mail.gmail.com/T/#t\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1527%2FTaoK%2Ftao-checkout-during-merge-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1527/TaoK/tao-checkout-during-merge-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1527\n\n branch.c | 7 ++++-\n branch.h | 6 +++++\n builtin/checkout.c | 64 ++++++++++++++++++++++++++++++++++++++++++----\n t/t2060-switch.sh | 18 ++++++++++++-\n 4 files changed, 88 insertions(+), 7 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 5aaf073dce1..8cc7fabe599 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -812,10 +812,15 @@ void remove_merge_branch_state(struct repository *r)\n }\n \n void remove_branch_state(struct repository *r, int verbose)\n+{\n+\tremove_branch_state_except_merge(r, verbose);\n+\tremove_merge_branch_state(r);\n+}\n+\n+void remove_branch_state_except_merge(struct repository *r, int verbose)\n {\n \tsequencer_post_commit_cleanup(r, verbose);\n \tunlink(git_path_squash_msg(r));\n-\tremove_merge_branch_state(r);\n }\n \n void die_if_checked_out(const char *branch, int ignore_current_worktree)\ndiff --git a/branch.h b/branch.h\nindex ef56103c050..c73d98b8766 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -135,6 +135,12 @@ void remove_merge_branch_state(struct repository *r);\n */\n void remove_branch_state(struct repository *r, int verbose);\n \n+/*\n+ * Remove information about the state of working on the current\n+ * branch, *except* merge state.\n+ */\n+void remove_branch_state_except_merge(struct repository *r, int verbose);\n+\n /*\n * Configure local branch \"local\" as downstream to branch \"remote\"\n * from remote \"origin\". Used by git branch --set-upstream.\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 21a4335abb0..cae54af997b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -985,7 +985,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tdelete_reflog(old_branch_info->path);\n \t\t}\n \t}\n-\tremove_branch_state(the_repository, !opts->quiet);\n+\n+\tremove_branch_state_except_merge(the_repository, !opts->quiet);\n+\tif (opts->force || old_branch_info->commit != new_branch_info->commit) {\n+\t\tremove_merge_branch_state(the_repository);\n+\t}\n \tstrbuf_release(&msg);\n \tif (!opts->quiet &&\n \t (new_branch_info->path || (!opts->force_detach && !strcmp(new_branch_info->name, \"HEAD\"))))\n@@ -1098,6 +1102,36 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne\n \trelease_revisions(&revs);\n }\n \n+/*\n+ * Check whether we're in a merge, and if so warn - about the ongoing merge and surprising merge\n+ * message if the merge state will be preserved, and about the destroyed merge state otherwise.\n+ */\n+static void merging_checkout_warning(const char *name, struct commit *old_commit,\n+\t\t\t\t struct commit *new_commit)\n+{\n+\tstruct wt_status_state state;\n+\tmemset(&state, 0, sizeof(state));\n+\twt_status_get_state(the_repository, &state, 0);\n+\n+\tif (!state.merge_in_progress)\n+\t{\n+\t\twt_status_state_free_buffers(&state);\n+\t\treturn;\n+\t}\n+\n+\tif (old_commit == new_commit)\n+\t\twarning(_(\"switching while merge-in-progress (without changing commit).\\n\"\n+\t\t\t \"An auto-generated commit message may still refer to the previous\\n\"\n+\t\t\t \"branch.\\n\"));\n+\telse\n+\t\twarning(_(\"switching to a different commit while while merge-in-progress;\\n\"\n+\t\t\t \"merge metadata was removed. To avoid accidentally losing merge,\\n\"\n+\t\t\t \"metadata in this way, please use \\\"git switch\\\" instead of\\n\"\n+\t\t\t \"\\\"git checkout\\\".\\n\"));\n+\n+\twt_status_state_free_buffers(&state);\n+}\n+\n static int switch_branches(const struct checkout_opts *opts,\n \t\t\t struct branch_info *new_branch_info)\n {\n@@ -1153,6 +1187,9 @@ static int switch_branches(const struct checkout_opts *opts,\n \tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n \t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);\n \n+\tif (!opts->quiet && !opts->force)\n+\t\tmerging_checkout_warning(old_branch_info.name, old_branch_info.commit, new_branch_info->commit);\n+\n \tupdate_refs_for_switch(opts, &old_branch_info, new_branch_info);\n \n \tret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);\n@@ -1445,15 +1482,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)\n \texit(code);\n }\n \n-static void die_if_some_operation_in_progress(void)\n+static void die_if_some_incompatible_operation_in_progress(struct commit *new_commit)\n {\n+\t/*\n+\t * Note: partially coordinated logic in related function\n+\t * merging_checkout_warning(), checking for merge_in_progress\n+\t * and old_commit != new_commit to issue warnings. Issuing those\n+\t * warnings here would be simpler to implement, but would make the\n+\t * language more complex to account for common situations where the\n+\t * switch still won't happen (namely working tree merge failure).\n+\t */\n+\n \tstruct wt_status_state state;\n+\tstruct branch_info old_branch_info = { 0 };\n+\tstruct object_id rev;\n+\tint flag;\n \n \tmemset(&state, 0, sizeof(state));\n \twt_status_get_state(the_repository, &state, 0);\n+\tmemset(&old_branch_info, 0, sizeof(old_branch_info));\n+\told_branch_info.path = resolve_refdup(\"HEAD\", 0, &rev, &flag);\n+\tif (old_branch_info.path)\n+\t\told_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1);\n \n-\tif (state.merge_in_progress)\n-\t\tdie(_(\"cannot switch branch while merging\\n\"\n+\tif (state.merge_in_progress && old_branch_info.commit != new_commit)\n+\t\tdie(_(\"cannot switch to a different commit while merging\\n\"\n \t\t \"Consider \\\"git merge --quit\\\" \"\n \t\t \"or \\\"git worktree add\\\".\"));\n \tif (state.am_in_progress)\n@@ -1476,6 +1529,7 @@ static void die_if_some_operation_in_progress(void)\n \t\twarning(_(\"you are switching branch while bisecting\"));\n \n \twt_status_state_free_buffers(&state);\n+\tbranch_info_release(&old_branch_info);\n }\n \n static int checkout_branch(struct checkout_opts *opts,\n@@ -1536,7 +1590,7 @@ static int checkout_branch(struct checkout_opts *opts,\n \t\tdie_expecting_a_branch(new_branch_info);\n \n \tif (!opts->can_switch_when_in_progress)\n-\t\tdie_if_some_operation_in_progress();\n+\t\tdie_if_some_incompatible_operation_in_progress(new_branch_info->commit);\n \n \tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n \t !opts->ignore_other_worktrees) {\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex 5a7caf958c3..9c80d469b6b 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -111,13 +111,29 @@ test_expect_success 'guess and create branch' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'not switching when something is in progress' '\n+test_expect_success 'not switching to a different commit when something is in progress' '\n \ttest_when_finished rm -f .git/MERGE_HEAD &&\n \t# fake a merge-in-progress\n \tcp .git/HEAD .git/MERGE_HEAD &&\n \ttest_must_fail git switch -d @^\n '\n \n+test_expect_success 'switching to same-commit when merge is in progress succeeds' '\n+\ttest_when_finished rm -f .git/MERGE_HEAD &&\n+\t# fake a merge-in-progress\n+\tcp .git/HEAD .git/MERGE_HEAD &&\n+\tgit switch -d @ &&\n+\t# confirm the merge-in-progress is still there\n+\ttest -e .git/MERGE_HEAD\n+'\n+test_expect_success 'switching with --force removes merge state' '\n+\ttest_when_finished rm -f .git/MERGE_HEAD &&\n+\t# fake a merge-in-progress\n+\tcp .git/HEAD .git/MERGE_HEAD &&\n+\tgit switch --force -d @ &&\n+\t# confirm the merge-in-progress is removed\n+\ttest ! -e .git/MERGE_HEAD\n+'\n test_expect_success 'tracking info copied with autoSetupMerge=inherit' '\n \t# default config does not copy tracking info\n \tgit switch -c foo-no-inherit foo &&\n\nbase-commit: 950264636c68591989456e3ba0a5442f93152c1a\n-- \ngitgitgadget\n"},{"id":"476427","messageId":"CABPp-BH8A=CnO3_UWXDegb87VTNEX8s+=CefB90m1_vjBZ_+Fw@mail.gmail.com","threadId":"59681","inReplyTo":"pull.1527.git.1683008869804.gitgitgadget@gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-02T15:55:39Z","receivedAt":"2023-05-02T15:55:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, May 1, 2023 at 11:57 PM Tao Klerks via GitGitGadget\n wrote:\n>\n> From: Tao Klerks \n>\n> When a \"git checkout\" branch-switch operation runs during a merge, the\n> in-progress merge metadata is removed. The refusal to maintain the merge\n> matadata makes sense if the new commit is different to the original commit,\n\ns/matadata/metadata/\n\n> because a merge operation against a different commit could have turned out\n> differently / maintaining the merge relationship would be misleading. It is\n> still a difficult-to-understand behavior for new users, however, as they\n> would expect a switch (to a new or same-commit branch at least) to allow\n> committing local changes \"faithfully\", as in the case of regular non-merge\n> local changes.\n\n> \"git switch\" introduces a little more safety, by refusing to switch if there\n\ns/little/lot/ or s/a little//\n\nBy the way, it was a problem that git-checkout wasn't updated to have\nthe same safety that git-switch has. We should fix that. (It's on my\ntodo list, along with adding other\nprevent-erroneous-command-while-in-middle-of-other-operation cases.)\n\n> is a merge in progress - or a number of other operations such as rebase,\n> cherry-pick, or \"git am\". This is less of a nasty surprise than the merge\n> metadata/state being silently discarded, but is still not very helpful, when\n> a user has a complex merge resolved, and wishes to commit it to a new branch\n> for testing.\n\nI'm worried this is likely to lead us into confusing UI mismatches,\nand makes it harder to understand the appropriate rules of what can\nand cannot be done. A very simple \"no switching branches in the\nmiddle of operations\" is a very simple rule, and saves users from lots\nof headaches.\n\nGranted, expert users may understand that with the commit being the\nsame, there is no issue. But expert users can use `git update-ref` to\ntweak HEAD, or edit .git/HEAD directly, and accept the consequences.\nWhy do we need to confuse the UI for the sake of expert users who\nalready have an escape hatch?\n\nMore importantly, though...\n\n> Change the behavior of \"git switch\" and \"git checkout\" to no longer delete\n> merge metadata, nor prohibit the switch, if a merge is in progress and the\n> commit being switched to is the same commit the HEAD was previously set to.\n\nEven if there are conflicts? For rebases, cherry-picks, ams, and\nreverts too? (Does allowing this during rebases and whatnot mean that\n--abort becomes really funny? Does it mean that some commits are\napplied to one branch, and all commits are applied to another? What\nabout autostashes? Does it interact weirdly with --update-refs?\netc.)\n\nI think this change is premature unless it discusses all these cases,\nbecause UI backward-compatibility requirements means we can't rip this\nout later if we add it, and any change here is going to lead to\nquestions about either inconsistencies in the UI for other operations\n(why can't I also switch branches if there are conflicts? why can't I\nalso switch branches to a same-commit branch during a rebase?) or\ncrazy problems we've introduced (`git rebase --abort` only aborted\nchanges to one of the branches I modified?? Which of the three\nbranches -- the one I started on, the one I was rebasing, or the one I\nswitched to in the middle -- is my autostash now found on??) by\nopening this can of worms.\n\nMy first gut guess is that switching with conflicts would be just as\nsafe as this is, and any users who likes your change is going to\ncomplain if we don't allow it during conflicts. But I think it'd take\na fair amount of work to figure out if it's safe during\nrebase/cherry-pick/am/revert (is it only okay on the very first patch\nof a series? And only if non-interactive? And only without\n--autostash and --update-refs? etc.), and whether the ending set of\nrules feels horribly inconsistent or feels fine to support.\n\n> Also add a warning when the merge metadata is deleted (in case of a\n> \"git checkout\" to another commit) to let the user know the merge state\n> was lost, and that \"git switch\" would prevent this.\n\nIf we're touching this area, we should employ the right fix rather\nthan a half measure. As I mentioned above, this should be an error\nwith the operation prevented -- just like switch behaves.\n\n> Also add a warning when the merge metadata is preserved (same commit),\n> to let the user know the commit message prepared for the merge may still\n> refer to the previous branch.\n\nSo, it's not entirely safe even when the commit of the target branch\nmatches HEAD? Is that perhaps reason to just leave this for expert\nusers to use the update-refs workaround?\n\n> Add tests to verify the exception works correctly, and to verify that\n> --force always wipes out in-progress merge metadata when it is discarding\n> worktree changes.\n>\n> Signed-off-by: Tao Klerks \n> ---\n> RFC: switch: allow same-commit switch during merge if conflicts resolved\n>\n> RFC V1: proposing a limited-scope change as per mailing discussion\n> https://lore.kernel.org/git/CAPMMpoht4FWnv-WuSM3+Z2R4HhwFY+pahJ6zirFU-BD5r34B7Q@mail.gmail.com/T/#t\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1527%2FTaoK%2Ftao-checkout-during-merge-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1527/TaoK/tao-checkout-during-merge-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1527\n>\n> branch.c | 7 ++++-\n> branch.h | 6 +++++\n> builtin/checkout.c | 64 ++++++++++++++++++++++++++++++++++++++++++----\n> t/t2060-switch.sh | 18 ++++++++++++-\n> 4 files changed, 88 insertions(+), 7 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 5aaf073dce1..8cc7fabe599 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -812,10 +812,15 @@ void remove_merge_branch_state(struct repository *r)\n> }\n>\n> void remove_branch_state(struct repository *r, int verbose)\n> +{\n> + remove_branch_state_except_merge(r, verbose);\n> + remove_merge_branch_state(r);\n> +}\n> +\n> +void remove_branch_state_except_merge(struct repository *r, int verbose)\n> {\n> sequencer_post_commit_cleanup(r, verbose);\n> unlink(git_path_squash_msg(r));\n> - remove_merge_branch_state(r);\n> }\n>\n> void die_if_checked_out(const char *branch, int ignore_current_worktree)\n> diff --git a/branch.h b/branch.h\n> index ef56103c050..c73d98b8766 100644\n> --- a/branch.h\n> +++ b/branch.h\n> @@ -135,6 +135,12 @@ void remove_merge_branch_state(struct repository *r);\n> */\n> void remove_branch_state(struct repository *r, int verbose);\n>\n> +/*\n> + * Remove information about the state of working on the current\n> + * branch, *except* merge state.\n> + */\n> +void remove_branch_state_except_merge(struct repository *r, int verbose);\n> +\n> /*\n> * Configure local branch \"local\" as downstream to branch \"remote\"\n> * from remote \"origin\". Used by git branch --set-upstream.\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 21a4335abb0..cae54af997b 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -985,7 +985,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n> delete_reflog(old_branch_info->path);\n> }\n> }\n> - remove_branch_state(the_repository, !opts->quiet);\n> +\n> + remove_branch_state_except_merge(the_repository, !opts->quiet);\n> + if (opts->force || old_branch_info->commit != new_branch_info->commit) {\n> + remove_merge_branch_state(the_repository);\n> + }\n> strbuf_release(&msg);\n> if (!opts->quiet &&\n> (new_branch_info->path || (!opts->force_detach && !strcmp(new_branch_info->name, \"HEAD\"))))\n> @@ -1098,6 +1102,36 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne\n> release_revisions(&revs);\n> }\n>\n> +/*\n> + * Check whether we're in a merge, and if so warn - about the ongoing merge and surprising merge\n> + * message if the merge state will be preserved, and about the destroyed merge state otherwise.\n> + */\n> +static void merging_checkout_warning(const char *name, struct commit *old_commit,\n> + struct commit *new_commit)\n> +{\n> + struct wt_status_state state;\n> + memset(&state, 0, sizeof(state));\n> + wt_status_get_state(the_repository, &state, 0);\n> +\n> + if (!state.merge_in_progress)\n> + {\n> + wt_status_state_free_buffers(&state);\n> + return;\n> + }\n> +\n> + if (old_commit == new_commit)\n> + warning(_(\"switching while merge-in-progress (without changing commit).\\n\"\n> + \"An auto-generated commit message may still refer to the previous\\n\"\n> + \"branch.\\n\"));\n> + else\n> + warning(_(\"switching to a different commit while while merge-in-progress;\\n\"\n> + \"merge metadata was removed. To avoid accidentally losing merge,\\n\"\n> + \"metadata in this way, please use \\\"git switch\\\" instead of\\n\"\n> + \"\\\"git checkout\\\".\\n\"));\n> +\n> + wt_status_state_free_buffers(&state);\n> +}\n> +\n> static int switch_branches(const struct checkout_opts *opts,\n> struct branch_info *new_branch_info)\n> {\n> @@ -1153,6 +1187,9 @@ static int switch_branches(const struct checkout_opts *opts,\n> if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n> orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);\n>\n> + if (!opts->quiet && !opts->force)\n> + merging_checkout_warning(old_branch_info.name, old_branch_info.commit, new_branch_info->commit);\n> +\n> update_refs_for_switch(opts, &old_branch_info, new_branch_info);\n>\n> ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);\n> @@ -1445,15 +1482,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)\n> exit(code);\n> }\n>\n> -static void die_if_some_operation_in_progress(void)\n> +static void die_if_some_incompatible_operation_in_progress(struct commit *new_commit)\n> {\n> + /*\n> + * Note: partially coordinated logic in related function\n> + * merging_checkout_warning(), checking for merge_in_progress\n> + * and old_commit != new_commit to issue warnings. Issuing those\n> + * warnings here would be simpler to implement, but would make the\n> + * language more complex to account for common situations where the\n> + * switch still won't happen (namely working tree merge failure).\n> + */\n> +\n> struct wt_status_state state;\n> + struct branch_info old_branch_info = { 0 };\n> + struct object_id rev;\n> + int flag;\n>\n> memset(&state, 0, sizeof(state));\n> wt_status_get_state(the_repository, &state, 0);\n> + memset(&old_branch_info, 0, sizeof(old_branch_info));\n> + old_branch_info.path = resolve_refdup(\"HEAD\", 0, &rev, &flag);\n> + if (old_branch_info.path)\n> + old_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1);\n>\n> - if (state.merge_in_progress)\n> - die(_(\"cannot switch branch while merging\\n\"\n> + if (state.merge_in_progress && old_branch_info.commit != new_commit)\n> + die(_(\"cannot switch to a different commit while merging\\n\"\n> \"Consider \\\"git merge --quit\\\" \"\n> \"or \\\"git worktree add\\\".\"));\n> if (state.am_in_progress)\n> @@ -1476,6 +1529,7 @@ static void die_if_some_operation_in_progress(void)\n> warning(_(\"you are switching branch while bisecting\"));\n>\n> wt_status_state_free_buffers(&state);\n> + branch_info_release(&old_branch_info);\n> }\n>\n> static int checkout_branch(struct checkout_opts *opts,\n> @@ -1536,7 +1590,7 @@ static int checkout_branch(struct checkout_opts *opts,\n> die_expecting_a_branch(new_branch_info);\n>\n> if (!opts->can_switch_when_in_progress)\n> - die_if_some_operation_in_progress();\n> + die_if_some_incompatible_operation_in_progress(new_branch_info->commit);\n>\n> if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> !opts->ignore_other_worktrees) {\n> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\n> index 5a7caf958c3..9c80d469b6b 100755\n> --- a/t/t2060-switch.sh\n> +++ b/t/t2060-switch.sh\n> @@ -111,13 +111,29 @@ test_expect_success 'guess and create branch' '\n> test_cmp expected actual\n> '\n>\n> -test_expect_success 'not switching when something is in progress' '\n> +test_expect_success 'not switching to a different commit when something is in progress' '\n> test_when_finished rm -f .git/MERGE_HEAD &&\n> # fake a merge-in-progress\n> cp .git/HEAD .git/MERGE_HEAD &&\n> test_must_fail git switch -d @^\n> '\n>\n> +test_expect_success 'switching to same-commit when merge is in progress succeeds' '\n> + test_when_finished rm -f .git/MERGE_HEAD &&\n> + # fake a merge-in-progress\n> + cp .git/HEAD .git/MERGE_HEAD &&\n> + git switch -d @ &&\n> + # confirm the merge-in-progress is still there\n> + test -e .git/MERGE_HEAD\n> +'\n> +test_expect_success 'switching with --force removes merge state' '\n> + test_when_finished rm -f .git/MERGE_HEAD &&\n> + # fake a merge-in-progress\n> + cp .git/HEAD .git/MERGE_HEAD &&\n> + git switch --force -d @ &&\n> + # confirm the merge-in-progress is removed\n> + test ! -e .git/MERGE_HEAD\n> +'\n> test_expect_success 'tracking info copied with autoSetupMerge=inherit' '\n> # default config does not copy tracking info\n> git switch -c foo-no-inherit foo &&\n>\n> base-commit: 950264636c68591989456e3ba0a5442f93152c1a\n> --\n> gitgitgadget\n"},{"id":"476437","messageId":"xmqq1qjy1xv2.fsf@gitster.g","threadId":"59681","inReplyTo":"CABPp-BH8A=CnO3_UWXDegb87VTNEX8s+=CefB90m1_vjBZ_+Fw@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-02T16:50:25Z","receivedAt":"2023-05-02T16:50:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> By the way, it was a problem that git-checkout wasn't updated to have\n> the same safety that git-switch has. We should fix that. (It's on my\n> todo list, along with adding other\n> prevent-erroneous-command-while-in-middle-of-other-operation cases.)\n\nYes.\n\n> I'm worried this is likely to lead us into confusing UI mismatches,\n> and makes it harder to understand the appropriate rules of what can\n> and cannot be done. A very simple \"no switching branches in the\n> middle of operations\" is a very simple rule, and saves users from lots\n> of headaches.\n>\n> Granted, expert users may understand that with the commit being the\n> same, there is no issue. But expert users can use `git update-ref` to\n> tweak HEAD, or edit .git/HEAD directly, and accept the consequences.\n> Why do we need to confuse the UI for the sake of expert users who\n> already have an escape hatch?\n>\n> More importantly, though...\n>\n>> Change the behavior of \"git switch\" and \"git checkout\" to no longer delete\n>> merge metadata, nor prohibit the switch, if a merge is in progress and the\n>> commit being switched to is the same commit the HEAD was previously set to.\n>\n> Even if there are conflicts? For rebases, cherry-picks, ams, and\n> reverts too? (Does allowing this during rebases and whatnot mean that\n> --abort becomes really funny? Does it mean that some commits are\n> applied to one branch, and all commits are applied to another? What\n> about autostashes? Does it interact weirdly with --update-refs?\n> etc.)\n>\n> I think this change is premature unless it discusses all these cases,\n\nIt is pretty much what I wanted to say about why we haven't done\nthis in ,\nso it makes two of us ;-). I didn't look at Tao's RFC patch but if\nthe way it determines \"we are in a middle of conflicted merge and\nwe'll allow switching to the same commit only in this case\" were\n\"the index has an unmerged entry\", then it is an overly broad test\nand the consequences of allowing the switch for these other merge-y\noperations that are ongoing must be evaluated.\n\nThanks.\n"},{"id":"476481","messageId":"CABPp-BEd_53EfEfFfWf8zEt0K7Mp4iMzN=q6smK4_08xfj6Tiw@mail.gmail.com","threadId":"59681","inReplyTo":"xmqq1qjy1xv2.fsf@gitster.g","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-03T00:34:00Z","receivedAt":"2023-05-03T00:35:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 2, 2023 at 9:50 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > By the way, it was a problem that git-checkout wasn't updated to have\n> > the same safety that git-switch has. We should fix that. (It's on my\n> > todo list, along with adding other\n> > prevent-erroneous-command-while-in-middle-of-other-operation cases.)\n>\n> Yes.\n>\n> > I'm worried this is likely to lead us into confusing UI mismatches,\n> > and makes it harder to understand the appropriate rules of what can\n> > and cannot be done. A very simple \"no switching branches in the\n> > middle of operations\" is a very simple rule, and saves users from lots\n> > of headaches.\n> >\n> > Granted, expert users may understand that with the commit being the\n> > same, there is no issue. But expert users can use `git update-ref` to\n> > tweak HEAD, or edit .git/HEAD directly, and accept the consequences.\n> > Why do we need to confuse the UI for the sake of expert users who\n> > already have an escape hatch?\n> >\n> > More importantly, though...\n> >\n> >> Change the behavior of \"git switch\" and \"git checkout\" to no longer delete\n> >> merge metadata, nor prohibit the switch, if a merge is in progress and the\n> >> commit being switched to is the same commit the HEAD was previously set to.\n> >\n> > Even if there are conflicts? For rebases, cherry-picks, ams, and\n> > reverts too? (Does allowing this during rebases and whatnot mean that\n> > --abort becomes really funny? Does it mean that some commits are\n> > applied to one branch, and all commits are applied to another? What\n> > about autostashes? Does it interact weirdly with --update-refs?\n> > etc.)\n> >\n> > I think this change is premature unless it discusses all these cases,\n>\n> It is pretty much what I wanted to say about why we haven't done\n> this in ,\n> so it makes two of us ;-). I didn't look at Tao's RFC patch but if\n> the way it determines \"we are in a middle of conflicted merge and\n> we'll allow switching to the same commit only in this case\" were\n> \"the index has an unmerged entry\", then it is an overly broad test\n> and the consequences of allowing the switch for these other merge-y\n> operations that are ongoing must be evaluated.\n\nHe does tie it specifically to \"is-this-a-merge-operation\" (and\nactually doesn't check for conflicts at all since there are existing\nchecks he leaves untouched). That certainly prevents some problems,\nbut doesn't address my concerns.\n\nI think the usecase Tao presents has multiple simple workarounds, and\nI'm worried that the particular proposal might paint us into a corner.\n\nPersonally, I think that before we consider a\nmerge-specific-if-no-conflicts exception, someone should evaluate all\nthe cases where exceptions could or should be allowed, get a\ndocumented story about them, and then if a consistent-ish UI is\npossible then propose patches to start taking us down this path.\n"},{"id":"476589","messageId":"CAPMMpogiTVksUKgZ==n4d3xm4ZJqxm7ki2dOF8j8S5BaJvu1Ew@mail.gmail.com","threadId":"59681","inReplyTo":"CABPp-BH8A=CnO3_UWXDegb87VTNEX8s+=CefB90m1_vjBZ_+Fw@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-04T05:01:02Z","receivedAt":"2023-05-04T05:01:20Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, May 2, 2023 at 5:55 PM Elijah Newren wrote:\n>\n> On Mon, May 1, 2023 at 11:57 PM Tao Klerks via GitGitGadget\n> wrote:\n> >\n> > From: Tao Klerks \n> >\n> > When a \"git checkout\" branch-switch operation runs during a merge, the\n> > in-progress merge metadata is removed. The refusal to maintain the merge\n> > matadata makes sense if the new commit is different to the original commit,\n>\n> s/matadata/metadata/\n\nThx!\n\n>\n> > because a merge operation against a different commit could have turned out\n> > differently / maintaining the merge relationship would be misleading. It is\n> > still a difficult-to-understand behavior for new users, however, as they\n> > would expect a switch (to a new or same-commit branch at least) to allow\n> > committing local changes \"faithfully\", as in the case of regular non-merge\n> > local changes.\n>\n> > \"git switch\" introduces a little more safety, by refusing to switch if there\n>\n> s/little/lot/ or s/a little//\n\nYep, this paragraph is straight-up wrong - safety is an entirely\nseparate concern from usability & meeting reasonable expectations.\n\n>\n> By the way, it was a problem that git-checkout wasn't updated to have\n> the same safety that git-switch has. We should fix that. (It's on my\n> todo list, along with adding other\n> prevent-erroneous-command-while-in-middle-of-other-operation cases.)\n>\n\nThis surprises me because the difference in \"safety\" is very\nexplicitly expressed and implemented in an option\n\"can_switch_when_in_progress\", which is driven purely by \"checkout vs\nswitch\", and determines whether the validations in\ndie_if_some_operation_in_progress() apply - dying on merge, am, rebase\ncherry-pick, or revert.\n\nIf we are comfortable changing the behavior of branch checkout to be\nsafe-and-limiting like switch, then that should be almost as simple as\nremoving that condition. The wrinkle is that I believe \"--force\"\nshould still be allowed at least in the cases where it is safe\n(whereas currently switch does *not* allow even --force if a merge is\nin progress, and this proposed patch accidentally \"fixed\" that for the\nsame-commit case only).\n\n> > is a merge in progress - or a number of other operations such as rebase,\n> > cherry-pick, or \"git am\". This is less of a nasty surprise than the merge\n> > metadata/state being silently discarded, but is still not very helpful, when\n> > a user has a complex merge resolved, and wishes to commit it to a new branch\n> > for testing.\n>\n> I'm worried this is likely to lead us into confusing UI mismatches,\n> and makes it harder to understand the appropriate rules of what can\n> and cannot be done. A very simple \"no switching branches in the\n> middle of operations\" is a very simple rule, and saves users from lots\n> of headaches.\n\nI'm not convinced a simple rule that prevents a natural majority-case\nworkflow is better than a complex rule that allows one majority-case\nworkflow while prohibiting others.\n\nMy claim, which I understand is quite tenuous, is that the situation\nwhere you find yourself legitimately thinking \"wow, I just did a lot\nof work, but I'm not sure it's *right*. I should test it on a new\nbranch *before* committing to this branch\" is significantly more\nlikely to arise with \"git merge\", where what you are merging in is\noften others' work, in a single step, than with those other commands.\n\n(ignoring rebase, which may be as common, but is a completely\ndifferent workflow, where \"let me save it on a new branch before I\nshare with others\" doesn't make as much sense; in most workflows you\ndon't rebase shared branches)\n\nIt makes sense to say that prohibiting something that will hurt you\nsaves you a headache, but I think we agree that changing to a\nsame-commit branch during a resolved-index merge as enabled here *does\nnot* hurt you. And the only reasonable alternative (committing the the\noriginal branch, *then* creating a new testing branch, and resetting\nthe original branch) is very non-obvious.\n\n>\n> Granted, expert users may understand that with the commit being the\n> same, there is no issue. But expert users can use `git update-ref` to\n> tweak HEAD, or edit .git/HEAD directly, and accept the consequences.\n> Why do we need to confuse the UI for the sake of expert users who\n> already have an escape hatch?\n\nExpert users are not the targeted audience of these changes. Users\nused to the natural git pattern of \"I have some local uncommitted\nchanges, I'm not quite sure about them, so let me create a new branch\nand commit them there, so I can validate them properly, and then I'll\nbring them to the original branch\" are.\n\n>\n> More importantly, though...\n>\n> > Change the behavior of \"git switch\" and \"git checkout\" to no longer delete\n> > merge metadata, nor prohibit the switch, if a merge is in progress and the\n> > commit being switched to is the same commit the HEAD was previously set to.\n>\n> Even if there are conflicts? For rebases, cherry-picks, ams, and\n> reverts too? (Does allowing this during rebases and whatnot mean that\n> --abort becomes really funny? Does it mean that some commits are\n> applied to one branch, and all commits are applied to another? What\n> about autostashes? Does it interact weirdly with --update-refs?\n> etc.)\n\nI believe this question was resolved later in the thread. The proposal\nis to allow the simplest case of merge only, for resolved\n(unconflicted) indexes only. If the change were to make sense I could\nupdate this message to be clearer that none of those other operations\nor situations are impacted by this change.\n\n>\n> I think this change is premature unless it discusses all these cases,\n> because UI backward-compatibility requirements means we can't rip this\n> out later if we add it, and any change here is going to lead to\n> questions about either inconsistencies in the UI for other operations\n> (why can't I also switch branches if there are conflicts? why can't I\n> also switch branches to a same-commit branch during a rebase?) or\n> crazy problems we've introduced (`git rebase --abort` only aborted\n> changes to one of the branches I modified?? Which of the three\n> branches -- the one I started on, the one I was rebasing, or the one I\n> switched to in the middle -- is my autostash now found on??) by\n> opening this can of worms.\n>\n\nMy argument is that making things a little better is better than not\nmaking them better at all (although yes, of course, ideally we would\nalso cover those other cases long-term, if they make logical sense). I\nunderstand that's easy for me to say of course, as the dilletante who\npops in to make a tiny change and leaves the team on the hook to\nresolve the edge-cases / address broader consistency in the long term.\n\n> My first gut guess is that switching with conflicts would be just as\n> safe as this is, and any users who likes your change is going to\n> complain if we don't allow it during conflicts.\n\nIn principle I believe so too, I just haven't checked whether the\ntree-merge process attempts to do anything for a same-commit switch,\nand if it does, whether the presence of conflict data \"bothers\" it in\nany way / causes it to do the wrong thing, eg remove it.\n\nIf verifying this and opening up the \"pending conflicts\" case meets\nthe consistency itch, I'm happy to explore this area and (try to)\nexpand the scope of the fix/exemption.\n\n> But I think it'd take\n> a fair amount of work to figure out if it's safe during\n> rebase/cherry-pick/am/revert (is it only okay on the very first patch\n> of a series? And only if non-interactive? And only without\n> --autostash and --update-refs? etc.), and whether the ending set of\n> rules feels horribly inconsistent or feels fine to support.\n\nI agree this gets complicated - I haven't thought or explored through\nmost of these, but I have confirmed that switching branch in the\nmiddle of a *rebase* is very confusing: your rebase continues on the\nnew HEAD, as you continue to commit, your rebased commits get\ncommitted to the branch you switched to, but at the end when you\n*complete* the rebase, the original ref you were rebasing still ends\nup being pointed to the new HEAD - so you end up with *both* the\nbranch you were rebasing, and the branch you switched to along the\nway, pointing to the same head commit.\n\nI understand how that works in terms of git's internal logic, but as a\nuser of rebase, if I tried to switch (to a new branch) in the middle,\nI would be intending to say \"I got scared of the changes I'm making\nhere, I want the that is ref pointed to the new commit graph at the\nend of the process to be this new ref, instead of the ref I originally\nstarted on\".\n\nSupporting that usecase, for rebase, sounds to me like it should be\ndone by something completely different to \"git switch\". The most\nhelpful behavior I can think of here would be that a \"git switch\"\nattempt would say \"cannot switch branch in the middle of a rebase. to\ncontinue your rebase and create a new branch, use 'git rebase\n--make-new-branch NEWBRANCHNAME\" instead of 'git switch'\"\n\n>\n> > Also add a warning when the merge metadata is deleted (in case of a\n> > \"git checkout\" to another commit) to let the user know the merge state\n> > was lost, and that \"git switch\" would prevent this.\n>\n> If we're touching this area, we should employ the right fix rather\n> than a half measure. As I mentioned above, this should be an error\n> with the operation prevented -- just like switch behaves.\n>\n\nMy understanding, given the code organization, was that we wanted to\npreserve current (funky) behavior for backwards-compatibility\npurposes. If we're comfortable changing behavior here, I am happy to\nchange the patch (while keeping/allowing the --force exemption, which\n*should* still destroy the merge state).\n\n> > Also add a warning when the merge metadata is preserved (same commit),\n> > to let the user know the commit message prepared for the merge may still\n> > refer to the previous branch.\n>\n> So, it's not entirely safe even when the commit of the target branch\n> matches HEAD? Is that perhaps reason to just leave this for expert\n> users to use the update-refs workaround?\n>\n\nIt is *safe*, it's just that one aspect of the outcome is *potentially\nconfusing*. You really did do the merge on the original branch. The\nmerge message is the same as it would be if you committed, created a\nnew branch, and reset the original branch.\n\n(and just to note - the reasonable workaround is to commit the merge\non the current \"wrong\" branch, create the other branch, and then reset\nthe original branch, as Chris Torek shows on StackOverflow; not to\nteach people all about update-refs)\n\n\nThanks so much for taking the time to go through all this!\n\nPlease let me know whether you would be comfortable with a patch that:\n* Fixed checkout to be more restrictive (except still allowing --force\nat least on a merging state)\n* More explicitly noted that we are relaxing things for merge only,\nnone of the other in-progress states that currently prevent switch\n* Also worked with outstanding conflicts in the index (verifying that\nthis is safe)\n\nThanks,\nTao\n"},{"id":"476614","messageId":"CAPMMpoi7+rdQzQPyVB8T9Pb+f332c68QvWLkwBdJZw=BcP0jbQ@mail.gmail.com","threadId":"59681","inReplyTo":"CAPMMpogiTVksUKgZ==n4d3xm4ZJqxm7ki2dOF8j8S5BaJvu1Ew@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-05T05:06:15Z","receivedAt":"2023-05-05T05:06:32Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Thu, May 4, 2023 at 7:01 AM Tao Klerks wrote:\n>\n> Please let me know whether you would be comfortable with a patch that:\n> * Fixed checkout to be more restrictive (except still allowing --force\n> at least on a merging state)\n> [...]\n\nHaving reviewed the commit by Nguyễn Thái Ngọc Duy that introduced the\n\"can_switch_when_in_progress\" boolean in 2019 (as part of the broader\nintroduction of \"git switch\" and \"git restore\"), it looks like I\nshould change my proposal here. I thought it made sense to continue to\nsupport --force because we can, but I now think this is wrong,\nbecause:\n1. the fact that --force does not work in git switch is *intentional*\n2. even though making it work for \"merging\" states would be trivial,\nmaking it work during rebase would not be\n3. the blocking of --force is not a significant inconvenience, as the\nerror message clearly tells you what to do (--quit), to continue on\nyour way\n\nSo I will set about trying to understand how to make this one-line\nchange work. I already see that some tests rely on \"git checkout -f\"\nbulldozing through an ongoing merge, so those tests will need to be\nadjusted at least.\n\nAre there any recommendations or processes around breaking changes for\nthe git project anywhere? The specific behaviors that we would be\nchanging here appear to be undocumented (I've looked through\nhttps://git-scm.com/docs/git-checkout at least and find no mention or\nexpectation that switching during a merge, or rebase, etc is\nsupported; nor do I see any explicit mention in\nhttps://git-scm.com/docs/git-switch that it is UNsupported)\n\n\nASIDE: I realized today that the warnings in\ndie_if_some_operation_in_progress() suggest \"--quit\" (potentially\nleaving a conflicted index) and do not mention \"--abort\". Is there any\nobjection to beefing up these messages a bit to offer both options?\n"},{"id":"476688","messageId":"CABPp-BGmPKyNcDa-wUh-oisTvvux+X=6BvGxSNQC2O7uodpFrA@mail.gmail.com","threadId":"59681","inReplyTo":"CAPMMpogiTVksUKgZ==n4d3xm4ZJqxm7ki2dOF8j8S5BaJvu1Ew@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-07T02:48:05Z","receivedAt":"2023-05-07T02:48:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Tao,\n\nOn Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n>\n[...]\n> > By the way, it was a problem that git-checkout wasn't updated to have\n> > the same safety that git-switch has. We should fix that. (It's on my\n> > todo list, along with adding other\n> > prevent-erroneous-command-while-in-middle-of-other-operation cases.)\n> >\n>\n> This surprises me because the difference in \"safety\" is very\n> explicitly expressed and implemented in an option\n> \"can_switch_when_in_progress\", which is driven purely by \"checkout vs\n> switch\", and determines whether the validations in\n> die_if_some_operation_in_progress() apply - dying on merge, am, rebase\n> cherry-pick, or revert.\n\nThe implementation was done that way, not because _anyone_ thought\nthat was the right design, but because Duy wanted to introduce a new\ncommand without touching the existing command, so that he only had to\ndeal with review comments about the new command. Two people commented\non it at the time, one of them me, saying that we should just change\ncheckout and fix it. But it was decided to leave it for later...\n\n> If we are comfortable changing the behavior of branch checkout to be\n> safe-and-limiting like switch, then that should be almost as simple as\n> removing that condition.\n\nI've never heard a dissenting vote against this, and I've brought it\nup a few times. Junio elsewhere in this thread agrees we should just\nchange it.\n\nBesides, this is typically the way backward incompatibilities are\nhandled in Git: First make something a warning, then make it an error,\nthen wait a while, then change the thing (e.g. git push defaults).\nFor simpler cases, you can jump straight to an error, especially when\nnot giving an error can hurt users and is unlikely to be what users\nmeant anyway. This is a case where it can really hurt.\n\n> The wrinkle is that I believe \"--force\"\n> should still be allowed at least in the cases where it is safe\n> (whereas currently switch does *not* allow even --force if a merge is\n> in progress, and this proposed patch accidentally \"fixed\" that for the\n> same-commit case only).\n\nI'd be okay with an override.\n\n[...]\n> >\n> > More importantly, though...\n> >\n> > > Change the behavior of \"git switch\" and \"git checkout\" to no longer delete\n> > > merge metadata, nor prohibit the switch, if a merge is in progress and the\n> > > commit being switched to is the same commit the HEAD was previously set to.\n> >\n> > Even if there are conflicts? For rebases, cherry-picks, ams, and\n> > reverts too? (Does allowing this during rebases and whatnot mean that\n> > --abort becomes really funny? Does it mean that some commits are\n> > applied to one branch, and all commits are applied to another? What\n> > about autostashes? Does it interact weirdly with --update-refs?\n> > etc.)\n>\n> I believe this question was resolved later in the thread. The proposal\n> is to allow the simplest case of merge only, for resolved\n> (unconflicted) indexes only. If the change were to make sense I could\n> update this message to be clearer that none of those other operations\n> or situations are impacted by this change.\n\nAs I mentioned to Junio, I understood fully that your implementation\nlimited the changes to this one case. That did not resolve my\nconcerns, it merely obviated some other bigger ones that I didn't\nraise.\n\nHowever, making it only available via a --force override (and then\nperhaps also limiting it to just some operations), would resolve my\nconcerns.\n\n> > My first gut guess is that switching with conflicts would be just as\n> > safe as this is, and any users who likes your change is going to\n> > complain if we don't allow it during conflicts.\n>\n> In principle I believe so too, I just haven't checked whether the\n> tree-merge process attempts to do anything for a same-commit switch,\n> and if it does, whether the presence of conflict data \"bothers\" it in\n> any way / causes it to do the wrong thing, eg remove it.\n>\n> If verifying this and opening up the \"pending conflicts\" case meets\n> the consistency itch, I'm happy to explore this area and (try to)\n> expand the scope of the fix/exemption.\n\nIf this behavior is behind a `--force` flag rather than the default\nbehavior, then I think there's much more leniency for a partial\nsolution.\n\nThat said, I do still think it'd be nice to handle this case for\nconsistency, so if you're willing to take a look, that'd be great. If\nyou are interested, here's a pointer: Stolee's commit 313677627a8\n(\"checkout: add simple check for 'git checkout -b'\", 2019-08-29) might\nbe of interest here. Essentially, when switching to a same-commit\nbranch, you can short-circuit most of the work and basically just\nupdate HEAD. (In his case, he was creating _and_ switching to a\nbranch, and he was essentially just trying to short-circuit the\nreading and writing of the index since he knew there would be no\nchanges, but the same basic logic almost certainly applies to this\nbroader case -- no index changes are needed, so the existence of\nconflicts shouldn't matter.)\n\nIf you don't want to handle that case, though, you should probably\nthink about what kind of message to give the user if they try to\n`--force` the checkout and they have conflicts. They'd probably\ndeserve a longer explanation due to the inconsistency.\n\n> > But I think it'd take\n> > a fair amount of work to figure out if it's safe during\n> > rebase/cherry-pick/am/revert (is it only okay on the very first patch\n> > of a series? And only if non-interactive? And only without\n> > --autostash and --update-refs? etc.), and whether the ending set of\n> > rules feels horribly inconsistent or feels fine to support.\n>\n> I agree this gets complicated - I haven't thought or explored through\n> most of these, but I have confirmed that switching branch in the\n> middle of a *rebase* is very confusing: your rebase continues on the\n> new HEAD, as you continue to commit, your rebased commits get\n> committed to the branch you switched to, but at the end when you\n> *complete* the rebase, the original ref you were rebasing still ends\n> up being pointed to the new HEAD - so you end up with *both* the\n> branch you were rebasing, and the branch you switched to along the\n> way, pointing to the same head commit.\n>\n> I understand how that works in terms of git's internal logic, but as a\n> user of rebase, if I tried to switch (to a new branch) in the middle,\n> I would be intending to say \"I got scared of the changes I'm making\n> here, I want the that is ref pointed to the new commit graph at the\n> end of the process to be this new ref, instead of the ref I originally\n> started on\".\n>\n> Supporting that usecase, for rebase, sounds to me like it should be\n> done by something completely different to \"git switch\". The most\n> helpful behavior I can think of here would be that a \"git switch\"\n> attempt would say \"cannot switch branch in the middle of a rebase. to\n> continue your rebase and create a new branch, use 'git rebase\n> --make-new-branch NEWBRANCHNAME\" instead of 'git switch'\"\n\nThat all sounds reasonable.\n\nBut you know someone is going to try it anyway during a\nrebase/cherry-pick/revert. If we start letting `--force` override\nduring a merge, we should do something to address that inconsistency\nfor users. It doesn't need to be something big; we could likely\naddress it by just specifically checking for the `--force` case during\na rebase/cherry-pick/revert and providing an even more detailed error\nmessage in that case that spells out why the operation cannot be\n`--force`d.\n\n> > > Also add a warning when the merge metadata is deleted (in case of a\n> > > \"git checkout\" to another commit) to let the user know the merge state\n> > > was lost, and that \"git switch\" would prevent this.\n> >\n> > If we're touching this area, we should employ the right fix rather\n> > than a half measure. As I mentioned above, this should be an error\n> > with the operation prevented -- just like switch behaves.\n> >\n>\n> My understanding, given the code organization, was that we wanted to\n> preserve current (funky) behavior for backwards-compatibility\n> purposes.\n\nI totally understand how you'd reach that conclusion. I would\nprobably come to the same one reading the code for the first time.\nBut, as it turns out, that's not how things happened.\n\n> If we're comfortable changing behavior here, I am happy to\n> change the patch (while keeping/allowing the --force exemption, which\n> *should* still destroy the merge state).\n\nYaay!\n\n> > > Also add a warning when the merge metadata is preserved (same commit),\n> > > to let the user know the commit message prepared for the merge may still\n> > > refer to the previous branch.\n> >\n> > So, it's not entirely safe even when the commit of the target branch\n> > matches HEAD? Is that perhaps reason to just leave this for expert\n> > users to use the update-refs workaround?\n> >\n>\n> It is *safe*, it's just that one aspect of the outcome is *potentially\n> confusing*. You really did do the merge on the original branch. The\n> merge message is the same as it would be if you committed, created a\n> new branch, and reset the original branch.\n>\n> (and just to note - the reasonable workaround is to commit the merge\n> on the current \"wrong\" branch, create the other branch, and then reset\n> the original branch, as Chris Torek shows on StackOverflow; not to\n> teach people all about update-refs)\n>\n>\n> Thanks so much for taking the time to go through all this!\n>\n> Please let me know whether you would be comfortable with a patch that:\n> * Fixed checkout to be more restrictive\n\nAbsolutely.\n\n> (except still allowing --force at least on a merging state)\n\nThat's fine too.\n\n> * More explicitly noted that we are relaxing things for merge only,\n> none of the other in-progress states that currently prevent switch\n\nThat wouldn't resolve any of my concerns; it was totally clear to me\nthe first time.\n\n> * Also worked with outstanding conflicts in the index (verifying that\n> this is safe)\n\nIn combination with `--force`, I think that would be very nice.\n"},{"id":"476689","messageId":"CABPp-BGtRF68YJaN+nQ6bCqeh4R-dC9a6mBZvAFkX9YbkFR3sQ@mail.gmail.com","threadId":"59681","inReplyTo":"CAPMMpoi7+rdQzQPyVB8T9Pb+f332c68QvWLkwBdJZw=BcP0jbQ@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-07T02:57:25Z","receivedAt":"2023-05-07T02:57:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, May 4, 2023 at 10:06 PM Tao Klerks wrote:\n>\n[...]\n> ASIDE: I realized today that the warnings in\n> die_if_some_operation_in_progress() suggest \"--quit\" (potentially\n> leaving a conflicted index) and do not mention \"--abort\". Is there any\n> objection to beefing up these messages a bit to offer both options?\n\nHonestly, I'd prefer to just change them to --abort.\n\n--quit is for very unusual expert situations (I did the operation,\nforgot I was in the middle, did all kinds of funny resets and tweaks\nand who-knows-what, and then later discovered there was an in-progress\noperation I had forgotten, but I decided I liked my totally munged\nstate better and want to keep it while somehow marking the operation\nas over.[1]) I think recommending it to users is a bit of a\ndisservice. If someone feels strongly about keeping it, I'd argue for\nhaving both --abort and --quit, with --abort more prominent.\n\nBut my first vote would be for changing these to mention --abort. And\nadding some scary warnings to the places where --quit is documented,\nto recommend users consider --abort instead.\n\n\n[1] That might sound like an exaggeration, but I think that's exactly\nhow it was advertised originally: 9512177b682 (\"rebase: add --quit to\ncleanup rebase, leave everything else untouched\", 2016-11-12)\n"},{"id":"476731","messageId":"64581fc358ede_4e6129442@chronos.notmuch","threadId":"59681","inReplyTo":"CABPp-BGmPKyNcDa-wUh-oisTvvux+X=6BvGxSNQC2O7uodpFrA@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-07T22:01:39Z","receivedAt":"2023-05-07T22:02:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n\n> > If we are comfortable changing the behavior of branch checkout to be\n> > safe-and-limiting like switch, then that should be almost as simple as\n> > removing that condition.\n> \n> I've never heard a dissenting vote against this\n\nHere is my dissenting vote: I'm against this change.\n\nIf I want to use a high-level command meant for novices, I use `git switch`. If\ninstead I simply want to switch to a different commit and I want git to shut up\nabout it, then I use `git checkout`.\n\nYou want to strip away the options for experts, in search for what?\n\nIf there was a way of doing:\n\n git -c core.iknowwhatimdoing=true checkout $whatever\n\nThen I wouldn't oppose such change.\n\nBut this is not the proposal. The proposal is to break backwards compatibility\nfor expert users with no way to retain the existing behavior.\n\nGenerally, breaking backwards compatibility for no reason is frowned upon.\n\n-- \nFelipe Contreras"},{"id":"476739","messageId":"CAPMMpojTjFn7JCo8QsDcOJf6NoJYASbV1bL_JxDhUr7DS12DJg@mail.gmail.com","threadId":"59681","inReplyTo":"64581fc358ede_4e6129442@chronos.notmuch","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-08T08:30:51Z","receivedAt":"2023-05-08T08:31:11Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, May 8, 2023 at 12:01 AM Felipe Contreras\n wrote:\n>\n> Elijah Newren wrote:\n> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n>\n> > > If we are comfortable changing the behavior of branch checkout to be\n> > > safe-and-limiting like switch, then that should be almost as simple as\n> > > removing that condition.\n> >\n> > I've never heard a dissenting vote against this\n>\n> Here is my dissenting vote: I'm against this change.\n>\n> If I want to use a high-level command meant for novices, I use `git switch`. If\n> instead I simply want to switch to a different commit and I want git to shut up\n> about it, then I use `git checkout`.\n\nThank you for your perspective on the relationship between these commands.\n\nI don't fully share this perspective, in two ways:\n- In my experience most novices don't see or know about \"git switch\"\nat all - the vast majority of the internet is still stuck on \"git\ncheckout\", as are existing users. Google search result counts are of\ncourse a poor metric of anything, but compare 100k for \"git switch\" to\n2.4M for \"git checkout\".\n- As far as I can tell, \"git switch\" and \"git restore\" have exactly\nthe same power and expressiveness (except specifically the lack of\n\"git switch --force\" support for bulldozing ongoing merges) - they are\njust as much \"expert\" tools as \"git checkout\"; the main way they\ndiffer is that they are clearer about what they're doing / what\nthey're for. I'd love to see \"git checkout\" deprecated one day,\nalthough I'm not sure I'll live to see it happen :)\n\n>\n> You want to strip away the options for experts, in search for what?\n\nWhat *I* want is a generally safe system, in which you don't have to\nbe an expert to avoid causing yourself problems, especially losing\nwork.\n\nThe specific example that motivated my wanting to change \"git\ncheckout\" here was the case of a normal (non-expert, non-novice) user\nwho is used to doing \"git checkout -b\nactually-those-changes-I-made-shouldnt-go-on-the-branch-I-was-working-on-yet\".\nIn their day-to-day work, that action will always have achieved\nexactly what they wanted. The day they make exactly the same\ninvocation just before they commit a merge, it will do something\ncompletely different and confusing - it will destroy a merge state,\nand result in the commit, shortly after, being a regular non-merge\ncommot. Leveraging that committed tree in a merge commit *is* indeed\nan expert action, and most novice and maybe intermediate users will\ninstead find themselves cursing git, and starting the merge again from\nscratch - if they even notice the problem. If they don't notice the\nproblem, then they will instead have a new and fascinating source of\nmerge conflicts at some future time.\n\nIn general I *will* be willing to make things a little harder for\nexperts in favor of novices - absolutely.\n\nThat said, I don't believe the (new) change proposed here strips away\n*useful* options from experts at all. When I said \"safe-and-limiting\",\nI meant it in the most literal way - that there are some operations\nthat could be performed before and would achieve certain outcomes that\nwon't be possible afterwards. What I didn't mean to imply is that\nthose options are *valuable* to anyone - even to experts.\n\n>\n> If there was a way of doing:\n>\n> git -c core.iknowwhatimdoing=true checkout $whatever\n>\n> Then I wouldn't oppose such change.\n\nI know I keep wavering back and forth on this, my apologies for my\ninconstancy: *I once again think adding support for \"--force\" (to\ncheckout and switch) with ongoing operations makes sense.*\n\nThis does not achieve exactly what you seem to be suggesting above,\nfor two reasons:\n1. It could not be implicit in config, but rather would need to be\nexplicit in the command\n2. The outcome of using --force is not exactly the same as \"git\ncheckout\" without it (but that's a good thing)\n\nI would (and will) argue that not achieving exactly what you propose\n*is OK* because the behavior of \"git checkout\", without \"--force\",\nwhen there is a (merge, rebase, cherry-pick, am, bisect) operation in\ncourse, especially the way that behavior differs from when \"--force\"\nis specified, is *not useful* - even to expert users.\n\nI will provide a table of behaviors with a proposed patch in a few\ndays, but basically the main behavior we're taking away is a\none-command behavior of \"switch branch and remove the (merge,\ncherry-pick) in-progress state\". The explicit equivalent is and will\ncontinue to be \"git [merge|cherry-pick] --quit && git checkout\" -\nleaving the in-progress merge changes in the index, but switching to\nthe specified branch.\n\nMy expectation is that this is not something even expert users find\nthemselves doing... ever. But I would like to know about it if I'm\nwrong of course!\n\nSomething that I *do* see quite a lot in the test suite, and is\nprompting my turn-about on \"--force\" support, is \"git checkout -f\nwhatever\" as a shorthand for \"just get my worktree to the state of\nthat branch, regardless of the current ongoing operation\".\n\nThis shorthand happens to *fail to work correctly* during a rebase\n(clearing of the rebasing state was never implemented), but I believe\nthat has more to do with priorities and scope of changes than\nintentional \"let's set an extra-confusing trap for rebase\" reasons.\nThe resulting state, where you have switched to the requested branch\nand discarded any local changes, but are still in a rebase,\npotentially with pending rebase sequence steps to complete, is not one\nthat I can see even expert users making constructive use of.\n\nGenerally, the \"allow 'checkout --force' to destroy in-progress\noperation states\" behavior looks like an expert shortcut worth\npreserving, and improving/fixing in the case of rebase.\n\n>\n> But this is not the proposal. The proposal is to break backwards compatibility\n> for expert users with no way to retain the existing behavior.\n>\n\nIt is true that the (updated) proposal closes the doors on *specific*\nbehaviors as a single command, requiring them to instead be spread\nacross two commands. However, I believe that those are effectively\n*unused behaviors*, and that the increase in consistency and safety,\nfor all users, by far outweighs the cost of this particular break in\nbackwards compatibility.\n\nI am, again, very interested in anything I might be missing!\n\n> Generally, breaking backwards compatibility for no reason is frowned upon.\n>\n\nI absolutely understand and agree that breaking backwards\ncompatibility *for no reason* is never the right thing - and I take\nnote that being clear about exactly what the reasons are, and what the\ncosts are, *before* talking about doing it and asking for opinions, is\nadvisable and something that I failed to do sensibly here.\n\nThanks again for the feedback, please let me know if you know of any\nuseful expert use cases that I *am* missing in this updated proposal.\n"},{"id":"476740","messageId":"CAPMMpojDm8jHWFr8i5EC-oEKK8WBt1g3iyRvixfy1bhk8qck2g@mail.gmail.com","threadId":"59681","inReplyTo":"CABPp-BGmPKyNcDa-wUh-oisTvvux+X=6BvGxSNQC2O7uodpFrA@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-08T10:44:45Z","receivedAt":"2023-05-08T10:46:01Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Sun, May 7, 2023 at 4:48 AM Elijah Newren wrote:\n>\n> On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n> >\n> > I believe this question was resolved later in the thread. The proposal\n> > is to allow the simplest case of merge only, for resolved\n> > (unconflicted) indexes only. If the change were to make sense I could\n> > update this message to be clearer that none of those other operations\n> > or situations are impacted by this change.\n>\n> As I mentioned to Junio, I understood fully that your implementation\n> limited the changes to this one case. That did not resolve my\n> concerns, it merely obviated some other bigger ones that I didn't\n> raise.\n>\n> However, making it only available via a --force override (and then\n> perhaps also limiting it to just some operations), would resolve my\n> concerns.\n>\n\nHmm, I think there is confusion here.\n\nMy proposal was (and now, again, is) to add support for \"--force\" to\n\"git switch\", and to keep and improve that existing support for \"git\ncheckout\" (where it is in my opinion broken during a rebase), but that\nproposal was mostly-unrelated to my main goal and proposal for\nsupporting same-commit switches in the first place:\n\nA same-commit switch (*without* --force) serves the use-case of\n*completing a merge on another branch*. This is, as far as I can tell\nonly *useful* for merges:\n * during a rebase, switching in the middle (to the same commit,\nwithout --force) won't achieve anything useful; your rebase is still\nin progress, any previously rebased commits in the sequence are lost,\nand if you continue the rebase you'll end up with a very strange and\nlikely-surprising partial rebase state)\n * during a cherry-pick, it's just \"not very useful\" - it's not bad\nlike rebase, because in-progress cherry-pick metadata is destroyed\n * during am, and bisect I'm not sure, I haven't tested yet.\n\nThe reason this in-progress is *valuable* for merges (in a way that it\nis not for those other states) is that the merge metadata not only\nsays what you're in the middle of, but also contains additional useful\ninformation about what you've done so far, which you want to have be a\npart of what you commit in the end - the identity of the commit you\nwere merging in.\n\nSupporting switch with --force, and having it implicitly destroy\nin-progress operation metadata, has value in that it makes it easier\nto break backwards compatibility of \"git checkout\" without impacting\nusers' or tests' workflows; it helps make a change to make checkout\nsafer; but it does not help with my other (/main?) objective of making\nit easy and intuitive to switch to another same-commit branch, to be\nable to commit your in-progress merge on another branch and avoid\ncommitting it where you started.\n\nHence, if/when we add support for same-commit switching during merge\n(and potentially other operations, if that makes sense), it should\n*not* take \"--force\", which has a substantially different purpose and\nmeaning.\n\n> > > My first gut guess is that switching with conflicts would be just as\n> > > safe as this is, and any users who likes your change is going to\n> > > complain if we don't allow it during conflicts.\n> >\n> > In principle I believe so too, I just haven't checked whether the\n> > tree-merge process attempts to do anything for a same-commit switch,\n> > and if it does, whether the presence of conflict data \"bothers\" it in\n> > any way / causes it to do the wrong thing, eg remove it.\n> >\n> > If verifying this and opening up the \"pending conflicts\" case meets\n> > the consistency itch, I'm happy to explore this area and (try to)\n> > expand the scope of the fix/exemption.\n>\n> If this behavior is behind a `--force` flag rather than the default\n> behavior, then I think there's much more leniency for a partial\n> solution.\n\nBut if it were behind \"--force\", it wouldn't work :)\n\n>\n> That said, I do still think it'd be nice to handle this case for\n> consistency, so if you're willing to take a look, that'd be great. If\n> you are interested, here's a pointer: Stolee's commit 313677627a8\n> (\"checkout: add simple check for 'git checkout -b'\", 2019-08-29) might\n> be of interest here. Essentially, when switching to a same-commit\n> branch, you can short-circuit most of the work and basically just\n> update HEAD. (In his case, he was creating _and_ switching to a\n> branch, and he was essentially just trying to short-circuit the\n> reading and writing of the index since he knew there would be no\n> changes, but the same basic logic almost certainly applies to this\n> broader case -- no index changes are needed, so the existence of\n> conflicts shouldn't matter.)\n\nWill look, thx\n\n>\n> If you don't want to handle that case, though, you should probably\n> think about what kind of message to give the user if they try to\n> `--force` the checkout and they have conflicts. They'd probably\n> deserve a longer explanation due to the inconsistency.\n>\n\n--force implicitly and intentionally discards the conflicts.\n\n> > > But I think it'd take\n> > > a fair amount of work to figure out if it's safe during\n> > > rebase/cherry-pick/am/revert (is it only okay on the very first patch\n> > > of a series? And only if non-interactive? And only without\n> > > --autostash and --update-refs? etc.), and whether the ending set of\n> > > rules feels horribly inconsistent or feels fine to support.\n> >\n> > I agree this gets complicated - I haven't thought or explored through\n> > most of these, but I have confirmed that switching branch in the\n> > middle of a *rebase* is very confusing: your rebase continues on the\n> > new HEAD, as you continue to commit, your rebased commits get\n> > committed to the branch you switched to, but at the end when you\n> > *complete* the rebase, the original ref you were rebasing still ends\n> > up being pointed to the new HEAD - so you end up with *both* the\n> > branch you were rebasing, and the branch you switched to along the\n> > way, pointing to the same head commit.\n> >\n> > I understand how that works in terms of git's internal logic, but as a\n> > user of rebase, if I tried to switch (to a new branch) in the middle,\n> > I would be intending to say \"I got scared of the changes I'm making\n> > here, I want the that is ref pointed to the new commit graph at the\n> > end of the process to be this new ref, instead of the ref I originally\n> > started on\".\n> >\n> > Supporting that usecase, for rebase, sounds to me like it should be\n> > done by something completely different to \"git switch\". The most\n> > helpful behavior I can think of here would be that a \"git switch\"\n> > attempt would say \"cannot switch branch in the middle of a rebase. to\n> > continue your rebase and create a new branch, use 'git rebase\n> > --make-new-branch NEWBRANCHNAME\" instead of 'git switch'\"\n>\n> That all sounds reasonable.\n>\n> But you know someone is going to try it anyway during a\n> rebase/cherry-pick/revert. If we start letting `--force` override\n> during a merge, we should do something to address that inconsistency\n> for users. It doesn't need to be something big; we could likely\n> address it by just specifically checking for the `--force` case during\n> a rebase/cherry-pick/revert and providing an even more detailed error\n> message in that case that spells out why the operation cannot be\n> `--force`d.\n\nThe behavior of \"--force\" is already clear - it resets your worktree\nto the state of the branch you are switching to. It also (or should\nbut doesn't, in the case of rebase) destroys in-progress operation\nstate/metadata.\n\nThat said, I understand and agree that there should be a difference\nbetween a generic error \"there is an operation in progress, you need\nto '--abort'\" for the operation types that can and should not benefit\nfrom a same-commit exception, and the operation(s) that do get a\nsame-commit exception when it doesn't apply (when you're trying to\nswitch commit). If the same-commit does end up behind some parameter,\nthere should be yet another message for a same-commit branch switch\noperation when the new needed parameter is not specified.\n\n>\n> > > > Also add a warning when the merge metadata is deleted (in case of a\n> > > > \"git checkout\" to another commit) to let the user know the merge state\n> > > > was lost, and that \"git switch\" would prevent this.\n> > >\n> > > If we're touching this area, we should employ the right fix rather\n> > > than a half measure. As I mentioned above, this should be an error\n> > > with the operation prevented -- just like switch behaves.\n> > >\n> >\n> > My understanding, given the code organization, was that we wanted to\n> > preserve current (funky) behavior for backwards-compatibility\n> > purposes.\n>\n> I totally understand how you'd reach that conclusion. I would\n> probably come to the same one reading the code for the first time.\n> But, as it turns out, that's not how things happened.\n>\n> > If we're comfortable changing behavior here, I am happy to\n> > change the patch (while keeping/allowing the --force exemption, which\n> > *should* still destroy the merge state).\n>\n> Yaay!\n\nAs suggested in my recent response to Felipe, I will create a separate\npatch (series) for the git checkout safety enhancements and related\n--force support enhancements.\n\n>\n> > > > Also add a warning when the merge metadata is preserved (same commit),\n> > > > to let the user know the commit message prepared for the merge may still\n> > > > refer to the previous branch.\n> > >\n> > > So, it's not entirely safe even when the commit of the target branch\n> > > matches HEAD? Is that perhaps reason to just leave this for expert\n> > > users to use the update-refs workaround?\n> > >\n> >\n> > It is *safe*, it's just that one aspect of the outcome is *potentially\n> > confusing*. You really did do the merge on the original branch. The\n> > merge message is the same as it would be if you committed, created a\n> > new branch, and reset the original branch.\n> >\n> > (and just to note - the reasonable workaround is to commit the merge\n> > on the current \"wrong\" branch, create the other branch, and then reset\n> > the original branch, as Chris Torek shows on StackOverflow; not to\n> > teach people all about update-refs)\n> >\n> >\n> > Thanks so much for taking the time to go through all this!\n> >\n> > Please let me know whether you would be comfortable with a patch that:\n> > * Fixed checkout to be more restrictive\n>\n> Absolutely.\n>\n> > (except still allowing --force at least on a merging state)\n>\n> That's fine too.\n>\n> > * More explicitly noted that we are relaxing things for merge only,\n> > none of the other in-progress states that currently prevent switch\n>\n> That wouldn't resolve any of my concerns; it was totally clear to me\n> the first time.\n>\n> > * Also worked with outstanding conflicts in the index (verifying that\n> > this is safe)\n>\n> In combination with `--force`, I think that would be very nice.\n\nI need this to work without --force, for the reasons noted above, *but\nI will split this into two patch series to avoid further confusion!*\n\nThanks so much for your help!\n"},{"id":"476750","messageId":"64591fbddaf2d_7c6829457@chronos.notmuch","threadId":"59681","inReplyTo":"CAPMMpojTjFn7JCo8QsDcOJf6NoJYASbV1bL_JxDhUr7DS12DJg@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-08T16:13:49Z","receivedAt":"2023-05-08T16:14:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Tao Klerks wrote:\n> On Mon, May 8, 2023 at 12:01 AM Felipe Contreras\n> wrote:\n> > Elijah Newren wrote:\n> > > On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n> >\n> > > > If we are comfortable changing the behavior of branch checkout to be\n> > > > safe-and-limiting like switch, then that should be almost as simple as\n> > > > removing that condition.\n> > >\n> > > I've never heard a dissenting vote against this\n> >\n> > Here is my dissenting vote: I'm against this change.\n> >\n> > If I want to use a high-level command meant for novices, I use `git switch`. If\n> > instead I simply want to switch to a different commit and I want git to shut up\n> > about it, then I use `git checkout`.\n> \n> Thank you for your perspective on the relationship between these commands.\n> \n> I don't fully share this perspective, in two ways:\n> - In my experience most novices don't see or know about \"git switch\"\n> at all - the vast majority of the internet is still stuck on \"git\n> checkout\", as are existing users. Google search result counts are of\n> course a poor metric of anything, but compare 100k for \"git switch\" to\n> 2.4M for \"git checkout\".\n\nYes, but that's something for the Git community to fix.\n\nWhy can't the git developers communicate effectively with the user base?\n\n> - As far as I can tell, \"git switch\" and \"git restore\" have exactly\n> the same power and expressiveness (except specifically the lack of\n> \"git switch --force\" support for bulldozing ongoing merges) - they are\n> just as much \"expert\" tools as \"git checkout\"; the main way they\n> differ is that they are clearer about what they're doing / what\n> they're for.\n\nThat is not true, you can't do `git switch master^0` because that would be\npotentially confusing to new users, but you can do the same with `git\ncheckout`.\n\n> I'd love to see \"git checkout\" deprecated one day, although I'm not\n> sure I'll live to see it happen :)\n\nBut that's not an excuse to break user experience.\n\n> > If there was a way of doing:\n> >\n> > git -c core.iknowwhatimdoing=true checkout $whatever\n> >\n> > Then I wouldn't oppose such change.\n> \n> I know I keep wavering back and forth on this, my apologies for my\n> inconstancy: *I once again think adding support for \"--force\" (to\n> checkout and switch) with ongoing operations makes sense.*\n> \n> This does not achieve exactly what you seem to be suggesting above,\n> for two reasons:\n> 1. It could not be implicit in config, but rather would need to be\n> explicit in the command\n> 2. The outcome of using --force is not exactly the same as \"git\n> checkout\" without it (but that's a good thing)\n> \n> I would (and will) argue that not achieving exactly what you propose\n> *is OK* because the behavior of \"git checkout\", without \"--force\",\n> when there is a (merge, rebase, cherry-pick, am, bisect) operation in\n> course, especially the way that behavior differs from when \"--force\"\n> is specified, is *not useful* - even to expert users.\n\nOK. That may be the case.\n\nBut it wouldn't be the first time some operation is considered not\nuseful, and then it turns out people did in fact use it.\n\nI would be much more confortable if there was a way to retain the\ncurrent behavior, but if we are 99.99% positive nobody is actually\nrelying on this behavior, we could chose to roll the die and see what\nhappens (hopefully nobody will shout).\n\nBut if that's the case, I think this is something that should be a\nconscious decision that is extremely clear in the commit message.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"476755","messageId":"CAPMMpoi74RFBptKkv23FSK-fQsnuan9EK5HodUBRLNtxLYdr_w@mail.gmail.com","threadId":"59681","inReplyTo":"64591fbddaf2d_7c6829457@chronos.notmuch","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-08T16:58:42Z","receivedAt":"2023-05-08T16:59:01Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, May 8, 2023 at 6:13 PM Felipe Contreras\n wrote:\n>\n> Tao Klerks wrote:\n> > On Mon, May 8, 2023 at 12:01 AM Felipe Contreras\n> > wrote:\n> > > Elijah Newren wrote:\n> > > > On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n> > >\n> > > > > If we are comfortable changing the behavior of branch checkout to be\n> > > > > safe-and-limiting like switch, then that should be almost as simple as\n> > > > > removing that condition.\n> > > >\n> > > > I've never heard a dissenting vote against this\n> > >\n> > > Here is my dissenting vote: I'm against this change.\n> > >\n> > > If I want to use a high-level command meant for novices, I use `git switch`. If\n> > > instead I simply want to switch to a different commit and I want git to shut up\n> > > about it, then I use `git checkout`.\n> >\n> > Thank you for your perspective on the relationship between these commands.\n> >\n> > I don't fully share this perspective, in two ways:\n> > - In my experience most novices don't see or know about \"git switch\"\n> > at all - the vast majority of the internet is still stuck on \"git\n> > checkout\", as are existing users. Google search result counts are of\n> > course a poor metric of anything, but compare 100k for \"git switch\" to\n> > 2.4M for \"git checkout\".\n>\n> Yes, but that's something for the Git community to fix.\n>\n> Why can't the git developers communicate effectively with the user base?\n\nEmm... I'm going to take that as a rhetorical question, since you've\nbeen around these parts for a lot longer than I have :)\n\n(I have opinions, but they are not pertinent to this thread, and I\ndon't have meaningful solutions)\n\n>\n> > - As far as I can tell, \"git switch\" and \"git restore\" have exactly\n> > the same power and expressiveness (except specifically the lack of\n> > \"git switch --force\" support for bulldozing ongoing merges) - they are\n> > just as much \"expert\" tools as \"git checkout\"; the main way they\n> > differ is that they are clearer about what they're doing / what\n> > they're for.\n>\n> That is not true, you can't do `git switch master^0` because that would be\n> potentially confusing to new users, but you can do the same with `git\n> checkout`.\n\nAh, I see your point - git switch requires you to be more verbose in\nthis case, specifying an extra --detach.\n\n>\n> > > If there was a way of doing:\n> > >\n> > > git -c core.iknowwhatimdoing=true checkout $whatever\n> > >\n> > > Then I wouldn't oppose such change.\n> >\n> > I know I keep wavering back and forth on this, my apologies for my\n> > inconstancy: *I once again think adding support for \"--force\" (to\n> > checkout and switch) with ongoing operations makes sense.*\n> >\n> > This does not achieve exactly what you seem to be suggesting above,\n> > for two reasons:\n> > 1. It could not be implicit in config, but rather would need to be\n> > explicit in the command\n> > 2. The outcome of using --force is not exactly the same as \"git\n> > checkout\" without it (but that's a good thing)\n> >\n> > I would (and will) argue that not achieving exactly what you propose\n> > *is OK* because the behavior of \"git checkout\", without \"--force\",\n> > when there is a (merge, rebase, cherry-pick, am, bisect) operation in\n> > course, especially the way that behavior differs from when \"--force\"\n> > is specified, is *not useful* - even to expert users.\n>\n> OK. That may be the case.\n>\n> But it wouldn't be the first time some operation is considered not\n> useful, and then it turns out people did in fact use it.\n>\n> I would be much more confortable if there was a way to retain the\n> current behavior, but if we are 99.99% positive nobody is actually\n> relying on this behavior, we could chose to roll the die and see what\n> happens (hopefully nobody will shout).\n\nIt sounds like you're distinguishing here between \"options for\nexperts\" (which should be valuable to warrant influencing the\nlong-term design) and \"behavior that users and systems may have come\nto rely on\". As I've argued here, I believe that the current behavior\nis not *useful*, and thus a \"but the expert users\" argument doesn't\nsway me at all... On the other hand, the \"we shouldn't break existing\n(scripted/automated) uses\" argument seems much more convincing, and\nmore in line with what I was fishing for in my first question about a\n\"breaking changes process\".\n\nI haven't found any use cases that I could imagine anyone credibly\nautomating against, but I did find some tests in the suite that were\ndoing (in my opinion) \"the wrong thing\" and need to be modified:\n\n```\n# fails for some reason other than conflicts, eg commit hook\ngit cherry-pick XXXXX\n\n# previously succeeded, removing cherry-pick state but leaving modified index;\n# will now newly fail with \"you need to --abort first\"\ngit checkout main\n\n# cleans up modified index state\ngit reset --hard\n```\n\nI can't imagine this pattern being used in real-life automation, but\nlike anyone my imagination is limited.\n\nMaking this behave correctly, after my planned changes, is very\nsimple: replace \"git checkout && git reset --hard\" with \"git checkout\n-f\", or even just with \"git cherry-pick --abort\". But it is still a\nchange in behavior that *could* cause breakage if anyone implemented a\ncorner-case cleanup process in the same way those particular tests\ndid. I believe this particular example is vanishingly unlikely,\n*because it doesn't deal with conflicts*. If the cherry-pick had left\nany conflicted files, then the checkout would have failed.\n\nThe question, I understand, is whether there should be a \"git -c\ncore.suckycheckoutstatemanagement=true checkout\" option *just in\ncase*, so any affected automation users could set it, fix their\naffected automated processes, and then remove it, before we finally\nremove the \"core.suckycheckoutstatemanagement\" option in a subsequent\nrelease.\n\nHere is precisely where I don't know how to judge \"breakage risk and\nvalue of being able to revert behavior without downgrading git\" vs\n\"complexity of implementation and communication\". Obviously I would\nprefer not to do a bunch of valueless work implementing and supporting\nan option that no-one would ever use, and removing it a couple months\nlater. I wonder, for example, whether there is any recommendation that\nautomation users be willing and able to downgrade git temporarily, or\nnot. That would be one way to make the risk of this kind of\n\"corner-case breakage\" more acceptable.\n\n>\n> But if that's the case, I think this is something that should be a\n> conscious decision that is extremely clear in the commit message.\n>\n\nI will do my best :)\n"},{"id":"476796","messageId":"xmqqzg6eocmi.fsf@gitster.g","threadId":"59681","inReplyTo":"CAPMMpoi74RFBptKkv23FSK-fQsnuan9EK5HodUBRLNtxLYdr_w@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-08T19:18:45Z","receivedAt":"2023-05-08T19:19:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n> The question, I understand, is whether there should be a \"git -c\n> core.suckycheckoutstatemanagement=true checkout\" option *just in\n> case*, so any affected automation users could set it, fix their\n> affected automated processes, and then remove it, before we finally\n> remove the \"core.suckycheckoutstatemanagement\" option in a subsequent\n> release.\n\nIf a new behaviour is hidden behind a new option nobody has heard of\nbefore, you would not risk breaking anybody who wrote their scripts\nlong time ago and have relied on them the way they currently work,\nand the new option would not have to be removed at all. I think the\n\"switch\" was written exactly for such a transition so that folks who\nwanted a different behaviour do not have to break existing users of\n\"checkout\".\n\nDo we still mark \"switch\" as experimental in bold red letters in the\ndocumentation? Then it is not too late to improve the end-user\nexperimence with the command without worrying about too much about\nbackward compatibility.\n\n"},{"id":"476865","messageId":"6459a814ee378_7c682949e@chronos.notmuch","threadId":"59681","inReplyTo":"CAPMMpoi74RFBptKkv23FSK-fQsnuan9EK5HodUBRLNtxLYdr_w@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T01:55:32Z","receivedAt":"2023-05-09T01:55:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Tao Klerks wrote:\n> On Mon, May 8, 2023 at 6:13 PM Felipe Contreras\n> wrote:\n> > Tao Klerks wrote:\n> > > On Mon, May 8, 2023 at 12:01 AM Felipe Contreras\n> > > wrote:\n> > > > Elijah Newren wrote:\n> > > > > On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n> > > >\n> > > > > > If we are comfortable changing the behavior of branch checkout to be\n> > > > > > safe-and-limiting like switch, then that should be almost as simple as\n> > > > > > removing that condition.\n> > > > >\n> > > > > I've never heard a dissenting vote against this\n> > > >\n> > > > Here is my dissenting vote: I'm against this change.\n> > > >\n> > > > If I want to use a high-level command meant for novices, I use `git switch`. If\n> > > > instead I simply want to switch to a different commit and I want git to shut up\n> > > > about it, then I use `git checkout`.\n> > >\n> > > Thank you for your perspective on the relationship between these commands.\n> > >\n> > > I don't fully share this perspective, in two ways:\n> > > - In my experience most novices don't see or know about \"git switch\"\n> > > at all - the vast majority of the internet is still stuck on \"git\n> > > checkout\", as are existing users. Google search result counts are of\n> > > course a poor metric of anything, but compare 100k for \"git switch\" to\n> > > 2.4M for \"git checkout\".\n> >\n> > Yes, but that's something for the Git community to fix.\n> >\n> > Why can't the git developers communicate effectively with the user base?\n> \n> Emm... I'm going to take that as a rhetorical question, since you've\n> been around these parts for a lot longer than I have :)\n> \n> (I have opinions, but they are not pertinent to this thread, and I\n> don't have meaningful solutions)\n\nYes, it was rhetorical.\n\nThat being said, if you feel like sharing that opinion off the record,\nI'm interested in hearing it.\n\n> > > - As far as I can tell, \"git switch\" and \"git restore\" have exactly\n> > > the same power and expressiveness (except specifically the lack of\n> > > \"git switch --force\" support for bulldozing ongoing merges) - they are\n> > > just as much \"expert\" tools as \"git checkout\"; the main way they\n> > > differ is that they are clearer about what they're doing / what\n> > > they're for.\n> >\n> > That is not true, you can't do `git switch master^0` because that would be\n> > potentially confusing to new users, but you can do the same with `git\n> > checkout`.\n> \n> Ah, I see your point - git switch requires you to be more verbose in\n> this case, specifying an extra --detach.\n\nYes, because it's meant for more novice users.\n\n> > > > If there was a way of doing:\n> > > >\n> > > > git -c core.iknowwhatimdoing=true checkout $whatever\n> > > >\n> > > > Then I wouldn't oppose such change.\n> > >\n> > > I know I keep wavering back and forth on this, my apologies for my\n> > > inconstancy: *I once again think adding support for \"--force\" (to\n> > > checkout and switch) with ongoing operations makes sense.*\n> > >\n> > > This does not achieve exactly what you seem to be suggesting above,\n> > > for two reasons:\n> > > 1. It could not be implicit in config, but rather would need to be\n> > > explicit in the command\n> > > 2. The outcome of using --force is not exactly the same as \"git\n> > > checkout\" without it (but that's a good thing)\n> > >\n> > > I would (and will) argue that not achieving exactly what you propose\n> > > *is OK* because the behavior of \"git checkout\", without \"--force\",\n> > > when there is a (merge, rebase, cherry-pick, am, bisect) operation in\n> > > course, especially the way that behavior differs from when \"--force\"\n> > > is specified, is *not useful* - even to expert users.\n> >\n> > OK. That may be the case.\n> >\n> > But it wouldn't be the first time some operation is considered not\n> > useful, and then it turns out people did in fact use it.\n> >\n> > I would be much more confortable if there was a way to retain the\n> > current behavior, but if we are 99.99% positive nobody is actually\n> > relying on this behavior, we could chose to roll the die and see what\n> > happens (hopefully nobody will shout).\n> \n> It sounds like you're distinguishing here between \"options for\n> experts\" (which should be valuable to warrant influencing the\n> long-term design) and \"behavior that users and systems may have come\n> to rely on\".\n\nSure, they are different, but they are related.\n\nIf somebody has only one week of expertice with git, I think it's safe\nto say they don't rely on the current behavior that much.\n\nOn the other hand somebody who has 15 years of experience with git has a\nhigher chance of relying on the current behavior.\n\n> As I've argued here, I believe that the current behavior\n> is not *useful*, and thus a \"but the expert users\" argument doesn't\n> sway me at all...\n\nAnd you may be right, I'm not going to argue against such claim.\n\nBut this is an argument from ignorance fallacy. The last time I argued\n\"I cannot imagine how X might be the case\" turned out X was the case.\n\n> I can't imagine this pattern being used in real-life automation, but\n> like anyone my imagination is limited.\n\nIndeed.\n\nOnce again: I'm not saying this is going to break user expectations,\nbecause it might not. I'm saying this *might* break user expectations,\nbut we could still roll the dice and find out.\n\nUltimately this is not my decision, it's the decision of the maintainer.\n\n> Here is precisely where I don't know how to judge \"breakage risk and\n> value of being able to revert behavior without downgrading git\" vs\n> \"complexity of implementation and communication\". Obviously I would\n> prefer not to do a bunch of valueless work implementing and supporting\n> an option that no-one would ever use, and removing it a couple months\n> later.\n\nAgreed, which is why I'm not suggesting this work has to be done, but\nthe maintainer might (I've often done what I consider unnecessary work\njust because of that reason).\n\nAll I'm saying is that because the git project puts a premium on\npreserving backwards compatibility (as any decent software project\nshould), then:\n\n> > But if that's the case, I think this is something that should be a\n> > conscious decision that is extremely clear in the commit message.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"477027","messageId":"CABPp-BHGAVb06BahQ0--15LWetyyx7eHAoPH8-So9UyqpJv0sg@mail.gmail.com","threadId":"59681","inReplyTo":"CAPMMpojDm8jHWFr8i5EC-oEKK8WBt1g3iyRvixfy1bhk8qck2g@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-11T07:06:39Z","receivedAt":"2023-05-11T07:07:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, May 8, 2023 at 3:44 AM Tao Klerks wrote:\n>\n> On Sun, May 7, 2023 at 4:48 AM Elijah Newren wrote:\n> >\n> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n> > >\n> > > I believe this question was resolved later in the thread. The proposal\n> > > is to allow the simplest case of merge only, for resolved\n> > > (unconflicted) indexes only. If the change were to make sense I could\n> > > update this message to be clearer that none of those other operations\n> > > or situations are impacted by this change.\n> >\n> > As I mentioned to Junio, I understood fully that your implementation\n> > limited the changes to this one case. That did not resolve my\n> > concerns, it merely obviated some other bigger ones that I didn't\n> > raise.\n> >\n> > However, making it only available via a --force override (and then\n> > perhaps also limiting it to just some operations), would resolve my\n> > concerns.\n> >\n>\n> Hmm, I think there is confusion here.\n>\n> My proposal was (and now, again, is) to add support for \"--force\" to\n> \"git switch\", and to keep and improve that existing support for \"git\n> checkout\" (where it is in my opinion broken during a rebase), but that\n> proposal was mostly-unrelated to my main goal and proposal for\n> supporting same-commit switches in the first place:\n>\n> A same-commit switch (*without* --force) serves the use-case of\n> *completing a merge on another branch*. This is, as far as I can tell\n> only *useful* for merges:\n> * during a rebase, switching in the middle (to the same commit,\n> without --force) won't achieve anything useful; your rebase is still\n> in progress, any previously rebased commits in the sequence are lost,\n> and if you continue the rebase you'll end up with a very strange and\n> likely-surprising partial rebase state)\n> * during a cherry-pick, it's just \"not very useful\" - it's not bad\n> like rebase, because in-progress cherry-pick metadata is destroyed\n> * during am, and bisect I'm not sure, I haven't tested yet.\n>\n> The reason this in-progress is *valuable* for merges (in a way that it\n> is not for those other states) is that the merge metadata not only\n> says what you're in the middle of, but also contains additional useful\n> information about what you've done so far, which you want to have be a\n> part of what you commit in the end - the identity of the commit you\n> were merging in.\n>\n> Supporting switch with --force, and having it implicitly destroy\n> in-progress operation metadata, has value in that it makes it easier\n> to break backwards compatibility of \"git checkout\" without impacting\n> users' or tests' workflows; it helps make a change to make checkout\n> safer; but it does not help with my other (/main?) objective of making\n> it easy and intuitive to switch to another same-commit branch, to be\n> able to commit your in-progress merge on another branch and avoid\n> committing it where you started.\n>\n> Hence, if/when we add support for same-commit switching during merge\n> (and potentially other operations, if that makes sense), it should\n> *not* take \"--force\", which has a substantially different purpose and\n> meaning.\n\nDoh, sorry, brain fart on my part forgetting the checkout/switch\nalready have a \"--force\". Replace \"--force\" in my email with \"an\noverride\" such as \"--ignore-in-progress\".\n"},{"id":"477638","messageId":"CAPMMpogFHnX2YPA4VmffmA0pku=43CQJ8iebCOkFm4ravBVTeg@mail.gmail.com","threadId":"59681","inReplyTo":"CAPMMpojDm8jHWFr8i5EC-oEKK8WBt1g3iyRvixfy1bhk8qck2g@mail.gmail.com","subject":"Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-21T20:08:45Z","receivedAt":"2023-05-21T20:09:00Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"FWIW, it looks like I bit off a lot more than I could chew when I\noffered/proposed to fix \"git checkout --force \" across all the\nin-progress operation types (and make it available in \"git switch\",\nand then disallow non-force \"git checkout\" with in-progress\noperations), especially given the 1-2 hours/week I'm managing to\nspend.\n\nI'm having enough troubles understanding all the ins & outs of the\ncurrent behavior that it will likely take me a few weeks to have any\nproposed changes.\n\nI will revive this thread at that time, if I manage to propose anything useful.\n\nOn Mon, May 8, 2023 at 12:44 PM Tao Klerks wrote:\n>\n> On Sun, May 7, 2023 at 4:48 AM Elijah Newren wrote:\n> >\n> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks wrote:\n> > >\n> > > I believe this question was resolved later in the thread. The proposal\n> > > is to allow the simplest case of merge only, for resolved\n> > > (unconflicted) indexes only. If the change were to make sense I could\n> > > update this message to be clearer that none of those other operations\n> > > or situations are impacted by this change.\n> >\n> > As I mentioned to Junio, I understood fully that your implementation\n> > limited the changes to this one case. That did not resolve my\n> > concerns, it merely obviated some other bigger ones that I didn't\n> > raise.\n> >\n> > However, making it only available via a --force override (and then\n> > perhaps also limiting it to just some operations), would resolve my\n> > concerns.\n> >\n>\n> Hmm, I think there is confusion here.\n>\n> My proposal was (and now, again, is) to add support for \"--force\" to\n> \"git switch\", and to keep and improve that existing support for \"git\n> checkout\" (where it is in my opinion broken during a rebase), but that\n> proposal was mostly-unrelated to my main goal and proposal for\n> supporting same-commit switches in the first place:\n>\n> A same-commit switch (*without* --force) serves the use-case of\n> *completing a merge on another branch*. This is, as far as I can tell\n> only *useful* for merges:\n> * during a rebase, switching in the middle (to the same commit,\n> without --force) won't achieve anything useful; your rebase is still\n> in progress, any previously rebased commits in the sequence are lost,\n> and if you continue the rebase you'll end up with a very strange and\n> likely-surprising partial rebase state)\n> * during a cherry-pick, it's just \"not very useful\" - it's not bad\n> like rebase, because in-progress cherry-pick metadata is destroyed\n> * during am, and bisect I'm not sure, I haven't tested yet.\n>\n> The reason this in-progress is *valuable* for merges (in a way that it\n> is not for those other states) is that the merge metadata not only\n> says what you're in the middle of, but also contains additional useful\n> information about what you've done so far, which you want to have be a\n> part of what you commit in the end - the identity of the commit you\n> were merging in.\n>\n> Supporting switch with --force, and having it implicitly destroy\n> in-progress operation metadata, has value in that it makes it easier\n> to break backwards compatibility of \"git checkout\" without impacting\n> users' or tests' workflows; it helps make a change to make checkout\n> safer; but it does not help with my other (/main?) objective of making\n> it easy and intuitive to switch to another same-commit branch, to be\n> able to commit your in-progress merge on another branch and avoid\n> committing it where you started.\n>\n> Hence, if/when we add support for same-commit switching during merge\n> (and potentially other operations, if that makes sense), it should\n> *not* take \"--force\", which has a substantially different purpose and\n> meaning.\n>\n> > > > My first gut guess is that switching with conflicts would be just as\n> > > > safe as this is, and any users who likes your change is going to\n> > > > complain if we don't allow it during conflicts.\n> > >\n> > > In principle I believe so too, I just haven't checked whether the\n> > > tree-merge process attempts to do anything for a same-commit switch,\n> > > and if it does, whether the presence of conflict data \"bothers\" it in\n> > > any way / causes it to do the wrong thing, eg remove it.\n> > >\n> > > If verifying this and opening up the \"pending conflicts\" case meets\n> > > the consistency itch, I'm happy to explore this area and (try to)\n> > > expand the scope of the fix/exemption.\n> >\n> > If this behavior is behind a `--force` flag rather than the default\n> > behavior, then I think there's much more leniency for a partial\n> > solution.\n>\n> But if it were behind \"--force\", it wouldn't work :)\n>\n> >\n> > That said, I do still think it'd be nice to handle this case for\n> > consistency, so if you're willing to take a look, that'd be great. If\n> > you are interested, here's a pointer: Stolee's commit 313677627a8\n> > (\"checkout: add simple check for 'git checkout -b'\", 2019-08-29) might\n> > be of interest here. Essentially, when switching to a same-commit\n> > branch, you can short-circuit most of the work and basically just\n> > update HEAD. (In his case, he was creating _and_ switching to a\n> > branch, and he was essentially just trying to short-circuit the\n> > reading and writing of the index since he knew there would be no\n> > changes, but the same basic logic almost certainly applies to this\n> > broader case -- no index changes are needed, so the existence of\n> > conflicts shouldn't matter.)\n>\n> Will look, thx\n>\n> >\n> > If you don't want to handle that case, though, you should probably\n> > think about what kind of message to give the user if they try to\n> > `--force` the checkout and they have conflicts. They'd probably\n> > deserve a longer explanation due to the inconsistency.\n> >\n>\n> --force implicitly and intentionally discards the conflicts.\n>\n> > > > But I think it'd take\n> > > > a fair amount of work to figure out if it's safe during\n> > > > rebase/cherry-pick/am/revert (is it only okay on the very first patch\n> > > > of a series? And only if non-interactive? And only without\n> > > > --autostash and --update-refs? etc.), and whether the ending set of\n> > > > rules feels horribly inconsistent or feels fine to support.\n> > >\n> > > I agree this gets complicated - I haven't thought or explored through\n> > > most of these, but I have confirmed that switching branch in the\n> > > middle of a *rebase* is very confusing: your rebase continues on the\n> > > new HEAD, as you continue to commit, your rebased commits get\n> > > committed to the branch you switched to, but at the end when you\n> > > *complete* the rebase, the original ref you were rebasing still ends\n> > > up being pointed to the new HEAD - so you end up with *both* the\n> > > branch you were rebasing, and the branch you switched to along the\n> > > way, pointing to the same head commit.\n> > >\n> > > I understand how that works in terms of git's internal logic, but as a\n> > > user of rebase, if I tried to switch (to a new branch) in the middle,\n> > > I would be intending to say \"I got scared of the changes I'm making\n> > > here, I want the that is ref pointed to the new commit graph at the\n> > > end of the process to be this new ref, instead of the ref I originally\n> > > started on\".\n> > >\n> > > Supporting that usecase, for rebase, sounds to me like it should be\n> > > done by something completely different to \"git switch\". The most\n> > > helpful behavior I can think of here would be that a \"git switch\"\n> > > attempt would say \"cannot switch branch in the middle of a rebase. to\n> > > continue your rebase and create a new branch, use 'git rebase\n> > > --make-new-branch NEWBRANCHNAME\" instead of 'git switch'\"\n> >\n> > That all sounds reasonable.\n> >\n> > But you know someone is going to try it anyway during a\n> > rebase/cherry-pick/revert. If we start letting `--force` override\n> > during a merge, we should do something to address that inconsistency\n> > for users. It doesn't need to be something big; we could likely\n> > address it by just specifically checking for the `--force` case during\n> > a rebase/cherry-pick/revert and providing an even more detailed error\n> > message in that case that spells out why the operation cannot be\n> > `--force`d.\n>\n> The behavior of \"--force\" is already clear - it resets your worktree\n> to the state of the branch you are switching to. It also (or should\n> but doesn't, in the case of rebase) destroys in-progress operation\n> state/metadata.\n>\n> That said, I understand and agree that there should be a difference\n> between a generic error \"there is an operation in progress, you need\n> to '--abort'\" for the operation types that can and should not benefit\n> from a same-commit exception, and the operation(s) that do get a\n> same-commit exception when it doesn't apply (when you're trying to\n> switch commit). If the same-commit does end up behind some parameter,\n> there should be yet another message for a same-commit branch switch\n> operation when the new needed parameter is not specified.\n>\n> >\n> > > > > Also add a warning when the merge metadata is deleted (in case of a\n> > > > > \"git checkout\" to another commit) to let the user know the merge state\n> > > > > was lost, and that \"git switch\" would prevent this.\n> > > >\n> > > > If we're touching this area, we should employ the right fix rather\n> > > > than a half measure. As I mentioned above, this should be an error\n> > > > with the operation prevented -- just like switch behaves.\n> > > >\n> > >\n> > > My understanding, given the code organization, was that we wanted to\n> > > preserve current (funky) behavior for backwards-compatibility\n> > > purposes.\n> >\n> > I totally understand how you'd reach that conclusion. I would\n> > probably come to the same one reading the code for the first time.\n> > But, as it turns out, that's not how things happened.\n> >\n> > > If we're comfortable changing behavior here, I am happy to\n> > > change the patch (while keeping/allowing the --force exemption, which\n> > > *should* still destroy the merge state).\n> >\n> > Yaay!\n>\n> As suggested in my recent response to Felipe, I will create a separate\n> patch (series) for the git checkout safety enhancements and related\n> --force support enhancements.\n>\n> >\n> > > > > Also add a warning when the merge metadata is preserved (same commit),\n> > > > > to let the user know the commit message prepared for the merge may still\n> > > > > refer to the previous branch.\n> > > >\n> > > > So, it's not entirely safe even when the commit of the target branch\n> > > > matches HEAD? Is that perhaps reason to just leave this for expert\n> > > > users to use the update-refs workaround?\n> > > >\n> > >\n> > > It is *safe*, it's just that one aspect of the outcome is *potentially\n> > > confusing*. You really did do the merge on the original branch. The\n> > > merge message is the same as it would be if you committed, created a\n> > > new branch, and reset the original branch.\n> > >\n> > > (and just to note - the reasonable workaround is to commit the merge\n> > > on the current \"wrong\" branch, create the other branch, and then reset\n> > > the original branch, as Chris Torek shows on StackOverflow; not to\n> > > teach people all about update-refs)\n> > >\n> > >\n> > > Thanks so much for taking the time to go through all this!\n> > >\n> > > Please let me know whether you would be comfortable with a patch that:\n> > > * Fixed checkout to be more restrictive\n> >\n> > Absolutely.\n> >\n> > > (except still allowing --force at least on a merging state)\n> >\n> > That's fine too.\n> >\n> > > * More explicitly noted that we are relaxing things for merge only,\n> > > none of the other in-progress states that currently prevent switch\n> >\n> > That wouldn't resolve any of my concerns; it was totally clear to me\n> > the first time.\n> >\n> > > * Also worked with outstanding conflicts in the index (verifying that\n> > > this is safe)\n> >\n> > In combination with `--force`, I think that would be very nice.\n>\n> I need this to work without --force, for the reasons noted above, *but\n> I will split this into two patch series to avoid further confusion!*\n>\n> Thanks so much for your help!\n"}]}