Re: The config include mechanism doesn't allow for overwriting

On Mon, Oct 22, 2012 at 11:15 PM, Jeff King [off-list ref] wrote:

On Mon, Oct 22, 2012 at 05:55:00PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

I was hoping to write something like this:

    [user]
        name = Luser
        email = some-default@example.com
    [include]
        path = ~/.gitconfig.d/user-email

Where that file would contain:

    [user]
        email = local-email@example.com

The intent is that it would work as you expect, and produce
local-email@example.com.

quoted

But when you do that git prints:

    $ git config --get user.email
     some-default@example.com
     error: More than one value for the key user.email: local-email@example.com

Ugh. The config code just feeds all the values sequentially to the
callback. The normal callbacks within git will overwrite old values,
whether from earlier in the file, from a file with lower priority (e.g.,
/etc/gitconfig versus ~/.gitconfig), or from an earlier included. Which
you can check with:

  $ git var GIT_AUTHOR_IDENT
  Luser [off-list ref] 1350936694 -0400

But git-config takes it upon itself to detect duplicates in its
callback. Which is just silly, since it is not something that regular
git would do. git-config should behave as much like the internal git
parser as possible.

quoted

I think config inclusion is much less useful when you can't clobber
previously assigned values.

Agreed. But I think the bug is in git-config, not in the include
mechanism. I think I'd like to do something like the patch below, which
just reuses the regular config code for git-config, collects the values,
and then reports them. It does mean we use a little more memory (for the
sake of simplicity, we store values instead of streaming them out), but
the code is much shorter, less confusing, and automatically matches what
regular git_config() does.

It fails a few tests in t1300, but it looks like those tests are testing
for the behavior we have identified as wrong, and should be fixed.

I think this patch looks good.

One other thing I think is worth clarifying (and I think should be
broken) is if you write a configuration like:

    [foo]
        bar = one
    [foo]
        bar = two
    [foo]
        bar = three

"git-{config,var} -l" will both give you:

    foo.bar=one
    foo.bar=two
    foo.bar=three

And git config --get foo.bar will give you:

    $ git config -f /tmp/test --get foo.bar
    one
    error: More than one value for the key foo.bar: two
    error: More than one value for the key foo.bar: three

I think that it would be better if the config mechanism just silently
overwrote keys that clobbered earlier keys like your patch does.

But in addition can we simplify things for the consumers of
"git-{config,var} -l" by only printing:

    foo.bar=three

Or are there too many variables like "include.path" that can
legitimately appear more than once.