From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Wed, 09 Dec 2020 13:12:01 GMT
Subject: Re: [PATCH v4 4/6] config: extract function to parse config pairs
Message-ID: <875z5bxgwu.fsf@evledraar.gmail.com>
In-Reply-To: <63fb8ad99742d748dc00306be6d2e05bd0ed583d.1607514692.git.ps@pks.im>


On Wed, Dec 09 2020, Patrick Steinhardt wrote:

> +static int config_parse_pair(const char *key, const char *value,
> +			  config_fn_t fn, void *data)
> +{
> +	char *canonical_name;
> +	int ret;
> +
> +	if (!strlen(key))
> +		return error(_("empty config key"));

We just did this check (just before the context of the second hunk in
this patch) before calling this function:

    if (!pair[0]->len) {
        strbuf_list_free(pair);
        return error(_("bogus config parameter: %s"), text);

I think just removing this is best, for such a closely coupled static
function we can just rely on the sanity of the caller, but it should at
least be:

    if (!strlen(key))
        BUG("clear that it's unreachable, and a translator won't bother with it, like _()...");

Aside: It's more C-idiom-y in this case to write:

    if (!*key)


> +	if (git_config_parse_key(key, &canonical_name, NULL))
> +		return -1;
> +
> +	ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
> +	free(canonical_name);
> +	return ret;
> +}
> +
>  int git_config_parse_parameter(const char *text,
>  			       config_fn_t fn, void *data)
>  {
>  	const char *value;
> -	char *canonical_name;
>  	struct strbuf **pair;
>  	int ret;
>  
> @@ -483,12 +498,7 @@ int git_config_parse_parameter(const char *text,
>  		return error(_("bogus config parameter: %s"), text);
>  	}
>  
> -	if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL)) {
> -		ret = -1;
> -	} else {
> -		ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
> -		free(canonical_name);
> -	}
> +	ret = config_parse_pair(pair[0]->buf, value, fn, data);
>  	strbuf_list_free(pair);
>  	return ret;
>  }