| Message ID | 20220224064710.2252637-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] switch: mention the --detach option when dying due to lack of a branch | expand |
On Wed, Feb 23 2022, Alex Henrie wrote: > Users who are accustomed to doing `git checkout <tag>` assume that > `git switch <tag>` will do the same thing. Inform them of the --detach > option so they aren't left wondering why `git switch` doesn't work but > `git checkout` does. Thanks, this looks good to me! FWIW while testing this v2 I came up with this on top, which you may or may not want (some unrelated changes): 1. We had a to_free but didn't free it, now we do. Didn't matter with SANITIZE=leak, but FWIW valgrind with "valgrind --leak-check=full --show-leak-kinds=all" is marginally happier 2. Maybe s/code = /return / with a helper is easier to read, maybe not. 3. That #2 makes the code/advice wrapper simpler (also with the to_free) 4. Used test_cmp in the test to check the actual output we got, and added a missing test for the "tag" case. In any case s/test_i18ngrep/grep/ is the right thing nowadays for new code. diff --git a/builtin/checkout.c b/builtin/checkout.c index 9244827ca02..9e4d03343fa 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1393,34 +1393,39 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts) return status; } -static void die_expecting_a_branch(const struct branch_info *branch_info) +static int die_expecting_a_branch_message(const struct branch_info *branch_info, char **to_free) { struct object_id oid; - char *to_free; - int code; - if (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) { - const char *ref = to_free; + if (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, to_free, 0) == 1) { + const char *ref = *to_free; if (skip_prefix(ref, "refs/tags/", &ref)) - code = die_message(_("a branch is expected, got tag '%s'"), ref); + return die_message(_("a branch is expected, got tag '%s'"), ref); else if (skip_prefix(ref, "refs/remotes/", &ref)) - code = die_message(_("a branch is expected, got remote branch '%s'"), ref); + return die_message(_("a branch is expected, got remote branch '%s'"), ref); else - code = die_message(_("a branch is expected, got '%s'"), ref); + return die_message(_("a branch is expected, got '%s'"), ref); } else if (branch_info->commit) - code = die_message(_("a branch is expected, got commit '%s'"), branch_info->name); + return die_message(_("a branch is expected, got commit '%s'"), branch_info->name); else /* * This case should never happen because we already die() on * non-commit, but just in case. */ - code = die_message(_("a branch is expected, got '%s'"), branch_info->name); + return die_message(_("a branch is expected, got '%s'"), branch_info->name); +} + +static void die_expecting_a_branch(const struct branch_info *branch_info) +{ + char *to_free = NULL; + int code = die_expecting_a_branch_message(branch_info, &to_free); if (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD)) advise(_("If you want to detach HEAD at the commit, try again with the --detach option.")); + free(to_free); exit(code); } diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh index f54691bac9f..0708359ee24 100755 --- a/t/t2060-switch.sh +++ b/t/t2060-switch.sh @@ -32,15 +32,31 @@ test_expect_success 'switch and detach' ' test_must_fail git symbolic-ref HEAD ' -test_expect_success 'suggestion to detach' ' - test_must_fail git switch main^{commit} 2>stderr && - test_i18ngrep "try again with the --detach option" stderr +test_expect_success 'suggestion to detach commit' ' + test_must_fail git switch main^{commit} 2>actual && + cat >expect <<-\EOF && + fatal: a branch is expected, got commit '\''main^{commit}'\'' + hint: If you want to detach HEAD at the commit, try again with the --detach option. + EOF + test_cmp expect actual +' + +test_expect_success 'suggestion to detach tag' ' + test_must_fail git switch first 2>actual && + cat >expect <<-\EOF && + fatal: a branch is expected, got tag '\''first'\'' + hint: If you want to detach HEAD at the commit, try again with the --detach option. + EOF + test_cmp expect actual ' test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' ' test_config advice.suggestDetachingHead false && - test_must_fail git switch main^{commit} 2>stderr && - test_i18ngrep ! "try again with the --detach option" stderr + test_must_fail git switch main^{commit} 2>actual && + cat >expect <<-\EOF && + fatal: a branch is expected, got commit '\''main^{commit}'\'' + EOF + test_cmp expect actual ' test_expect_success 'switch and detach current branch' '
Alex Henrie <alexhenrie24@gmail.com> writes: > diff --git a/builtin/checkout.c b/builtin/checkout.c > index d9b31bbb6d..9244827ca0 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1397,23 +1397,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info) > { > struct object_id oid; > char *to_free; > + int code; > > if (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) { > const char *ref = to_free; > > if (skip_prefix(ref, "refs/tags/", &ref)) > - die(_("a branch is expected, got tag '%s'"), ref); > - if (skip_prefix(ref, "refs/remotes/", &ref)) > - die(_("a branch is expected, got remote branch '%s'"), ref); > - die(_("a branch is expected, got '%s'"), ref); > + code = die_message(_("a branch is expected, got tag '%s'"), ref); > + else if (skip_prefix(ref, "refs/remotes/", &ref)) > + code = die_message(_("a branch is expected, got remote branch '%s'"), ref); > + else > + code = die_message(_("a branch is expected, got '%s'"), ref); > } > - if (branch_info->commit) > - die(_("a branch is expected, got commit '%s'"), branch_info->name); In the original code, when dwim_ref() said there is a unique hit, we died with varying messages. So it was OK to have this check not as a part of if/elseif/... cascade. > - /* > - * This case should never happen because we already die() on > - * non-commit, but just in case. > - */ > - die(_("a branch is expected, got '%s'"), branch_info->name); > + else if (branch_info->commit) But now, new code only sets code without dying, so we avoid overwriting the code (and calling die_message() twice) by turning it "else if". OK. > + code = die_message(_("a branch is expected, got commit '%s'"), branch_info->name); > + else > + /* > + * This case should never happen because we already die() on > + * non-commit, but just in case. > + */ > + code = die_message(_("a branch is expected, got '%s'"), branch_info->name); OK. So "code" gets assigned exactly once in the above if/elseif/... cascade. Not defining the variable with initialization at the beginning of this function is correct. > + if (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD)) > + advise(_("If you want to detach HEAD at the commit, try again with the --detach option.")); > + > + exit(code); > } OK. > diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh > index ebb961be29..f54691bac9 100755 > --- a/t/t2060-switch.sh > +++ b/t/t2060-switch.sh > @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' ' > test_must_fail git symbolic-ref HEAD > ' > > +test_expect_success 'suggestion to detach' ' > + test_must_fail git switch main^{commit} 2>stderr && > + test_i18ngrep "try again with the --detach option" stderr > +' > + > +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' ' > + test_config advice.suggestDetachingHead false && > + test_must_fail git switch main^{commit} 2>stderr && > + test_i18ngrep ! "try again with the --detach option" stderr > +' OK, we try to be consistent with other tests in the file, and leave s/test_i18n// to a file-wide clean-up outside the topic. > test_expect_success 'switch and detach current branch' ' > test_when_finished git switch main && > git switch main &&
On Thu, Feb 24 2022, Junio C Hamano wrote: > Alex Henrie <alexhenrie24@gmail.com> writes: >> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh >> index ebb961be29..f54691bac9 100755 >> --- a/t/t2060-switch.sh >> +++ b/t/t2060-switch.sh >> @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' ' >> test_must_fail git symbolic-ref HEAD >> ' >> >> +test_expect_success 'suggestion to detach' ' >> + test_must_fail git switch main^{commit} 2>stderr && >> + test_i18ngrep "try again with the --detach option" stderr >> +' >> + >> +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' ' >> + test_config advice.suggestDetachingHead false && >> + test_must_fail git switch main^{commit} 2>stderr && >> + test_i18ngrep ! "try again with the --detach option" stderr >> +' > > OK, we try to be consistent with other tests in the file, and leave > s/test_i18n// to a file-wide clean-up outside the topic. FWIW that's not the case here. This is the first use of test_i18ngrep in this file. But better to use test_cmp as noted in <220224.86sfs8abj6.gmgdl@evledraar.gmail.com> in the sid-thread.
On Fri, Feb 25, 2022 at 4:57 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Thu, Feb 24 2022, Junio C Hamano wrote: > > > Alex Henrie <alexhenrie24@gmail.com> writes: > >> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh > >> index ebb961be29..f54691bac9 100755 > >> --- a/t/t2060-switch.sh > >> +++ b/t/t2060-switch.sh > >> @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' ' > >> test_must_fail git symbolic-ref HEAD > >> ' > >> > >> +test_expect_success 'suggestion to detach' ' > >> + test_must_fail git switch main^{commit} 2>stderr && > >> + test_i18ngrep "try again with the --detach option" stderr > >> +' > >> + > >> +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' ' > >> + test_config advice.suggestDetachingHead false && > >> + test_must_fail git switch main^{commit} 2>stderr && > >> + test_i18ngrep ! "try again with the --detach option" stderr > >> +' > > > > OK, we try to be consistent with other tests in the file, and leave > > s/test_i18n// to a file-wide clean-up outside the topic. > > FWIW that's not the case here. This is the first use of test_i18ngrep in > this file. > > But better to use test_cmp as noted in > <220224.86sfs8abj6.gmgdl@evledraar.gmail.com> in the sid-thread. Why is test_cmp preferable to grep in tests like this? -Alex
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> + test_i18ngrep ! "try again with the --detach option" stderr >>> +' >> >> OK, we try to be consistent with other tests in the file, and leave >> s/test_i18n// to a file-wide clean-up outside the topic. > > FWIW that's not the case here. This is the first use of test_i18ngrep in > this file. Oh, thanks for pointing it out---I remembered that somebody gave a similar suggestion and blindly assumed that there are other instances already. If so, picking, between grep and test_cmp, whichever requires the least amount of boilerplate code is fine by me. Thanks.
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index adee26fbbb..c40eb09cb7 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -85,6 +85,9 @@ advice.*:: linkgit:git-switch[1] or linkgit:git-checkout[1] to move to the detach HEAD state, to instruct how to create a local branch after the fact. + suggestDetachingHead:: + Advice shown when linkgit:git-switch[1] refuses to detach HEAD + without the explicit `--detach` option. checkoutAmbiguousRemoteBranchName:: Advice shown when the argument to linkgit:git-checkout[1] and linkgit:git-switch[1] diff --git a/advice.c b/advice.c index e00d30254c..2e1fd48304 100644 --- a/advice.c +++ b/advice.c @@ -42,6 +42,7 @@ static struct { [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 }, [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 }, [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 }, + [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 }, [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 }, [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 }, [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 }, diff --git a/advice.h b/advice.h index a7521d6087..a3957123a1 100644 --- a/advice.h +++ b/advice.h @@ -20,6 +20,7 @@ struct string_list; ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME, ADVICE_COMMIT_BEFORE_MERGE, ADVICE_DETACHED_HEAD, + ADVICE_SUGGEST_DETACHING_HEAD, ADVICE_FETCH_SHOW_FORCED_UPDATES, ADVICE_GRAFT_FILE_DEPRECATED, ADVICE_IGNORED_HOOK, diff --git a/builtin/checkout.c b/builtin/checkout.c index d9b31bbb6d..9244827ca0 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1397,23 +1397,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info) { struct object_id oid; char *to_free; + int code; if (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) { const char *ref = to_free; if (skip_prefix(ref, "refs/tags/", &ref)) - die(_("a branch is expected, got tag '%s'"), ref); - if (skip_prefix(ref, "refs/remotes/", &ref)) - die(_("a branch is expected, got remote branch '%s'"), ref); - die(_("a branch is expected, got '%s'"), ref); + code = die_message(_("a branch is expected, got tag '%s'"), ref); + else if (skip_prefix(ref, "refs/remotes/", &ref)) + code = die_message(_("a branch is expected, got remote branch '%s'"), ref); + else + code = die_message(_("a branch is expected, got '%s'"), ref); } - if (branch_info->commit) - die(_("a branch is expected, got commit '%s'"), branch_info->name); - /* - * This case should never happen because we already die() on - * non-commit, but just in case. - */ - die(_("a branch is expected, got '%s'"), branch_info->name); + else if (branch_info->commit) + code = die_message(_("a branch is expected, got commit '%s'"), branch_info->name); + else + /* + * This case should never happen because we already die() on + * non-commit, but just in case. + */ + code = die_message(_("a branch is expected, got '%s'"), branch_info->name); + + if (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD)) + advise(_("If you want to detach HEAD at the commit, try again with the --detach option.")); + + exit(code); } static void die_if_some_operation_in_progress(void) diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh index ebb961be29..f54691bac9 100755 --- a/t/t2060-switch.sh +++ b/t/t2060-switch.sh @@ -32,6 +32,17 @@ test_expect_success 'switch and detach' ' test_must_fail git symbolic-ref HEAD ' +test_expect_success 'suggestion to detach' ' + test_must_fail git switch main^{commit} 2>stderr && + test_i18ngrep "try again with the --detach option" stderr +' + +test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' ' + test_config advice.suggestDetachingHead false && + test_must_fail git switch main^{commit} 2>stderr && + test_i18ngrep ! "try again with the --detach option" stderr +' + test_expect_success 'switch and detach current branch' ' test_when_finished git switch main && git switch main &&
Users who are accustomed to doing `git checkout <tag>` assume that `git switch <tag>` will do the same thing. Inform them of the --detach option so they aren't left wondering why `git switch` doesn't work but `git checkout` does. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- v2: - Rephrase advice - Print advice after error message - Create a new config variable to suppress the advice - Add tests --- Documentation/config/advice.txt | 3 +++ advice.c | 1 + advice.h | 1 + builtin/checkout.c | 30 +++++++++++++++++++----------- t/t2060-switch.sh | 11 +++++++++++ 5 files changed, 35 insertions(+), 11 deletions(-)