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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2022, 19:02 UTC
Message-ID: <xmqqv8x4m6jn.fsf@gitster.g>
In-Reply-To: <20220224064710.2252637-1-alexhenrie24@gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

Show 26 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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

Show 18 quoted lines

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