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

quoted text
> >         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.

quoted text
> 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.

quoted text
> 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.

quoted text
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html