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

[Tanay Abhra <tanayabh@gmail.com>]

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.

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.

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)

+	if (load_config_refs) {
+		values = git_config_get_value_multi("notes.displayref");
+		if (values) {
+			for (i = 0; i < values->nr; i++) {
+				if (!values->items[i].string) {
+					config_error_nonbool("notes.displayref");
+					git_die_config_exact("notes.displayref", values->items[i].string);
+				}
+				else
+					string_list_add_refs_by_glob(&display_notes_refs,
+								     values->items[i].string);
+			}
+		}
+	}

What do you think?