Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jun 24, 2014, 12:06 UTC
Message-ID: <53A969DD.2030201@gmail.com>
In-Reply-To: <vpqtx7bn8ln.fsf@anie.imag.fr>

On 6/23/2014 5:25 PM, Matthieu Moy wrote:

Show 25 quoted lines

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +/* for NULL values, 'util' for each `string_list_item` is flagged as 1 */
>
> It's a void *, so just saying that it is flagged as 1 does not say how
> it's encoded. How about "... is an 'int *' pointing to the value 1".
>
> And actually, you can save one malloc by encoding the value directly in
> the util pointer itself like
>
> #define NULL_FLAG (void *)1
>
> 	item->util = NULL_FLAG;
>
> I find this a bit ugly, but I think Git already uses this in a few
> places (git grep 'void \*) *1' for examples).
>
> Or you can use a pointer to a single static value:
>
> static const int boolean_null_flag = 1; // Or even without = 1.
>
> and then
>
> 	item->util = &boolean_null_flag;
>

Thanks for the review. I will change the flag format to what you have suggested. Instead of giving the users of the API the headache of thinking about the flag, let me give you an alternative,

+const struct string_list *git_config_get_string_multi(const char *key) +{ + int i, *flag; + struct string_list *temp = config_cache_get_value(key); + if (!temp) + return NULL; + /* create a NODUP string list which can have NULL values */ + struct string_list *l = xcalloc(1, sizeof(*l)); + for(i=0; i < temp->nr; i++) { + flag = temp->items[i].util; + if (*flag) + string_list_append(l, NULL); + else + string_list_append(l, temp->items[i].string); + } + return l; +}

Now the only headache for the user will be to free the string-list once he is done with it. What do you think about this approach?

Also I have a doubt, when rewriting git_config() callers I saw a curious case, when at the end of the callback they sometimes call git_default_config().

For example in add.c,

static int add_config(const char *var, const char *value, void *cb)
{
	if (!strcmp(var, "add.ignoreerrors") ||
	    !strcmp(var, "add.ignore-errors")) {
		ignore_add_errors = git_config_bool(var, value);
		return 0;
	}
	return git_default_config(var, value, cb);
}

What should I do about this, should I change the default_config() calls to git_config_get_string calls which I can easily do, if the values it is setting are not being set by any other callers.

For example in config.c,

static int git_default_core_config(const char *var, const char *value)
{
	/* This needs a better name */
	if (!strcmp(var, "core.filemode")) {
		trust_executable_bit = git_config_bool(var, value);
		return 0;
	}
	if (!strcmp(var, "core.trustctime")) {
		trust_ctime = git_config_bool(var, value);
		return 0;
	}
	if (!strcmp(var, "core.statinfo") ||
	    !strcmp(var, "core.checkstat")) {
--[snip]
variables like "core.filemode" or "core.statinfo" have been requested 
here only in the whole codebase, so I am safe to rewrite this function
with git_config_get_string, am I?

If not many callers (add_config...) behave like this. What should I do with them?

Also, what are your views for the points Ramsay has raised.

Cheers, Tanay Abhra.