From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: The config include mechanism doesn't allow for overwriting
Date: Mon, 22 Oct 2012 17:15:06 -0400 [thread overview]
Message-ID: <20121022211505.GA3301@sigill.intra.peff.net> (raw)
In-Reply-To: <CACBZZX4cu9XuS5AtduWrNeXNUeZ4rqDUzRdmyz2b3cXtmo1nqQ@mail.gmail.com>
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-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.
> 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 <local-email@example.com> 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 = git_pathdup("config");
- if (git_config_system())
- system_wide = git_etc_gitconfig();
- home_config_paths(&global, &xdg, "config");
- }
+ struct strbuf_list values = {0};
+ int i;
if (use_key_regexp) {
char *tl;
@@ -211,14 +204,11 @@ static int get_value(const char *key_, const char *regex_)
if (regcomp(key_regexp, key, REG_EXTENDED)) {
fprintf(stderr, "Invalid key pattern: %s\n", key_);
free(key);
- ret = CONFIG_INVALID_PATTERN;
- goto free_strings;
+ return CONFIG_INVALID_PATTERN;
}
} else {
- if (git_config_parse_key(key_, &key, NULL)) {
- ret = CONFIG_INVALID_KEY;
- goto free_strings;
- }
+ if (git_config_parse_key(key_, &key, NULL))
+ return CONFIG_INVALID_KEY;
}
if (regex_) {
@@ -230,37 +220,12 @@ static int get_value(const char *key_, const char *regex_)
regexp = (regex_t*)xmalloc(sizeof(regex_t));
if (regcomp(regexp, regex_, REG_EXTENDED)) {
fprintf(stderr, "Invalid pattern: %s\n", regex_);
- ret = CONFIG_INVALID_PATTERN;
- goto free_strings;
+ return CONFIG_INVALID_PATTERN;
}
}
- fn = show_config;
- data = NULL;
- if (respect_includes) {
- inc.fn = fn;
- inc.data = data;
- fn = git_config_include;
- data = &inc;
- }
-
- if (do_all && system_wide)
- git_config_from_file(fn, system_wide, data);
- if (do_all && xdg)
- git_config_from_file(fn, xdg, data);
- if (do_all && global)
- git_config_from_file(fn, global, data);
- if (do_all)
- git_config_from_file(fn, local, data);
- git_config_from_parameters(fn, data);
- if (!do_all && !seen)
- git_config_from_file(fn, local, data);
- if (!do_all && !seen && global)
- git_config_from_file(fn, global, data);
- if (!do_all && !seen && xdg)
- git_config_from_file(fn, xdg, data);
- if (!do_all && !seen && system_wide)
- git_config_from_file(fn, system_wide, data);
+ git_config_with_options(collect_config, &values,
+ given_config_file, respect_includes);
free(key);
if (regexp) {
@@ -268,16 +233,16 @@ static int get_value(const char *key_, const char *regex_)
free(regexp);
}
- if (do_all)
- ret = !seen;
- else
- ret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;
+ if (!values.nr)
+ return 1;
-free_strings:
- free(repo_config);
- free(global);
- free(xdg);
- return ret;
+ for (i = 0; i < values.nr; i++) {
+ struct strbuf *buf = values.items + i;
+ if (do_all || i == values.nr - 1)
+ fwrite(buf->buf, 1, buf->len, stdout);
+ strbuf_release(buf);
+ }
+ return 0;
}
static char *normalize_value(const char *key, const char *value)
next prev parent reply other threads:[~2012-10-22 21:15 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-22 15:55 The config include mechanism doesn't allow for overwriting Ævar Arnfjörð Bjarmason
2012-10-22 21:15 ` Jeff King [this message]
2012-10-23 14:13 ` Ævar Arnfjörð Bjarmason
2012-10-23 22:35 ` [PATCH 0/8] fix git-config with duplicate variable entries Jeff King
2012-10-23 22:35 ` [PATCH 1/8] t1300: style updates Jeff King
2012-10-24 6:33 ` Johannes Sixt
2012-10-24 6:37 ` Jeff King
2012-10-24 7:07 ` [PATCHv2 " Jeff King
2012-10-23 22:36 ` [PATCH 2/8] t1300: remove redundant test Jeff King
2012-10-23 22:36 ` [PATCH 3/8] t1300: test "git config --get-all" more thoroughly Jeff King
2012-10-23 22:36 ` [PATCH 4/8] git-config: remove memory leak of key regexp Jeff King
2012-10-23 22:38 ` [PATCH 5/8] git-config: fix regexp memory leaks on error conditions Jeff King
2012-10-23 22:40 ` [PATCH 6/8] git-config: collect values instead of immediately printing Jeff King
2012-10-23 22:41 ` [PATCH 7/8] git-config: do not complain about duplicate entries Jeff King
2012-10-23 22:41 ` [PATCH 8/8] git-config: use git_config_with_options Jeff King
2012-10-24 6:33 ` Johannes Sixt
2012-10-24 19:14 ` Ævar Arnfjörð Bjarmason
2012-11-20 19:08 ` [PATCH 0/8] fix git-config with duplicate variable entries Junio C Hamano
2012-11-21 19:27 ` Jeff King
2012-11-21 19:46 ` Junio C Hamano
2012-11-21 20:06 ` Jeff King
2012-11-21 20:42 ` Junio C Hamano
2012-11-23 10:37 ` Joachim Schmitz
2012-10-24 0:46 ` The config include mechanism doesn't allow for overwriting John Szakmeister
2012-10-24 0:51 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121022211505.GA3301@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).