Tanay Abhra <tanay...@gmail.com> writes:

> On 7/31/2014 10:22 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanay...@gmail.com> writes:
>>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:
>>>> Tanay Abhra <tanay...@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

> 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
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to