On 8/4/2014 11:37 PM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@xxxxxxxxx> writes:
>
>> Add `git_die_config` that dies printing the line number and the file name
>> of the highest priority value for the configuration variable `key`.
>>
>> It has usage in non-callback based config value retrieval where we can
>> raise an error and die if there is a semantic error.
>> For example,
>>
>> if (!git_config_get_value(key, &value)) {
>> /* NULL values not allowed */
>> if (!value)
>> git_config_die(key);
>> else
>> /* do work */
>> }
>
> It feels a bit unnatural at the API level that this does not take
> 'value'; I do understand that it is not a big deal in the error code
> path to locate again the value from the configuration using the key,
> but still.
>
But, we don't have a use for "value" as it is not denoted in the error
string, that is why I left it out.
> It feels even more unnatural that the caller cannot say _how_ it
> finds the value offending by not taking any message. For one
> particular callchain, e.g. git_config_get_string() that eventually
> calls git_config_string() which will show an error message via
> config_error_nonbool(), you may not want any extra message, but for
> new callers that wants to make sure value falls within a supported
> range, this forces it to write
>
> if (!git_config_get_int(key, &num)) {
> if (!(0 < num && num < 4)) {
> error("'%s' must be between 1 and 3");
> git_config_die(key);
> }
> /* otherwise work */
> }
>
> and then the error message would say something like:
>
> error: 'core.frotz' must be between 1 and 3
> fatal: bad config variable 'core.frotz' at file line 15 in .git/config
>
> which sounds somewhat backwards, at least to me.
>
I was aping the old git_config() system, it also does exactly what you described
above. for example, builtin/gc.c line 91,
if (!strcmp(var, "gc.pruneexpire")) {
if (value && strcmp(value, "now")) {
unsigned long now = approxidate("now");
if (approxidate(value) >= now)
return error(_("Invalid %s: '%s'"), var, value);
}
would print,
error: Invalid gc.pruneexpire: 'value'
fatal: bad config variable 'gc.pruneexpire' at file line 15 in .git/config
or imap-send.c line 1340,
if (!strcmp("sslverify", key))
server.ssl_verify = git_config_bool(key, val);
else if (!strcmp("preformattedhtml", key))
server.use_html = git_config_bool(key, val);
else if (!val)
return config_error_nonbool(key);
again would cause a error & die, message combo as above. There are many examples like that.
We can easily take a custom error message but again I was just aping the old system.
>> +NORETURN
>> +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);
>
> Do we have existing code that says "we signal that it is from the
> command line by setting linenr to zero" already? Otherwise I would
> have thought filename == NULL would be a more sensible convention.
>
> Otherwise OK.
>
Noted. Next reroll will have filename as the convention.
>> + else
>> + die(_("bad config variable '%s' at file line %d in %s"),
>
> At least, quote the last '%s'.
>
Noted. 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