On 7/29/2014 6:10 PM, Matthieu Moy wrote:
> Tanay Abhra <tanay...@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.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to