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
