Re: BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'

From: Jeff King <peff@peff.net>
Date: Feb 6, 2015, 19:33 UTC
Message-ID: <20150206193313.GA4220@peff.net>
In-Reply-To: <20150206124528.GA18859@inner.h.apk.li>

On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:

Show 17 quoted lines

> there seems to be a regression in the behaviour of 'git show_ref'
> (note the underscore). In v2.0.3-711-g586f414 it starts to say:
> 
>   $ ./git show_ref
>   error: invalid key: pager.show_ref
>   git: 'show_ref' is not a git command. See 'git --help'.
> 
> and somewhere (probably two commits, judging the diffs)
> later that changes again to:
> 
>   $ git show_ref
>   error: invalid key: pager.show_ref
>   error: invalid key: alias.show_ref
>   git: 'show_ref' is not a git command. See 'git --help'.
> 
> Apparently we need to squelch this message from
> within git_config_get_* in this case?

This is highlighting the problem with "pager.*" that Junio mentioned recently, which is that the keyname has arbitrary data, but syntactically is limited to alnum and "-". This should have been:

  pager.show_ref.enabled

from the beginning. But of course it was not. Even if we transition, we would want to support pager.* for a while.

I don't think squelching the messages is quite the right approach. They come from git_config_parse_key, which barfs on parsing the syntactically invalid keyname. So not only are we complaining, but we are not actually looking up the value. I don't think that's technically a regression in 586f414, though. The reader started to complain, but AFAICT git would not agree to parse a file containing:

  [pager]
  show_ref = true

in the first place. So it is not a new problem, but it is a bug that you cannot set pager config for such a command or alias.

I can think of a few possible paths forward:

  1. Squelch the messages, and declare "show_ref" and friends
     out-of-luck for pager config or aliases.

  2. Relax the syntactic rules for config keys to allow more characters.
     We cannot make this perfect (e.g., we cannot allow "." for reasons
     of ambiguity), but I imagine we could cover most practical cases.

     Note that we would need the matching loosening on the file-parsing
     side.

  3. Start phasing in pager.*.enabled (and I guess pager.*.command). We
     would still do the lookup of pager.* for backwards compatibility,
     but we would be careful to do so only when it is syntactically
     valid. IOW, this looks like (1), except the path forward for
     "show_ref" is to use the new, more robust, syntax.

-Peff