On Wed, Feb 23, 2022 at 5:08 PM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> I think another thing that needs updating here is the
> Documentation/config/advise.txt.
>
> I.e. it explicitly says this is advice we emit *after* we've switched to
> a detached head, instructing you how to proceed.
>
> But here you're adding another message guarded by that
> ADVICE_DETACHED_HEAD which doesn't fit that description, but is rather
> suggesting that you may want to use the --detach option.
>
> After you use the --detach option you'll get the existing (and described
> by the docs) ADVICE_DETACHED_HEAD message.
>
> The two things we could do is to adjust the docs and use
> ADVICE_DETACHED_HEAD for both, or to add a new
> ADVICE_SUGGEST_DETACHED_HEAD category with a new
> advice.suggestDetachedHead config.
Of those two options, I'm guessing that creating a new config option
is going to be more acceptable. I'll do that in v2.
> Also, you say "doesn't work", so is this about to die() after this
> advice is printed? What does the complete output look like then? Usually
> we'd emit the "die" message before the advice message
>
> *reads the surrounding code a bit*
>
> Ah, so yes, we'd do that in that order now.
>
> I think we'd like to instead use die_message(), followed by an optional
> advise(), followed by exit(). Maybe like this?:
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index d9b31bbb6d6..22cf9d6ad1b 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1397,23 +1397,32 @@ 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);
> + if (skip_prefix(ref, "refs/tags/", &ref)) {
> + code = die_message(_("a branch is expected, got tag '%s'"),
> + ref);
> + goto out;
> + }
> + if (skip_prefix(ref, "refs/remotes/", &ref)) {
> + code = die_message(_("a branch is expected, got remote branch '%s'"),
> + ref);
> + }
> + code = die_message(_("a branch is expected, got '%s'"), ref);
> + goto out;
> }
> - 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);
> + if (branch_info->commit) {
> + code = die_message(_("a branch is expected, got commit '%s'"),
> + branch_info->name);
> + goto out;
> + }
> + BUG("unreachable with '%s'", branch_info->name);
> +out:
> + /* advice here */
> + exit(code);
> }
>
> static void die_if_some_operation_in_progress(void)
>
> But that's probably a lot less nasty if die_expecting_a_branch() is made
> to call a helper that just does a "return die_message()" with a "return
> 0" fallback, and on "0" we call BUG(...).
The same thing can also be accomplished with "else if" instead of
"goto". I'll move the advice to after the error message in v2.
Thanks for the feedback!
-Alex
