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

[PATCH v2] 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>
---
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(-)

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 &&
-- 
2.35.1

Re: [PATCH v2] 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:

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

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Alex Henrie]

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

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

[Junio C Hamano]

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