Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> On 7/31/2014 11:39 PM, Matthieu Moy wrote:
>>
>>> This is the part I find weird. You're calling git_die_config_exact() on
>>> the first boggus value, and git_die_config_exact() will notify an error
>>> at the line of the last boggus value.
>>
>> Hmn, we may have some confusion here. I meant the implementation of
>> git_config_exact() to look like this,
>>
>> void git_die_config_exact(const char *key, const char *value)
>> {
>> 	int i;
>> 	const struct string_list *values;
>> 	struct key_value_info *kv_info;
>> 	values = git_config_get_value_multi(key);
>> 	for (i = 0; i < values.nr; i++) {
>> 		kv_info = values->items[i].util;
>> 		/* A null check will be here also */
>> 		if (!strcmp(value, values->items[i].string)) {
>> 			if (!kv_info->linenr)
>> 				die(_("unable to parse '%s' from command-line config"), key);
>> 			else
>> 				die(_("bad config variable '%s' at file line %d in %s"),
>> 					key,
>> 					kv_info->linenr,
>> 					kv_info->filename);
>> 		}
>> 	}
>> }
>>
>> The above code would print the info on first bogus value.
>
> OK, I got confused because git_die_config() errors out at the first
> error. So, your version works, but I do not see any added value in this
> extra complexity.

Hmm, I am still confused ;-)

Can there be more than one 'i' whose value->items[i].string is the
same as the given value?  IOW, if you have "[user] name<EOL>" in
both .git/config and ~/.gitconfig, don't we want to make sure that
we complain on the one in .git/config, which would have taken
precedence if it were spelled correctly?

Your version below makes it very clear that you only complain on the
last one, no matter how many identical copies of the value for the
given key are defined incorrectly the same way, which is easier to
understand what is going on.

> To be cleary, my version would be
>
> NORETURN static /* not sure about the "static" */
> void git_die_config_linenr(const char *key,
>                            const char *filename, int linenr)
> {
> 	if (!linenr)
> 		die(_("unable to parse '%s' from command-line config"), key);
> 	else
> 		die(_("bad config variable '%s' at file line %d in %s"),
> 			key,
> 			linenr,
> 			filename);
> }
>
> (hmm, I actually do not need the value, it's not printed)
>
> NORETURN
> void git_die_config(const char *key)
> {
> 	const struct string_list *values;
> 	struct key_value_info *kv_info;
> 	values = git_config_get_value_multi(key);
> 	kv_info = values->items[values->nr - 1].util;
> 	git_die_config_linenr(key, kv_info->linenr, kv_info->filename);
> }
>
> (we forgot the NORETURN in previous version BTW. It should be there in
> both functions here and in the .h file)
>
> In my version, git_die_config uses git_die_config_linenr, and there's no
> issue with first/last boggus value. Callers of git_die_config_linenr
> have all the information to call it directly.
>
> There are 3 code path that leads to the final die() calls, and having
> this little helper allows making sure the messages are the same for
> every callers, by construction (as opposed to cut-and-pasting).
>
> Really, I don't see any reason to do anything more complex.