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

[Karsten Blees <karsten.blees@gmail.com>]

Am 24.06.2014 14:06, schrieb Tanay Abhra:
> On 6/23/2014 5:25 PM, Matthieu Moy wrote:
>> 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;
> +}
> 

Returning a list that is 'half-owned' by the caller (i.e. list is, values are not) is kindof strange. Especially weird is that the caller needs to string_list_clear() _and_ free() the list. Why don't you store NULL values in the string_list in the cache instead of doing this flag magic? I.e.

  static int config_cache_add_value(const char *key, const char *value)
  {
  ...
    if (value == NULL)
      string_list_append_nodup(&e->value_list, NULL);
    else
      string_list_append(&e->value_list, value);

or even

    string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);