Re: The config include mechanism doesn't allow for overwriting
On Tue, Oct 23, 2012 at 08:46:47PM -0400, John Szakmeister wrote:
> On Tue, Oct 23, 2012 at 10:13 AM, Ævar Arnfjörð Bjarmason
> wrote:
> [snip]
> > 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.
>
> I frequently use pushurl in my remotes to push my master branch both
> to the original repo and my forked version. I find it very helpful in
> my workflow, and would hate to lose that. That said, I do like the
> idea of having a config file and the ability to override some of the
> variables.
No, that won't go anywhere. We really do have two classes of variables:
things that are expected to be single values, and things that are
expected to be lists.
>From the perspective of the config code, we don't know or care which is
which, and just feed all entries sequentially to a C callback. In
practice, the callbacks do one of two things:
1. Append the values into a list.
2. Overwrite, and end up with the final value seen.
The trouble is that git-config has to print the values in a reasonable
way, so it asks the caller to give a hint about which it wants (--get
versus --get-all). But in the single-value case did not behave like the
C callbacks, which is what my series fixes.
Using "git config -l" is more like asking the config machinery to just
feed us everything, which is what the C callbacks see. Which is more
flexible, but way less convenient for the caller. But it doesn't need to
be fixed, since the caller has all the information to implement whatever
semantics they like.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The config include mechanism doesn't allow for overwriting
On Tue, Oct 23, 2012 at 10:13 AM, Ævar Arnfjörð Bjarmason
wrote:
[snip]
> 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.
I frequently use pushurl in my remotes to push my master branch both
to the original repo and my forked version. I find it very helpful in
my workflow, and would hate to lose that. That said, I do like the
idea of having a config file and the ability to override some of the
variables.
-John
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The config include mechanism doesn't allow for overwriting
On Mon, Oct 22, 2012 at 11:15 PM, Jeff King wrote:
> On Mon, Oct 22, 2012 at 05:55:00PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> I was hoping to write something like this:
>>
>> [user]
>> name = Luser
>> email = some-defa...@example.com
>> [include]
>> path = ~/.gitconfig.d/user-email
>>
>> Where that file would contain:
>>
>> [user]
>> email = local-em...@example.com
>
> The intent is that it would work as you expect, and produce
> local-em...@example.com.
>
>> But when you do that git prints:
>>
>> $ git config --get user.email
>> some-defa...@example.com
>> error: More than one value for the key user.email:
>> local-em...@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 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.
>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The config include mechanism doesn't allow for overwriting
On Mon, Oct 22, 2012 at 05:55:00PM +0200, Ævar Arnfjörð Bjarmason wrote:
> I was hoping to write something like this:
>
> [user]
> name = Luser
> email = some-defa...@example.com
> [include]
> path = ~/.gitconfig.d/user-email
>
> Where that file would contain:
>
> [user]
> email = local-em...@example.com
The intent is that it would work as you expect, and produce
local-em...@example.com.
> But when you do that git prints:
>
> $ git config --get user.email
> some-defa...@example.com
> error: More than one value for the key user.email:
> local-em...@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 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.
> 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.
---
builtin/config.c | 111 ---
1 file changed, 38 insertions(+), 73 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index d6a066b..72cb0a8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -16,7 +16,6 @@ static int do_not_match;
static int use_key_regexp;
static int do_all;
static int do_not_match;
-static int seen;
static char delim = '=';
static char key_delim = ' ';
static char term = '\n';
@@ -110,12 +109,19 @@ static int show_config(const char *key_, const char
*value_, void *cb)
return 0;
}
-static int show_config(const char *key_, const char *value_, void *cb)
+struct strbuf_list {
+ struct strbuf *items;
+ int nr;
+ int alloc;
+};
+
+static int collect_config(const char *key_, const char *value_, void *cb)
{
+ struct strbuf_list *values = cb;
+ struct strbuf *buf;
char value[256];
const char *vptr = value;
int must_free_vptr = 0;
- int dup_error = 0;
int must_print_delim = 0;
if (!use_key_regexp && strcmp(key_, key))
@@ -126,12 +132,15 @@ static int show_config(const char *key_, const char
*value_, void *cb)
(do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
return 0;
+ ALLOC_GROW(values->items, values->nr + 1, values->alloc);
+ buf = &values->items[values->nr++];
+ strbuf_init(buf, 0);
+
if (show_keys) {
- printf("%s", key_);
+ strbuf_addstr(buf, key_);
must_print_delim = 1;
}
- if (seen && !do_all)
- dup_error = 1;
+
if (types == TYPE_INT)
sprintf(value, "%d", git_config_int(key_, value_?value_:""));
else if (types == TYPE_BOOL)
@@ -153,16 +162,12 @@ static int show_config(const char *key_, const char
*value_, void *cb)
vptr = "";
must_print_delim = 0;
}
- seen++;
- if (dup_error) {
- error("More than one value for the key %s: %s",
- key_, vptr);
- }
- else {
- if (must_print_delim)
- printf("%c", key_delim);
- printf("%s%c", vptr, term);
- }
+
+ if (must_print_delim)
+ strbuf_addch(buf, key_delim);
+ strbuf_addstr(buf, vptr);
+ strbuf_addch(buf, term);
+
if (must_free_vptr)
/* If vptr must be freed, it's a pointer to a
* dynamically allocated buffer, it's safe to cast to
@@ -175,20 +180,8 @@ static int get_value(const char *key_, const char *regex_)
static int get_value(const char *key_, const char *regex_)
{
- int ret = CONFIG_GENERIC_ERROR;
- char *global = NULL, *xdg = NULL, *repo_config = NULL;
- const char *system_wide = NULL, *local;
- struct config_include_data inc = CONFIG_INCLUDE_INIT;
- config_fn_t fn;
- void *data;
-
- local = given_config_file;
- if (!local) {
- local = repo_config = gi
