{"thread":{"id":"57468","subject":"[PATCH v2] switch: mention the --detach option when dying due to lack of a branch","startedAt":"2022-02-24T06:47:29Z","lastAt":"2022-02-25T17:26:39Z","messageCount":6,"participants":["Alex Henrie","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"449395","messageId":"20220224064710.2252637-1-alexhenrie24@gmail.com","threadId":"57468","inReplyTo":null,"subject":"[PATCH v2] switch: mention the --detach option when dying due to lack of a branch","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-24T06:47:10Z","receivedAt":"2022-02-24T06:47:29Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Users who are accustomed to doing `git checkout <tag>` assume that\n`git switch <tag>` will do the same thing. Inform them of the --detach\noption so they aren't left wondering why `git switch` doesn't work but\n`git checkout` does.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nv2:\n- Rephrase advice\n- Print advice after error message\n- Create a new config variable to suppress the advice\n- Add tests\n---\n Documentation/config/advice.txt |  3 +++\n advice.c                        |  1 +\n advice.h                        |  1 +\n builtin/checkout.c              | 30 +++++++++++++++++++-----------\n t/t2060-switch.sh               | 11 +++++++++++\n 5 files changed, 35 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex adee26fbbb..c40eb09cb7 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -85,6 +85,9 @@ advice.*::\n \t\tlinkgit:git-switch[1] or linkgit:git-checkout[1]\n \t\tto move to the detach HEAD state, to instruct how to\n \t\tcreate a local branch after the fact.\n+\tsuggestDetachingHead::\n+\t\tAdvice shown when linkgit:git-switch[1] refuses to detach HEAD\n+\t\twithout the explicit `--detach` option.\n \tcheckoutAmbiguousRemoteBranchName::\n \t\tAdvice shown when the argument to\n \t\tlinkgit:git-checkout[1] and linkgit:git-switch[1]\ndiff --git a/advice.c b/advice.c\nindex e00d30254c..2e1fd48304 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -42,6 +42,7 @@ static struct {\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\n+\t[ADVICE_SUGGEST_DETACHING_HEAD]\t\t\t= { \"suggestDetachingHead\", 1 },\n \t[ADVICE_FETCH_SHOW_FORCED_UPDATES]\t\t= { \"fetchShowForcedUpdates\", 1 },\n \t[ADVICE_GRAFT_FILE_DEPRECATED]\t\t\t= { \"graftFileDeprecated\", 1 },\n \t[ADVICE_IGNORED_HOOK]\t\t\t\t= { \"ignoredHook\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a7521d6087..a3957123a1 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -20,6 +20,7 @@ struct string_list;\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\n+\tADVICE_SUGGEST_DETACHING_HEAD,\n \tADVICE_FETCH_SHOW_FORCED_UPDATES,\n \tADVICE_GRAFT_FILE_DEPRECATED,\n \tADVICE_IGNORED_HOOK,\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d9b31bbb6d..9244827ca0 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1397,23 +1397,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)\n {\n \tstruct object_id oid;\n \tchar *to_free;\n+\tint code;\n \n \tif (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) {\n \t\tconst char *ref = to_free;\n \n \t\tif (skip_prefix(ref, \"refs/tags/\", &ref))\n-\t\t\tdie(_(\"a branch is expected, got tag '%s'\"), ref);\n-\t\tif (skip_prefix(ref, \"refs/remotes/\", &ref))\n-\t\t\tdie(_(\"a branch is expected, got remote branch '%s'\"), ref);\n-\t\tdie(_(\"a branch is expected, got '%s'\"), ref);\n+\t\t\tcode = die_message(_(\"a branch is expected, got tag '%s'\"), ref);\n+\t\telse if (skip_prefix(ref, \"refs/remotes/\", &ref))\n+\t\t\tcode = die_message(_(\"a branch is expected, got remote branch '%s'\"), ref);\n+\t\telse\n+\t\t\tcode = die_message(_(\"a branch is expected, got '%s'\"), ref);\n \t}\n-\tif (branch_info->commit)\n-\t\tdie(_(\"a branch is expected, got commit '%s'\"), branch_info->name);\n-\t/*\n-\t * This case should never happen because we already die() on\n-\t * non-commit, but just in case.\n-\t */\n-\tdie(_(\"a branch is expected, got '%s'\"), branch_info->name);\n+\telse if (branch_info->commit)\n+\t\tcode = die_message(_(\"a branch is expected, got commit '%s'\"), branch_info->name);\n+\telse\n+\t\t/*\n+\t\t * This case should never happen because we already die() on\n+\t\t * non-commit, but just in case.\n+\t\t */\n+\t\tcode = die_message(_(\"a branch is expected, got '%s'\"), branch_info->name);\n+\n+\tif (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD))\n+\t\tadvise(_(\"If you want to detach HEAD at the commit, try again with the --detach option.\"));\n+\n+\texit(code);\n }\n \n static void die_if_some_operation_in_progress(void)\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex ebb961be29..f54691bac9 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -32,6 +32,17 @@ test_expect_success 'switch and detach' '\n \ttest_must_fail git symbolic-ref HEAD\n '\n \n+test_expect_success 'suggestion to detach' '\n+\ttest_must_fail git switch main^{commit} 2>stderr &&\n+\ttest_i18ngrep \"try again with the --detach option\" stderr\n+'\n+\n+test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '\n+\ttest_config advice.suggestDetachingHead false &&\n+\ttest_must_fail git switch main^{commit} 2>stderr &&\n+\ttest_i18ngrep ! \"try again with the --detach option\" stderr\n+'\n+\n test_expect_success 'switch and detach current branch' '\n \ttest_when_finished git switch main &&\n \tgit switch main &&\n-- \n2.35.1\n\n"},{"id":"449397","messageId":"220224.86sfs8abj6.gmgdl@evledraar.gmail.com","threadId":"57468","inReplyTo":"20220224064710.2252637-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v2] switch: mention the --detach option when dying due to lack of a branch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-24T08:53:09Z","receivedAt":"2022-02-24T08:56:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 23 2022, Alex Henrie wrote:\n\n> Users who are accustomed to doing `git checkout <tag>` assume that\n> `git switch <tag>` will do the same thing. Inform them of the --detach\n> option so they aren't left wondering why `git switch` doesn't work but\n> `git checkout` does.\n\nThanks, this looks good to me! FWIW while testing this v2 I came up with\nthis on top, which you may or may not want (some unrelated changes):\n\n1. We had a to_free but didn't free it, now we do. Didn't matter with\n   SANITIZE=leak, but FWIW valgrind with \"valgrind --leak-check=full\n   --show-leak-kinds=all\" is marginally happier\n\n2. Maybe s/code = /return / with a helper is easier to read, maybe not.\n\n3. That #2 makes the code/advice wrapper simpler (also with the to_free)\n\n4. Used test_cmp in the test to check the actual output we got, and added\n   a missing test for the \"tag\" case.\n\n   In any case s/test_i18ngrep/grep/ is the right thing nowadays for new code.\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9244827ca02..9e4d03343fa 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1393,34 +1393,39 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n \treturn status;\n }\n \n-static void die_expecting_a_branch(const struct branch_info *branch_info)\n+static int die_expecting_a_branch_message(const struct branch_info *branch_info, char **to_free)\n {\n \tstruct object_id oid;\n-\tchar *to_free;\n-\tint code;\n \n-\tif (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) {\n-\t\tconst char *ref = to_free;\n+\tif (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, to_free, 0) == 1) {\n+\t\tconst char *ref = *to_free;\n \n \t\tif (skip_prefix(ref, \"refs/tags/\", &ref))\n-\t\t\tcode = die_message(_(\"a branch is expected, got tag '%s'\"), ref);\n+\t\t\treturn die_message(_(\"a branch is expected, got tag '%s'\"), ref);\n \t\telse if (skip_prefix(ref, \"refs/remotes/\", &ref))\n-\t\t\tcode = die_message(_(\"a branch is expected, got remote branch '%s'\"), ref);\n+\t\t\treturn die_message(_(\"a branch is expected, got remote branch '%s'\"), ref);\n \t\telse\n-\t\t\tcode = die_message(_(\"a branch is expected, got '%s'\"), ref);\n+\t\t\treturn die_message(_(\"a branch is expected, got '%s'\"), ref);\n \t}\n \telse if (branch_info->commit)\n-\t\tcode = die_message(_(\"a branch is expected, got commit '%s'\"), branch_info->name);\n+\t\treturn die_message(_(\"a branch is expected, got commit '%s'\"), branch_info->name);\n \telse\n \t\t/*\n \t\t * This case should never happen because we already die() on\n \t\t * non-commit, but just in case.\n \t\t */\n-\t\tcode = die_message(_(\"a branch is expected, got '%s'\"), branch_info->name);\n+\t\treturn die_message(_(\"a branch is expected, got '%s'\"), branch_info->name);\n+}\n+\n+static void die_expecting_a_branch(const struct branch_info *branch_info)\n+{\n+\tchar *to_free = NULL;\n+\tint code = die_expecting_a_branch_message(branch_info, &to_free);\n \n \tif (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD))\n \t\tadvise(_(\"If you want to detach HEAD at the commit, try again with the --detach option.\"));\n \n+\tfree(to_free);\n \texit(code);\n }\n \ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex f54691bac9f..0708359ee24 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -32,15 +32,31 @@ test_expect_success 'switch and detach' '\n \ttest_must_fail git symbolic-ref HEAD\n '\n \n-test_expect_success 'suggestion to detach' '\n-\ttest_must_fail git switch main^{commit} 2>stderr &&\n-\ttest_i18ngrep \"try again with the --detach option\" stderr\n+test_expect_success 'suggestion to detach commit' '\n+\ttest_must_fail git switch main^{commit} 2>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: a branch is expected, got commit '\\''main^{commit}'\\''\n+\thint: If you want to detach HEAD at the commit, try again with the --detach option.\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'suggestion to detach tag' '\n+\ttest_must_fail git switch first 2>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: a branch is expected, got tag '\\''first'\\''\n+\thint: If you want to detach HEAD at the commit, try again with the --detach option.\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '\n \ttest_config advice.suggestDetachingHead false &&\n-\ttest_must_fail git switch main^{commit} 2>stderr &&\n-\ttest_i18ngrep ! \"try again with the --detach option\" stderr\n+\ttest_must_fail git switch main^{commit} 2>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: a branch is expected, got commit '\\''main^{commit}'\\''\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'switch and detach current branch' '\n"},{"id":"449492","messageId":"xmqqv8x4m6jn.fsf@gitster.g","threadId":"57468","inReplyTo":"20220224064710.2252637-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v2] switch: mention the --detach option when dying due to lack of a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-24T19:02:52Z","receivedAt":"2022-02-24T19:02:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index d9b31bbb6d..9244827ca0 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1397,23 +1397,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)\n>  {\n>  \tstruct object_id oid;\n>  \tchar *to_free;\n> +\tint code;\n>  \n>  \tif (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) {\n>  \t\tconst char *ref = to_free;\n>  \n>  \t\tif (skip_prefix(ref, \"refs/tags/\", &ref))\n> -\t\t\tdie(_(\"a branch is expected, got tag '%s'\"), ref);\n> -\t\tif (skip_prefix(ref, \"refs/remotes/\", &ref))\n> -\t\t\tdie(_(\"a branch is expected, got remote branch '%s'\"), ref);\n> -\t\tdie(_(\"a branch is expected, got '%s'\"), ref);\n> +\t\t\tcode = die_message(_(\"a branch is expected, got tag '%s'\"), ref);\n> +\t\telse if (skip_prefix(ref, \"refs/remotes/\", &ref))\n> +\t\t\tcode = die_message(_(\"a branch is expected, got remote branch '%s'\"), ref);\n> +\t\telse\n> +\t\t\tcode = die_message(_(\"a branch is expected, got '%s'\"), ref);\n>  \t}\n> -\tif (branch_info->commit)\n> -\t\tdie(_(\"a branch is expected, got commit '%s'\"), branch_info->name);\n\nIn the original code, when dwim_ref() said there is a unique hit, we\ndied with varying messages.  So it was OK to have this check not as\na part of if/elseif/... cascade.\n\n> -\t/*\n> -\t * This case should never happen because we already die() on\n> -\t * non-commit, but just in case.\n> -\t */\n> -\tdie(_(\"a branch is expected, got '%s'\"), branch_info->name);\n> +\telse if (branch_info->commit)\n\nBut now, new code only sets code without dying, so we avoid\noverwriting the code (and calling die_message() twice) by turning it\n\"else if\".  OK.\n\n> +\t\tcode = die_message(_(\"a branch is expected, got commit '%s'\"), branch_info->name);\n> +\telse\n> +\t\t/*\n> +\t\t * This case should never happen because we already die() on\n> +\t\t * non-commit, but just in case.\n> +\t\t */\n> +\t\tcode = die_message(_(\"a branch is expected, got '%s'\"), branch_info->name);\n\nOK.  So \"code\" gets assigned exactly once in the above\nif/elseif/... cascade.  Not defining the variable with\ninitialization at the beginning of this function is correct.\n\n> +\tif (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD))\n> +\t\tadvise(_(\"If you want to detach HEAD at the commit, try again with the --detach option.\"));\n> +\n> +\texit(code);\n>  }\n\nOK.\n\n> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\n> index ebb961be29..f54691bac9 100755\n> --- a/t/t2060-switch.sh\n> +++ b/t/t2060-switch.sh\n> @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' '\n>  \ttest_must_fail git symbolic-ref HEAD\n>  '\n>  \n> +test_expect_success 'suggestion to detach' '\n> +\ttest_must_fail git switch main^{commit} 2>stderr &&\n> +\ttest_i18ngrep \"try again with the --detach option\" stderr\n> +'\n> +\n> +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '\n> +\ttest_config advice.suggestDetachingHead false &&\n> +\ttest_must_fail git switch main^{commit} 2>stderr &&\n> +\ttest_i18ngrep ! \"try again with the --detach option\" stderr\n> +'\n\nOK, we try to be consistent with other tests in the file, and leave\ns/test_i18n// to a file-wide clean-up outside the topic.\n\n>  test_expect_success 'switch and detach current branch' '\n>  \ttest_when_finished git switch main &&\n>  \tgit switch main &&\n"},{"id":"449575","messageId":"220225.86h78n88gb.gmgdl@evledraar.gmail.com","threadId":"57468","inReplyTo":"xmqqv8x4m6jn.fsf@gitster.g","subject":"Re: [PATCH v2] switch: mention the --detach option when dying due to lack of a branch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-25T11:57:01Z","receivedAt":"2022-02-25T11:57:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 24 2022, Junio C Hamano wrote:\n\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\n>> index ebb961be29..f54691bac9 100755\n>> --- a/t/t2060-switch.sh\n>> +++ b/t/t2060-switch.sh\n>> @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' '\n>>  \ttest_must_fail git symbolic-ref HEAD\n>>  '\n>>  \n>> +test_expect_success 'suggestion to detach' '\n>> +\ttest_must_fail git switch main^{commit} 2>stderr &&\n>> +\ttest_i18ngrep \"try again with the --detach option\" stderr\n>> +'\n>> +\n>> +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '\n>> +\ttest_config advice.suggestDetachingHead false &&\n>> +\ttest_must_fail git switch main^{commit} 2>stderr &&\n>> +\ttest_i18ngrep ! \"try again with the --detach option\" stderr\n>> +'\n>\n> OK, we try to be consistent with other tests in the file, and leave\n> s/test_i18n// to a file-wide clean-up outside the topic.\n\nFWIW that's not the case here. This is the first use of test_i18ngrep in\nthis file.\n\nBut better to use test_cmp as noted in\n<220224.86sfs8abj6.gmgdl@evledraar.gmail.com> in the sid-thread.\n"},{"id":"449609","messageId":"CAMMLpeTSMzx4mFjV-fQtbqo1cPfME_ekRO5Xt5TQaKGj0fmNxQ@mail.gmail.com","threadId":"57468","inReplyTo":"220225.86h78n88gb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] switch: mention the --detach option when dying due to lack of a branch","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-25T17:20:08Z","receivedAt":"2022-02-25T17:20:23Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Fri, Feb 25, 2022 at 4:57 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Thu, Feb 24 2022, Junio C Hamano wrote:\n>\n> > Alex Henrie <alexhenrie24@gmail.com> writes:\n> >> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\n> >> index ebb961be29..f54691bac9 100755\n> >> --- a/t/t2060-switch.sh\n> >> +++ b/t/t2060-switch.sh\n> >> @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' '\n> >>      test_must_fail git symbolic-ref HEAD\n> >>  '\n> >>\n> >> +test_expect_success 'suggestion to detach' '\n> >> +    test_must_fail git switch main^{commit} 2>stderr &&\n> >> +    test_i18ngrep \"try again with the --detach option\" stderr\n> >> +'\n> >> +\n> >> +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '\n> >> +    test_config advice.suggestDetachingHead false &&\n> >> +    test_must_fail git switch main^{commit} 2>stderr &&\n> >> +    test_i18ngrep ! \"try again with the --detach option\" stderr\n> >> +'\n> >\n> > OK, we try to be consistent with other tests in the file, and leave\n> > s/test_i18n// to a file-wide clean-up outside the topic.\n>\n> FWIW that's not the case here. This is the first use of test_i18ngrep in\n> this file.\n>\n> But better to use test_cmp as noted in\n> <220224.86sfs8abj6.gmgdl@evledraar.gmail.com> in the sid-thread.\n\nWhy is test_cmp preferable to grep in tests like this?\n\n-Alex\n"},{"id":"449611","messageId":"xmqq1qzqg8mv.fsf@gitster.g","threadId":"57468","inReplyTo":"220225.86h78n88gb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] switch: mention the --detach option when dying due to lack of a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T17:26:32Z","receivedAt":"2022-02-25T17:26:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>> +\ttest_i18ngrep ! \"try again with the --detach option\" stderr\n>>> +'\n>>\n>> OK, we try to be consistent with other tests in the file, and leave\n>> s/test_i18n// to a file-wide clean-up outside the topic.\n>\n> FWIW that's not the case here. This is the first use of test_i18ngrep in\n> this file.\n\nOh, thanks for pointing it out---I remembered that somebody gave a\nsimilar suggestion and blindly assumed that there are other\ninstances already.  If so, picking, between grep and test_cmp,\nwhichever requires the least amount of boilerplate code is fine by\nme.\n\nThanks.\n"}]}