Re: [PATCH v12 1/2] add `config_set` API for caching config-like files

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2014, 18:41 UTC
Message-ID: <xmqqwqb28uqg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <53D120FE.8000809@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 21 quoted lines

> +int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
> +{
> +	const char *value;
> +	if (!git_configset_get_value(cs, key, &value))
> +		return git_config_string(dest, key, value);
> +	else
> +		return 1;
> +}
> +
> +int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
> +{
> +	const char *value;
> +	if (!git_configset_get_value(cs, key, &value)) {
> +		if (!value)
> +			return config_error_nonbool(key);
> +		*dest = xstrdup(value);
> +		return 0;
> +	}
> +	else
> +		return 1;
> +}

Hmm, do we really need duplicate and an almost identical implementations?

I would have expected either one of these two (expecting the latter more than the former from the discussion):

 1. Because git_configset_get_string_const() returns a const string
    not to be touched by the caller, it does not even xstrdup(),
    while git_configset_get_string() does.

 2. These behave identically, and the only reason we have two is
    because some callers want to receive the string in "char *"
    while others want to use "const char *".

If you were going route #1, then you would need duplicate but subtly different implementations; get_string_const() variant would not be calling git_config_string() because it does xstrdup() the value and give the caller a new string the caller has to free.

If you were going route #2, the I would have expected there is only one implementation, _get_string(), and _get_string_const() would call it while casting the dest parameter it receives.