On 8/4/2014 11:37 PM, Junio C Hamano wrote:
> Tanay Abhra <tanay...@gmail.com> 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 

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 

>> +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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to