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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/31/2014 10:22 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:
>>>> Tanay Abhra <tanayabh@gmail.com> writes:
>>>>
>>>>> +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;
>>>>> +	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);
>>>>> + }
>>>>
>>>> Extra whitespace before }.
>>>>
>>>> Also, didn't we agree that it was a good thing to factor this
>>>> if/then/else into a helper function?
>>>>
>>>
>>> I have been thinking about it, wouldn't it be better to give users
>>> a function like,
>>>
>>> git_config_die_exact(key, value);
>>>
>>> where user supplies key & value both and it would print the correct message based
>>> on that.
>> 
>> I suggested git_config_die_linenr(key, linenr), and I now realize it
>> should take the value too.
>> 
>> You're suggesting git_config_die_exact(key, value). Is it a typo that
>> you forgot the line number, or is it intentional? If intentional, I
>> don't think it solves your issue:
>> 
>> [section]
>>    key
>>    key
>>
>> There are two errors in this file, and you need to provide a line
>> number. key and value alone do not allow you to know which line the
>> error is. You can use a convention like "complain on the first value
>> equal to the argument", but I'm not sure that would always work. And
>> that seems backward to me to reconstruct the line number since the
>> function can be called from places where the line number is already
>> known (while iterating over the string_list for example).
>
> Still the user would have to know that the linenr info is there.
> First hear my argument, then we can go either way.
>
> Let's first see the previous code behavior, git_config() would die on the
> first corrupt value, we wouldn't live to see the future value.
>
> for example,
>
> [section]
> 	key	// error(old git_config() would die here)
> 	key = good_value
>
> [section]
> 	key	//again error
>
> Now for the new behavior,
>
> single valued callers use git_config_get_value() which will directly
> supply the last value, so we don't see the first error value.
> For such cases, git_die_config(key) is enough.

Yes. And it is better than the old behavior which was dying on the
erroneous value without giving a chance to the user to override the
boggus value in a more specific config file (e.g. if your sysadmin
messed-up /etc/gitconfig).

But since git_die_config(key) is simpler to use for the caller, it's
independant from git_die_config_exact()'s parameters.

> The new git_config() works exactly as the old code, it would die
> on the first case of erroneous value. Here, git_die_config_exact(key, value)
> would be enough.

Yes. But this callsite has the line number information, so it could use
it.

> The last case is git_config_get_value_multi(), here we iterate over the keys,
> and then call git_die_config_exact(key, value) for the erroneous value.
> (pros and cons: abstracts the error message implementation from the user
> but there is an extra call to git_config_get_value_multi(), but its cheap and
> we are dying anyway)

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.

I agree it works (if we give only one error message, it can be the first
or the last, the user doesn't really care), but I find the
implementation backwards. You have the line number already, as you are
iterating over the string_list which contain it. So why forget the line
number, and recompute one, possibly different, right after?

So, I see only cases where you already have the line number, hence no
reason to recompute it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/