Re: [PATCH 1/4] Split up default "core" config parsing into helper routine

From: Jeff King <peff@peff.net>
Date: Jun 18, 2008, 23:13 UTC
Message-ID: <20080618231316.GB23053@sigill.intra.peff.net>
In-Reply-To: <alpine.LFD.1.10.0806181552590.2907@woody.linux-foundation.org>

On Wed, Jun 18, 2008 at 03:58:13PM -0700, Linus Torvalds wrote:

Show 5 quoted lines

> >         if (!prefixcmp(var, "core."))
> >                 return git_default_core_config(var+5, value);
> 
> I considered it, but I think that screws up error reporting (ie if some 
> value is unparseable, it would then print out the wrong variable name).

Good point.

> Yeah, I know. I love the config file format (quite frankly, anybody who 
> thinks XML and friends are sane is a total moron and should be shot before 
> they reproduce), but the whole parsing code was a really quick hack.

I actually find the hierarchy a bit nonsensical. We talk about core.pager and branch.master.remote, but the config file doesn't look like that. So I buy that it's

[core]
  pager

but then it seems kind of arbitrary to have

[branch "master"]
  remote

instead of

[branch]
  master.remote

or

[branch]
  [master]
    remote

which is of course impossible because we don't have a "close section" syntax element.

It seems like the '.' implies hierarchy, but the syntax of the file doesn't really follow it. But that's a minor issue, and really not worth the pain it would take to change at this point.

> I've several times wanted to rewrite it so that it does something smarter 
> (parse it once, save it in a nice data structure), but let's face it, the 
> upside is rather small. So I've never really ended up having the energy.

I think we need to refactor what happens at the beginning of the 'git' wrapper, and this may be part of it. But as it stands now, the worktree setup is a minefield of bugs (e.g., calling a command via an alias versus directly can get you different results for whether you have a worktree!), and a simple patch I did to make pager use configurable blew up in my face due to implicit constraints in when we look at the config file (i.e., it mostly worked, but reading the config file implied doing a setup_git_directory, which then screwed up a later invocation of setup_git_directory).

So I think those things need cleaning up, and refactoring the config reading could probably be part of that. I might try to look at it in the next release cycle.

Show 10 quoted lines

> > Then we could do useful things like say "you specified core.foobar, but
> > there is no such variable."
> 
> No. We could already do that (just add it to the end of 
> git_default_core_config - it should be called last even if there was a 
> chain), but avoid doing that very much on purpose.
> 
> Why? Because it's really irritating to have a parser that complains about 
> newer values (or old deprecated ones) that don't matter for that version 
> of the program.

Even a "check my config for bogosity" program would be nice, and then it wouldn't bug you all the time. But you _can't_ do it now, because the callback mechanism only looks at a subset of the values. IOW, there is no single function that you call that represents every config value; different programs use different callbacks.

In the case of just core.*, though, you might be able to get away with it (I don't know how closely we've adhered to "all of core.* is in git_default_config).

-Peff