On 8/1/2014 12:11 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@xxxxxxxxx> writes:
>
>> On 7/31/2014 11:39 PM, Matthieu Moy wrote:
>>
>>> 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.
>
> OK, I got confused because git_die_config() errors out at the first
> error. So, your version works, but I do not see any added value in this
> extra complexity.
>
> To be cleary, my version would be
>
> NORETURN static /* not sure about the "static" */
> void git_die_config_linenr(const char *key,
> const char *filename, int linenr)
> {
> if (!linenr)
> die(_("unable to parse '%s' from command-line config"), key);
> else
> die(_("bad config variable '%s' at file line %d in %s"),
> key,
> linenr,
> filename);
> }
>
> (hmm, I actually do not need the value, it's not printed)
>
> NORETURN
> 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;
> git_die_config_linenr(key, kv_info->linenr, kv_info->filename);
> }
>
> (we forgot the NORETURN in previous version BTW. It should be there in
> both functions here and in the .h file)
>
> In my version, git_die_config uses git_die_config_linenr, and there's no
> issue with first/last boggus value. Callers of git_die_config_linenr
> have all the information to call it directly.
>
> There are 3 code path that leads to the final die() calls, and having
> this little helper allows making sure the messages are the same for
> every callers, by construction (as opposed to cut-and-pasting).
>
> Really, I don't see any reason to do anything more complex.
>
Thanks, I am sending your version with the reroll.
Also, for clarity the multi value use case would look like,
struct key_value_info *kv_info;
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");
kv_info = values->items[i].util;
git_die_config_linenr("notes.displayref",
kv_info->filename,
kv_info->linenr);
}
else
string_list_add_refs_by_glob(&display_notes_refs,
values->items[i].string);
}
}
}
with my function it would have looked like,
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);
}
}
}
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html