Re: [PATCH v4 3/6] rewrite git_config() to use the config-set API

[Tanay Abhra <tanayabh@gmail.com>]

On 7/29/2014 6:10 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
> configset_iter unconditionnally returns 0 (or it dies). Since it is more
> or less the equivalent of the old git_config(), I understand why we
> never encounter the situation where git_config() would return -1 (syntax
> error, weird permission issue => cannot happen when reading from
> memory).
> 
> But then, do we really want this return value, and not just return void?
>

Sounds sane to me.

>> +static void git_config_check_init(void);
>> +
>> +int git_config(config_fn_t fn, void *data)
>> +{
>> +	git_config_check_init();
>> +	return configset_iter(&the_config_set, fn, data);
>> +}
> 
> Here too, git_config now unconditionnally returns 0.
> 
> Most callers of git_config already ignore the return value. Actually,
> there's only one exception in branch.c, but git still compiles with
> this:
>

branch.c is in my git_config() rewrite patch so it should not be a problem
in the future even if it was the case.

> 
> So, I think it's time to make it official that git_config() does not
> return an error code, and make it return void. I would do that in a
> patch before the git_config() -> git_config_raw() rewrite.
> 
> My preference would be to get the return value from
> git_config_with_options and die() if it's negative, but I can also live

Doesn't git_config_with_options() only return positive values, we checked it
pretty intensively last time.

> with a solution where the return value from git_config_with_options() is
> ignored. It's the same discussion we already had about the call to
> git_config() in git_config_check_init() actually, but I now think a
> die() statement should be within git_config(), not after, so that every
> callers benefit from it.

The above patch works like that, doesn't it?

> 
> In any case, doing this in a separate patch means the commit message
> (and possibly a comment next to the git_config() call) should explain
> the situation clearly and justify the choice.
>

The choice being not to return a error code for git_config()?
I am pretty much confused by now.

> The current situation looks like someone tried to get good error
> recovery, but the error code is lost in the way between
> git_config_with_options() and the caller of git_config(), without a
> clear justification of why an error code was once returned, nor a
> justification of why it was later ignored.
> 
> So, in summary, my advice (but not the only option) would be: take my
> patch above, add a die() statement and a comment, write a good commit

Where can the die() statement be inserted? Again, I am confused.
Only thing which sounds reasonable to me is to rewrite existing git_config()
as void first. Other than that, it went over my head.

> message and insert this before this patch.
> 
>>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>>  {
>>  	struct config_set_element k;
>> @@ -1268,6 +1296,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>>  {
>>  	struct config_set_element *e;
>>  	struct string_list_item *si;
>> +	struct configset_list_item *l_item;
>>  	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
>>  
>>  	e = configset_find_element(cs, key);
>> @@ -1283,6 +1312,12 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>>  		hashmap_add(&cs->config_hash, e);
>>  	}
>>  	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
>> +
>> +	ALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc);
>> +	l_item = &cs->list.items[cs->list.nr++];
>> +	l_item->e = e;
>> +	l_item->value_index = e->value_list.nr - 1;
> 
> I would spell this
> 
> 	l_item = &cs->list.items[cs->list.nr];
> 	l_item->e = e;
> 	l_item->value_index = e->value_list.nr;
> 	cs->list.nr++;
> 
> to avoid having to wonder why the "- 1" is needed. But I'm OK with the
> current code.
> 

Yup, you are right. Thanks.