{"thread":{"id":"61719","subject":"[PATCH] checkout: special case error messages during noop switching","startedAt":"2024-07-02T20:51:49Z","lastAt":"2024-07-17T23:40:26Z","messageCount":4,"participants":["Junio C Hamano","Martin von Zweigbergk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497991","messageId":"xmqqikxnqzz4.fsf@gitster.g","threadId":"61719","inReplyTo":null,"subject":"[PATCH] checkout: special case error messages during noop switching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-02T20:51:43Z","receivedAt":"2024-07-02T20:51:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git checkout\" ran with no branch and no pathspec behaves like\nswitching the branch to the current branch (in other words, a\nno-op, except that it gives a side-effect \"here are the modified\npaths\" report).  But unlike \"git checkout HEAD\" or \"git checkout\nmain\" (when you are on the 'main' branch), the user is much less\nconscious that they are \"switching\" to the current branch.\n\nThis twists end-user expectation in a strange way.  There are\noptions (like \"--ours\") that make sense only when we are checking\nout paths out of either the tree-ish or out of the index.  So the\nerror message the command below gives\n\n    $ git checkout --ours\n    fatal: '--ours/theirs' cannot be used with switching branches\n\nis technically correct, but because the end-user may not even be\naware of the fact that the command they are issuing is about no-op\nbranch switching [*], they may find the error confusing.\n\nLet's refactor the code to make it easier to special case the \"no-op\nbranch switching\" situation, and then customize the exact error\nmessage for \"--ours/--theirs\".  Since it is more likely that the\nend-user forgot to give pathspec that is required by the option,\nlet's make it say\n\n    $ git checkout --ours\n    fatal: '--ours/theirs' needs the paths to check out\n\ninstead.\n\nAmong the other options that are incompatible with branch switching,\nthere may be some that benefit by having messages tweaked when a\nno-op branch switching is done, but I'll leave them as #leftoverbits\nmaterial.\n\n[Footnote]\n\n * Yes, the end-users are irrational.  When they did not give\n   \"--ours\", they take it granted that \"git checkout\" gives a short\n   status, e.g..\n\n    $ git checkout\n    M\tbuiltin/checkout.c\n    M\tt/t7201-co.sh\n\n   exactly as a branch switching command.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/checkout.c | 21 ++++++++++++++-------\n t/t7201-co.sh      | 13 +++++++++++++\n 2 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3cf44b4683..1748d68c96 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1572,6 +1572,10 @@ static void die_if_switching_to_a_branch_in_use(struct checkout_opts *opts,\n static int checkout_branch(struct checkout_opts *opts,\n \t\t\t   struct branch_info *new_branch_info)\n {\n+\tint noop_switch = (!new_branch_info->name &&\n+\t\t\t   !opts->new_branch &&\n+\t\t\t   !opts->force_detach);\n+\n \tif (opts->pathspec.nr)\n \t\tdie(_(\"paths cannot be used with switching branches\"));\n \n@@ -1583,9 +1587,14 @@ static int checkout_branch(struct checkout_opts *opts,\n \t\tdie(_(\"'%s' cannot be used with switching branches\"),\n \t\t    \"--[no]-overlay\");\n \n-\tif (opts->writeout_stage)\n-\t\tdie(_(\"'%s' cannot be used with switching branches\"),\n-\t\t    \"--ours/--theirs\");\n+\tif (opts->writeout_stage) {\n+\t\tconst char *msg;\n+\t\tif (noop_switch)\n+\t\t\tmsg = _(\"'%s' needs the paths to check out\");\n+\t\telse\n+\t\t\tmsg = _(\"'%s' cannot be used with switching branches\");\n+\t\tdie(msg, \"--ours/--theirs\");\n+\t}\n \n \tif (opts->force && opts->merge)\n \t\tdie(_(\"'%s' cannot be used with '%s'\"), \"-f\", \"-m\");\n@@ -1612,10 +1621,8 @@ static int checkout_branch(struct checkout_opts *opts,\n \t\tdie(_(\"Cannot switch branch to a non-commit '%s'\"),\n \t\t    new_branch_info->name);\n \n-\tif (!opts->switch_branch_doing_nothing_is_ok &&\n-\t    !new_branch_info->name &&\n-\t    !opts->new_branch &&\n-\t    !opts->force_detach)\n+\tif (noop_switch &&\n+\t    !opts->switch_branch_doing_nothing_is_ok)\n \t\tdie(_(\"missing branch or commit argument\"));\n \n \tif (!opts->implicit_detach &&\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 42352dc0db..793da6e64e 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -497,6 +497,19 @@ test_expect_success 'checkout unmerged stage' '\n \ttest ztheirside = \"z$(cat file)\"\n '\n \n+test_expect_success 'checkout --ours is incompatible with switching' '\n+\ttest_must_fail git checkout --ours 2>error &&\n+\ttest_grep \"needs the paths to check out\" error &&\n+\n+\ttest_must_fail git checkout --ours HEAD 2>error &&\n+\ttest_grep \"cannot be used with switching\" error &&\n+\n+\ttest_must_fail git checkout --ours main 2>error &&\n+\ttest_grep \"cannot be used with switching\" error &&\n+\n+\tgit checkout --ours file\n+'\n+\n test_expect_success 'checkout path with --merge from tree-ish is a no-no' '\n \tsetup_conflicting_index &&\n \ttest_must_fail git checkout -m HEAD -- file\n-- \n2.45.2-892-g51054c73d0\n\n"},{"id":"498837","messageId":"CANiSa6hs1AEp1e+o0hT55DvCwPe2EUyU1EXg1E4BKCkeuEOPvw@mail.gmail.com","threadId":"61719","inReplyTo":"xmqqikxnqzz4.fsf@gitster.g","subject":"Re: [PATCH] checkout: special case error messages during noop switching","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2024-07-17T16:05:13Z","receivedAt":"2024-07-17T16:05:26Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Thanks! (This is a fix for a bug I reported internally at work.)\n\nOn Tue, Jul 2, 2024 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"git checkout\" ran with no branch and no pathspec behaves like\n> switching the branch to the current branch (in other words, a\n> no-op, except that it gives a side-effect \"here are the modified\n> paths\" report).  But unlike \"git checkout HEAD\" or \"git checkout\n> main\" (when you are on the 'main' branch), the user is much less\n> conscious that they are \"switching\" to the current branch.\n\nYes, that's exactly what happened to me. I should have used `git\nrestore` instead. I know that's the modern way of updating paths, so I\ndon't know why I didn't think of it. I just verified that `git restore\n--ours <path>` works for restoring a conflicted file to my side.\n\n> [Footnote]\n>\n>  * Yes, the end-users are irrational.  When they did not give\n>    \"--ours\", they take it granted that \"git checkout\" gives a short\n>    status, e.g..\n\nI actually did not even know that it does that :) I'm a bit surprised\nthat it does, especially since `git checkout <non-HEAD>` doesn't seem\nto do that. But that's off topic.\n"},{"id":"498838","messageId":"xmqqttgo3shk.fsf@gitster.g","threadId":"61719","inReplyTo":"CANiSa6hs1AEp1e+o0hT55DvCwPe2EUyU1EXg1E4BKCkeuEOPvw@mail.gmail.com","subject":"Re: [PATCH] checkout: special case error messages during noop switching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T16:15:03Z","receivedAt":"2024-07-17T16:15:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk <martinvonz@gmail.com> writes:\n\n>>  * Yes, the end-users are irrational.  When they did not give\n>>    \"--ours\", they take it granted that \"git checkout\" gives a short\n>>    status, e.g..\n>\n> I actually did not even know that it does that :) I'm a bit surprised\n> that it does, especially since `git checkout <non-HEAD>` doesn't seem\n> to do that. But that's off topic.\n\nIt does but you wouldn't know unless you have modified paths.\n\nAlso checkout and switch would only allow checking out another\nbranch when these modified paths are the same between the original\nHEAD and the other branch.\n\n        $ git reset --hard\n        $ echo >>GIT-VERSION-GEN\n        $ git checkout\n\tM       GIT-VERSION-GEN\n\nWith a modified file, no-op checkout, report only.\n\n\t$ git checkout next\n\tM       GIT-VERSION-GEN\n\tSwitched to branch 'next'\n\nWith a modified file, checkout another branch, with report.\n\n\t$ git checkout maint\n\terror: Your local changes to the following files would be overwritten...\n\t\tGIT-VERSION-GEN\n\tPlease commit your changes or stash them...\n\nWith a modified file, that is different between next and maint, fail\nto checkout the other branch.\n"},{"id":"498886","messageId":"xmqqwmlj1te9.fsf@gitster.g","threadId":"61719","inReplyTo":"CANiSa6hs1AEp1e+o0hT55DvCwPe2EUyU1EXg1E4BKCkeuEOPvw@mail.gmail.com","subject":"Re: [PATCH] checkout: special case error messages during noop switching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T23:38:22Z","receivedAt":"2024-07-17T23:40:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk <martinvonz@gmail.com> writes:\n\n> Thanks! (This is a fix for a bug I reported internally at work.)\n\nGlad if this worked for you.\n\nI'll mark the topic for 'next', but being so late in the cycle, one\nday before 2.46-rc1 gets tagged, it is unlikely that it will become\na part of the upcoming release.\n\nThanks.\n"}]}