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