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

[Tanay Abhra <tanayabh@gmail.com>]

On 8/1/2014 12:11 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> 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.