Re: [PATCH] branch: advise about ref syntax rules

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2024, 18:06 UTC
Message-ID: <xmqq1q8t7roc.fsf@gitster.g>
In-Reply-To: <d275d1d179b90592ddd7b5da2ae4573b3f7a37b7.1709307442.git.code@khaugsbakk.name>

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Notes (series):
>     Hopefully I am using `advice.h` correctly here.

Let's see.

Show 7 quoted lines

> -	if (strbuf_check_branch_ref(ref, name))
> -		die(_("'%s' is not a valid branch name"), name);
> +	if (strbuf_check_branch_ref(ref, name)) {
> +		error(_("'%s' is not a valid branch name"), name);
> +		advise(_("See `man git check-ref-format`"));
> +		exit(1);
> +	}

This will give the message with "hint:" prefix, which is a good starting point.

The message is given unconditionally, without any way to turn it off. For those who ...

> git-branch(1) will error out if you give it a bad ref name. But the user
> might not understand why or what part of the name is illegal.

... do not understand why, it is helpful, but once they learned, it is one extra line of unwanted text. If you want to give it a way to squelch, see the comment before where enum advice_type is declared in advice.h header file. The callsites would become something like

	advise_if_enabled(ADVICE_VALID_REF_NAME,
		_("See `man git check-ref-format` for valid refname syntax."));

Another thing is that rewriting die() into error() + advice() + manual exit() is an anti-pattern these days.

	int code = die_message(_("'%s' is not a valid branch name"), name);
	advice_if_enabled(...); /* see above */
	exit(code);

In the same source file, you will find an existing example to mimic.

Thanks.