[PATCH 0/8] fix git-config with duplicate variable entries

From: Jeff King <peff@peff.net>
Date: Oct 23, 2012, 22:35 UTC
Message-ID: <20121023223502.GA23194@sigill.intra.peff.net>
In-Reply-To: <CACBZZX5mOb7_i9r8AqNK5V3r-gVnzN+rkeY9xrhecGv1rS-anA@mail.gmail.com>

On Tue, Oct 23, 2012 at 04:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

Thanks. It had a few minor flaws (like a memory leak). I fixed those, updated the tests, and split it out into a few more readable commits. In the process, I managed to uncover and fix a few other memory leaks in the area. I think this version is much more readable, and writing the rationale for patch 7 convinced me that it's the right thing to do. Another round of review would be appreciated.

  [1/8]: t1300: style updates
  [2/8]: t1300: remove redundant test
  [3/8]: t1300: test "git config --get-all" more thoroughly
  [4/8]: git-config: remove memory leak of key regexp
  [5/8]: git-config: fix regexp memory leaks on error conditions
  [6/8]: git-config: collect values instead of immediately printing
  [7/8]: git-config: do not complain about duplicate entries
  [8/8]: git-config: use git_config_with_options

For those just joining us, the interesting bit is patch 7, which fixes some inconsistencies between the "git-config" tool and how the internal config callbacks work.

Show 15 quoted lines

> 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

Yes, that looks right.

Show 9 quoted lines

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

Right.

Show 7 quoted lines

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

No. Some variables can legitimately appear multiple times. E.g., remote.*.fetch, remote.*.push, and remote.*.url. Probably more that I am forgetting. There are not many, but they do exist.

It's OK to tweak the regular "get" for them, since they are already broken for that case[1]. You need to use "--get-all" if you expect the variable to have multiple values. But when we are listing, we do not have the hint as to what is expected, and we need to show all entries.

-Peff

[1] So the one useful thing that the current duplicate check is doing is
    flagging errors where you wanted to use --get-all, but forgot to.
    However, it's not really a sufficient safeguard anyway, since it
    would not catch cases where the list was split across multiple
    files (which does work with the internal callbacks that handle
    lists, since they never even see that multiple files are involved).
    It's much more important for git-config to be consistent with the
    internal parsing behavior.