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

[Tanay Abhra <tanayabh@gmail.com>]

On 7/31/2014 11:39 PM, Matthieu Moy wrote:
> 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.
>

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.
I am only rooting for it because the caller has to think very little to use it.
It's your call, I am open to both ideas.

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