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

On 7/29/2014 6:10 PM, Matthieu Moy wrote:

Tanay Abhra [off-list ref] 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.

quoted

+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.

quoted

 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.