[PATCH] switch: mention the --detach option when dying due to lack of a branch

[PATCH] switch: mention the --detach option when dying due to lack of a branch

[Alex Henrie]

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>
---
 builtin/checkout.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d9b31bbb6d..10a035feed 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1398,6 +1398,10 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)
 	struct object_id oid;
 	char *to_free;
 
+	if (advice_enabled(ADVICE_DETACHED_HEAD))
+		advise(_("The specified commit is not a local branch.\n"
+			 "If you want to enter detached head mode, try again with the --detach option."));
+
 	if (dwim_ref(branch_info->name, strlen(branch_info->name), &oid, &to_free, 0) == 1) {
 		const char *ref = to_free;
 
-- 
2.35.1

Re: [PATCH] switch: mention the --detach option when dying due to lack of a branch

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> 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.

An error message certainly would help over showing

	fatal: a branch is expected, got tag 'abc'

but I have to wonder why we shouldn't DWIM and detach the HEAD at
the commit the user wanted to.  That would also solve the issue of
leaving them wondering why switch is broken and checkout is not, no?

If the advice for detached HEAD state is enabled, then the user will
be reminded that they are not on any branch the usual way when the
HEAD is detached at the named commit.  And if the advice is not
enabled, then they will not be helped by this new advise() message
we add here.

> +	if (advice_enabled(ADVICE_DETACHED_HEAD))
> +		advise(_("The specified commit is not a local branch.\n"
> +			 "If you want to enter detached head mode, try again with the --detach option."));
> +

"detached HEAD" is a state, and not a mode.

s/enter detached head mode/detach HEAD at the commit/ perhaps.

Thanks.

Re: [PATCH] switch: mention the --detach option when dying due to lack of a branch

[Alex Henrie]

On Wed, Feb 23, 2022 at 4:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > 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.
>
> An error message certainly would help over showing
>
>         fatal: a branch is expected, got tag 'abc'
>
> but I have to wonder why we shouldn't DWIM and detach the HEAD at
> the commit the user wanted to.  That would also solve the issue of
> leaving them wondering why switch is broken and checkout is not, no?
>
> If the advice for detached HEAD state is enabled, then the user will
> be reminded that they are not on any branch the usual way when the
> HEAD is detached at the named commit.  And if the advice is not
> enabled, then they will not be helped by this new advise() message
> we add here.

Before commit 7968bef06b "switch: only allow explicit detached HEAD",
`git switch` did not require --detach to enter a detached HEAD state.
The justification in the commit message is worth reading, but I don't
have an opinion on whether or not it was a change for the better.

> > +     if (advice_enabled(ADVICE_DETACHED_HEAD))
> > +             advise(_("The specified commit is not a local branch.\n"
> > +                      "If you want to enter detached head mode, try again with the --detach option."));
> > +
>
> "detached HEAD" is a state, and not a mode.
>
> s/enter detached head mode/detach HEAD at the commit/ perhaps.

Sure. I'll send a v2 tonight.

-Alex

Re: [PATCH] switch: mention the --detach option when dying due to lack of a branch

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 23 2022, Alex Henrie wrote:

> On Wed, Feb 23, 2022 at 4:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> > 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.
>>
>> An error message certainly would help over showing
>>
>>         fatal: a branch is expected, got tag 'abc'
>>
>> but I have to wonder why we shouldn't DWIM and detach the HEAD at
>> the commit the user wanted to.  That would also solve the issue of
>> leaving them wondering why switch is broken and checkout is not, no?
>>
>> If the advice for detached HEAD state is enabled, then the user will
>> be reminded that they are not on any branch the usual way when the
>> HEAD is detached at the named commit.  And if the advice is not
>> enabled, then they will not be helped by this new advise() message
>> we add here.
>
> Before commit 7968bef06b "switch: only allow explicit detached HEAD",
> `git switch` did not require --detach to enter a detached HEAD state.
> The justification in the commit message is worth reading, but I don't
> have an opinion on whether or not it was a change for the better.
>
>> > +     if (advice_enabled(ADVICE_DETACHED_HEAD))
>> > +             advise(_("The specified commit is not a local branch.\n"
>> > +                      "If you want to enter detached head mode, try again with the --detach option."));
>> > +
>>
>> "detached HEAD" is a state, and not a mode.
>>
>> s/enter detached head mode/detach HEAD at the commit/ perhaps.
>
> Sure. I'll send a v2 tonight.

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.

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(...).

Re: [PATCH] switch: mention the --detach option when dying due to lack of a branch

[Alex Henrie]

On Wed, Feb 23, 2022 at 5:08 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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