Alex Henrie <alexhenrie24@xxxxxxxxx> 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 &&
