Re: [PATCH v2 02/12] fmt-merge-msg: introduce a way to override the main branch name

[Jeff King <peff@peff.net>]

On Mon, Jun 15, 2020 at 12:50:06PM +0000, Johannes Schindelin via GitGitGadget wrote:

> +char *repo_main_branch_name(struct repository *r)
> +{
> +	const char *config_key = "core.mainbranch";
> +	const char *config_display_key = "core.mainBranch";
> +	const char *fall_back = "master";
> +	char *name = NULL, *ret;
> +
> +	if (repo_config_get_string(r, config_key, &name) < 0)
> +		die(_("could not retrieve `%s`"), config_display_key);
> +
> +	ret = name ? name : xstrdup(fall_back);
> +
> +	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
> +		die(_("invalid branch name: %s = %s"),
> +		    config_display_key, name);

Ah, this fixes the "we do not check the format of the short name" issue
I pointed out in v1 (sorry, I just realized that v2 existed so I'll
resume reviewing from there; I do still think this might make life
easier for callers by returning a const pointer).

I'm not sure if this check_refname_format() is valid, though. IIRC we've
had issues where "ONELEVEL" was used to check a branch name, but misses
some cases. The more full check done by strbuf_check_branch_ref()
actually creates the full refname and checks that. It also catches stuff
like refs/heads/HEAD.

I doubt that it matters too much for us to be completely thorough here
(unlike some other spots, we are not enforcing rules against potentially
malicious names, but rather just helping the user realize early that
their config is bogus). So I'm not sure how careful we want to be.

-Peff