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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2014, 15:46 UTC
Message-ID: <xmqqoawqwrov.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1405434571-25459-2-git-send-email-tanayabh@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 10 quoted lines

> diff --git a/config.c b/config.c
> index ba882a1..89e2d67 100644
> --- a/config.c
> +++ b/config.c
> ...
> +const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
> +{
> +	struct config_set_element *e = configset_find_element(cs, key);
> +	return e ? &e->value_list : NULL;
> +}

Somehow I find the placement of this function out of place. Didn't you get the same impression while proofreading the entire file after you are done writing this patch?

The right place for it would probably be immediately before git_configset_get_value(), I would think.

Show 11 quoted lines

> +int git_configset_add_file(struct config_set *cs, const char *filename)
> +{
> +	int ret = 0;
> +	ret = git_config_from_file(config_hash_callback, filename, cs);
> +	if (!ret)
> +		return 0;
> +	else {
> +		git_configset_clear(cs);
> +		return -1;
> +	}
> +}

If I add contents from file "A" successfully and then attempt to further add contents from file "B" which fails, I would lose contents I read from "A"?

It would not be worth trying to make it transactional (i.e. making sure cs contains what was there before the config-from-file was called if it failed), so in such a case we may end up seeing a mixture of full contents from A and partial contents from B, which is just as useless as a cleared configset, so it is not wrong to clear it per-se. An alternative might be to return without clearing, as we are leaving the configset in a useless state either way and give the caller a choice between clearing it and continue, and dying without even spending unnecessary cycles to clear. That might be more consistent with what git_config_check_init() does, where you die without bothering to clear what was half-read into the configset.

Whatever behaviour is chosen in the error codepath, it needs to be documented.

Thanks.