From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Tanay Abhra <tanayabh@gmail•com>
Cc: git@vger•kernel.org, Ramkumar Ramachandra <artagnon@gmail•com>
Subject: Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API
Date: Thu, 31 Jul 2014 20:09:09 +0200 [thread overview]
Message-ID: <vpqa97p8koq.fsf@anie.imag.fr> (raw)
In-Reply-To: <53DA7C23.3090603@gmail.com> (Tanay Abhra's message of "Thu, 31 Jul 2014 22:55:55 +0530")
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/
next prev parent reply other threads:[~2014-07-31 18:09 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-31 15:47 [PATCH v6 0/7] Rewrite `git_config()` using config-set API Tanay Abhra
2014-07-31 15:47 ` [PATCH v6 1/7] config.c: fix accuracy of line number in errors Tanay Abhra
2014-07-31 15:47 ` [PATCH v6 2/7] add line number and file name info to `config_set` Tanay Abhra
2014-07-31 15:47 ` [PATCH v6 3/7] change `git_config()` return value to void Tanay Abhra
2014-07-31 15:47 ` [PATCH v6 4/7] rewrite git_config() to use the config-set API Tanay Abhra
2014-07-31 15:47 ` [PATCH v6 5/7] add a test for semantic errors in config files Tanay Abhra
2014-07-31 15:47 ` [PATCH v6 6/7] config: add `git_die_config()` to the config-set API Tanay Abhra
2014-07-31 15:55 ` Matthieu Moy
2014-07-31 16:44 ` Tanay Abhra
2014-07-31 16:52 ` Matthieu Moy
2014-07-31 17:25 ` Tanay Abhra
2014-07-31 18:09 ` Matthieu Moy [this message]
2014-07-31 18:26 ` Tanay Abhra
2014-07-31 18:41 ` Matthieu Moy
2014-07-31 21:17 ` Junio C Hamano
2014-08-01 8:33 ` Matthieu Moy
2014-08-01 9:04 ` Tanay Abhra
2014-08-01 9:22 ` Matthieu Moy
2014-07-31 15:47 ` [PATCH v6 7/7] add tests for `git_config_get_string_const()` Tanay Abhra
2014-07-31 15:56 ` [PATCH v6 0/7] Rewrite `git_config()` using config-set API Matthieu Moy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqa97p8koq.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=artagnon@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=tanayabh@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox