* [PATCH 1/8] t1300: add missing &&-chaining
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
@ 2012-02-16 8:02 ` Jeff King
2012-02-16 8:03 ` [PATCH 2/8] config: copy the return value of prefix_filename Jeff King
` (7 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Signed-off-by: Jeff King <peff@peff.net>
---
t/t1300-repo-config.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0690e0e..6de46bb 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -451,7 +451,7 @@ test_expect_success 'refer config from subdirectory' '
mkdir x &&
(
cd x &&
- echo strasse >expect
+ echo strasse >expect &&
git config --get --file ../other-config ein.bahn >actual &&
test_cmp expect actual
)
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 2/8] config: copy the return value of prefix_filename
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
2012-02-16 8:02 ` [PATCH 1/8] t1300: add missing &&-chaining Jeff King
@ 2012-02-16 8:03 ` Jeff King
2012-02-16 8:04 ` [PATCH 3/8] config: teach git_config_set_multivar_in_file a default path Jeff King
` (6 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The prefix_filename function returns a pointer to a static
buffer which may be overwritten by subsequent calls. Since
we are going to keep the result around for a while, let's be
sure to duplicate it for safety.
I don't think this can be triggered as a bug in the current
code, but it's a good idea to be defensive, as any resulting
bug would be quite subtle.
Signed-off-by: Jeff King <peff@peff.net>
---
I was tempted to simply use OPT_FILENAME to get the argument to "-f",
which would handle the prefixing for us. However, we can also get the
value from $GIT_CONFIG. That value doesn't actually run through this
code currently, but I think it should (and I fix this in patch 6).
builtin/config.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 09bf778..5a43a3c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -391,9 +391,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
config_exclusive_filename = git_pathdup("config");
else if (given_config_file) {
if (!is_absolute_path(given_config_file) && prefix)
- config_exclusive_filename = prefix_filename(prefix,
- strlen(prefix),
- given_config_file);
+ config_exclusive_filename =
+ xstrdup(prefix_filename(prefix,
+ strlen(prefix),
+ given_config_file));
else
config_exclusive_filename = given_config_file;
}
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 3/8] config: teach git_config_set_multivar_in_file a default path
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
2012-02-16 8:02 ` [PATCH 1/8] t1300: add missing &&-chaining Jeff King
2012-02-16 8:03 ` [PATCH 2/8] config: copy the return value of prefix_filename Jeff King
@ 2012-02-16 8:04 ` Jeff King
2012-02-16 8:04 ` [PATCH 4/8] config: teach git_config_rename_section a file argument Jeff King
` (5 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The git_config_set_multivar_in_file function takes a
filename argument to specify the file into which the values
should be written. Currently, this value must be non-NULL.
Callers which want to write to the default location must use
the regular, non-"in_file" version, which will either write
to config_exclusive_filename, or to the repo config if the
exclusive filename is NULL.
Let's migrate the "default to using repo config" logic into
the "in_file" form. That will let callers get the same
default-if-NULL behavior as one gets with
config_exclusive_filename, but without having to use the
global variable.
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/config.c b/config.c
index e3fcf75..a9eec58 100644
--- a/config.c
+++ b/config.c
@@ -1300,6 +1300,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
int fd = -1, in_fd;
int ret;
struct lock_file *lock = NULL;
+ char *filename_buf = NULL;
/* parse-key returns negative; flip the sign to feed exit(3) */
ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
@@ -1308,6 +1309,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
store.multi_replace = multi_replace;
+ if (!config_filename)
+ config_filename = filename_buf = git_pathdup("config");
/*
* The lock serves a purpose in addition to locking: the new
@@ -1477,6 +1480,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
out_free:
if (lock)
rollback_lock_file(lock);
+ free(filename_buf);
return ret;
write_err_out:
@@ -1488,19 +1492,9 @@ write_err_out:
int git_config_set_multivar(const char *key, const char *value,
const char *value_regex, int multi_replace)
{
- const char *config_filename;
- char *buf = NULL;
- int ret;
-
- if (config_exclusive_filename)
- config_filename = config_exclusive_filename;
- else
- config_filename = buf = git_pathdup("config");
-
- ret = git_config_set_multivar_in_file(config_filename, key, value,
- value_regex, multi_replace);
- free(buf);
- return ret;
+ return git_config_set_multivar_in_file(config_exclusive_filename,
+ key, value, value_regex,
+ multi_replace);
}
static int section_name_match (const char *buf, const char *name)
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 4/8] config: teach git_config_rename_section a file argument
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
` (2 preceding siblings ...)
2012-02-16 8:04 ` [PATCH 3/8] config: teach git_config_set_multivar_in_file a default path Jeff King
@ 2012-02-16 8:04 ` Jeff King
2012-02-16 8:05 ` [PATCH 5/8] config: provide a version of git_config with more options Jeff King
` (4 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The other config-writing functions (git_config_set and
git_config_set_multivar) each have an -"in_file" version to
write a specific file. Let's add one for rename_section,
with the eventual goal of moving away from the magic
config_exclusive_filename global.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 1 +
config.c | 20 +++++++++++++-------
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index 94f3eaf..7e375ce 100644
--- a/cache.h
+++ b/cache.h
@@ -1130,6 +1130,7 @@ extern int git_config_parse_key(const char *, char **, int *);
extern int git_config_set_multivar(const char *, const char *, const char *, int);
extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
+extern int git_config_rename_section_in_file(const char *, const char *, const char *);
extern const char *git_etc_gitconfig(void);
extern int check_repository_format_version(const char *var, const char *value, void *cb);
extern int git_env_bool(const char *, int);
diff --git a/config.c b/config.c
index a9eec58..c456600 100644
--- a/config.c
+++ b/config.c
@@ -1537,19 +1537,19 @@ static int section_name_match (const char *buf, const char *name)
}
/* if new_name == NULL, the section is removed instead */
-int git_config_rename_section(const char *old_name, const char *new_name)
+int git_config_rename_section_in_file(const char *config_filename,
+ const char *old_name, const char *new_name)
{
int ret = 0, remove = 0;
- char *config_filename;
+ char *filename_buf = NULL;
struct lock_file *lock = xcalloc(sizeof(struct lock_file), 1);
int out_fd;
char buf[1024];
FILE *config_file;
- if (config_exclusive_filename)
- config_filename = xstrdup(config_exclusive_filename);
- else
- config_filename = git_pathdup("config");
+ if (!config_filename)
+ config_filename = filename_buf = git_pathdup("config");
+
out_fd = hold_lock_file_for_update(lock, config_filename, 0);
if (out_fd < 0) {
ret = error("could not lock config file %s", config_filename);
@@ -1613,10 +1613,16 @@ unlock_and_out:
if (commit_lock_file(lock) < 0)
ret = error("could not commit config file %s", config_filename);
out:
- free(config_filename);
+ free(filename_buf);
return ret;
}
+int git_config_rename_section(const char *old_name, const char *new_name)
+{
+ return git_config_rename_section_in_file(config_exclusive_filename,
+ old_name, new_name);
+}
+
/*
* Call this to report error for your variable that should not
* get a boolean value (i.e. "[my] var" means "true").
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 5/8] config: provide a version of git_config with more options
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
` (3 preceding siblings ...)
2012-02-16 8:04 ` [PATCH 4/8] config: teach git_config_rename_section a file argument Jeff King
@ 2012-02-16 8:05 ` Jeff King
2012-02-16 8:07 ` [PATCH 6/8] config: stop using config_exclusive_filename Jeff King
` (3 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Callers may want to provide a specific version of a file in
which to look for config. Right now this can be done by
setting the magic global config_exclusive_filename variable.
By providing a version of git_config that takes a filename,
we can take a step towards making this magic global go away.
Furthermore, by providing a more "advanced" interface, we
now have a a natural place to add new options for callers
like git-config, which care about tweaking the specifics of
config lookup, without disturbing the large number of
"simple" users (i.e., every other part of git).
The astute reader of this patch may notice that the logic
for handling config_exclusive_filename was taken out of
git_config_early, but added into git_config. This means that
git_config_early will no longer respect config_exclusive_filename.
That's OK, because the only other caller of git_config_early
is check_repository_format_gently, but the only function
which sets config_exclusive_filename is cmd_config, which
does not call check_repository_format_gently (and if it did,
it would have been a bug, anyway, as we would be checking
the repository format in the wrong file).
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously this could also learn about a respect_includes flag, and it in
fact does in patch 8. I didn't include it in the initial version because
this part of the series is really about refactoring the
config_exclusive_filename stuff (and is semantically independent of the
notion of included files).
cache.h | 1 +
config.c | 22 +++++++++++++++++-----
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 7e375ce..7cb8874 100644
--- a/cache.h
+++ b/cache.h
@@ -1115,6 +1115,7 @@ extern int git_config_from_file(config_fn_t fn, const char *, void *);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
extern int git_config(config_fn_t fn, void *);
+extern int git_config_with_options(config_fn_t fn, void *, const char *filename);
extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index c456600..fbf883d 100644
--- a/config.c
+++ b/config.c
@@ -942,9 +942,6 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
int ret = 0, found = 0;
const char *home = NULL;
- /* Setting $GIT_CONFIG makes git read _only_ the given config file. */
- if (config_exclusive_filename)
- return git_config_from_file(fn, config_exclusive_filename, data);
if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
@@ -980,7 +977,8 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
return ret == 0 ? found : ret;
}
-int git_config(config_fn_t fn, void *data)
+int git_config_with_options(config_fn_t fn, void *data,
+ const char *filename)
{
char *repo_config = NULL;
int ret;
@@ -988,14 +986,28 @@ int git_config(config_fn_t fn, void *data)
inc.fn = fn;
inc.data = data;
+ fn = git_config_include;
+ data = &inc;
+
+ /*
+ * If we have a specific filename, use it. Otherwise, follow the
+ * regular lookup sequence.
+ */
+ if (filename)
+ return git_config_from_file(fn, filename, data);
repo_config = git_pathdup("config");
- ret = git_config_early(git_config_include, &inc, repo_config);
+ ret = git_config_early(fn, data, repo_config);
if (repo_config)
free(repo_config);
return ret;
}
+int git_config(config_fn_t fn, void *data)
+{
+ return git_config_with_options(fn, data, config_exclusive_filename);
+}
+
/*
* Find all the stuff for git_config_set() below.
*/
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 6/8] config: stop using config_exclusive_filename
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
` (4 preceding siblings ...)
2012-02-16 8:05 ` [PATCH 5/8] config: provide a version of git_config with more options Jeff King
@ 2012-02-16 8:07 ` Jeff King
2012-02-16 8:09 ` [PATCH 7/8] config: eliminate config_exclusive_filename Jeff King
` (2 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The git-config command sometimes operates on the default set
of config files (either reading from all, or writing to repo
config), and sometimes operates on a specific file. In the
latter case, we set the magic global config_exclusive_filename,
and the code in config.c does the right thing.
Instead, let's have git-config use the "advanced" variants
of config.c's functions which let it specify an individual
filename (or NULL for the default). This makes the code a
lot more obvious, and fixes two small bugs:
1. A relative path specified by GIT_CONFIG=foo will look
in the wrong directory if we have to chdir as part of
repository setup. We already handle this properly for
"git config -f foo", but the GIT_CONFIG lookup used
config_exclusive_filename directly. By dropping to a
single magic variable, the GIT_CONFIG case now just
works.
2. Calling "git config -f foo --edit" would not respect
core.editor. This is because just before editing, we
called git_config, which would respect the
config_exclusive_filename setting, even though this
particular git_config call was not about looking in the
user's specified file, but rather about loading actual
git config, just as any other git program would.
Signed-off-by: Jeff King <peff@peff.net>
---
The diffstat is slightly disappointing. It really is a one-for-one
conversion in almost every case, but the functions and variable names
are so bloody long that I had to wrap the calls to stay under 80
characters.
builtin/config.c | 61 ++++++++++++++++++++++++++++-------------------
t/t1300-repo-config.sh | 25 +++++++++++++++++++
2 files changed, 61 insertions(+), 25 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 5a43a3c..8901dd9 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -167,7 +167,7 @@ static int get_value(const char *key_, const char *regex_)
config_fn_t fn;
void *data;
- local = config_exclusive_filename;
+ local = given_config_file;
if (!local) {
const char *home = getenv("HOME");
local = repo_config = git_pathdup("config");
@@ -315,7 +315,8 @@ static void get_color(const char *def_color)
{
get_color_found = 0;
parsed_color[0] = '\0';
- git_config(git_get_color_config, NULL);
+ git_config_with_options(git_get_color_config, NULL,
+ given_config_file);
if (!get_color_found && def_color)
color_parse(def_color, "command line", parsed_color);
@@ -342,7 +343,8 @@ static int get_colorbool(int print)
{
get_colorbool_found = -1;
get_diff_color_found = -1;
- git_config(git_get_colorbool_config, NULL);
+ git_config_with_options(git_get_colorbool_config, NULL,
+ given_config_file);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -365,7 +367,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
int nongit = !startup_info->have_repository;
char *value;
- config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
+ given_config_file = getenv(CONFIG_ENVIRONMENT);
argc = parse_options(argc, argv, prefix, builtin_config_options,
builtin_config_usage,
@@ -380,27 +382,27 @@ int cmd_config(int argc, const char **argv, const char *prefix)
char *home = getenv("HOME");
if (home) {
char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
- config_exclusive_filename = user_config;
+ given_config_file = user_config;
} else {
die("$HOME not set");
}
}
else if (use_system_config)
- config_exclusive_filename = git_etc_gitconfig();
+ given_config_file = git_etc_gitconfig();
else if (use_local_config)
- config_exclusive_filename = git_pathdup("config");
+ given_config_file = git_pathdup("config");
else if (given_config_file) {
if (!is_absolute_path(given_config_file) && prefix)
- config_exclusive_filename =
+ given_config_file =
xstrdup(prefix_filename(prefix,
strlen(prefix),
given_config_file));
else
- config_exclusive_filename = given_config_file;
+ given_config_file = given_config_file;
}
if (respect_includes == -1)
- respect_includes = !config_exclusive_filename;
+ respect_includes = !given_config_file;
if (end_null) {
term = '\0';
@@ -438,28 +440,29 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
- if (git_config(show_all_config, NULL) < 0) {
- if (config_exclusive_filename)
+ if (git_config_with_options(show_all_config, NULL,
+ given_config_file) < 0) {
+ if (given_config_file)
die_errno("unable to read config file '%s'",
- config_exclusive_filename);
+ given_config_file);
else
die("error processing config file(s)");
}
}
else if (actions == ACTION_EDIT) {
check_argc(argc, 0, 0);
- if (!config_exclusive_filename && nongit)
+ if (!given_config_file && nongit)
die("not in a git directory");
git_config(git_default_config, NULL);
- launch_editor(config_exclusive_filename ?
- config_exclusive_filename : git_path("config"),
+ launch_editor(given_config_file ?
+ given_config_file : git_path("config"),
NULL, NULL);
}
else if (actions == ACTION_SET) {
int ret;
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
- ret = git_config_set(argv[0], value);
+ ret = git_config_set_in_file(given_config_file, argv[0], value);
if (ret == CONFIG_NOTHING_SET)
error("cannot overwrite multiple values with a single value\n"
" Use a regexp, --add or --replace-all to change %s.", argv[0]);
@@ -468,17 +471,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
else if (actions == ACTION_SET_ALL) {
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
- return git_config_set_multivar(argv[0], value, argv[2], 0);
+ return git_config_set_multivar_in_file(given_config_file,
+ argv[0], value, argv[2], 0);
}
else if (actions == ACTION_ADD) {
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
- return git_config_set_multivar(argv[0], value, "^$", 0);
+ return git_config_set_multivar_in_file(given_config_file,
+ argv[0], value, "^$", 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
- return git_config_set_multivar(argv[0], value, argv[2], 1);
+ return git_config_set_multivar_in_file(given_config_file,
+ argv[0], value, argv[2], 1);
}
else if (actions == ACTION_GET) {
check_argc(argc, 1, 2);
@@ -499,18 +505,22 @@ int cmd_config(int argc, const char **argv, const char *prefix)
else if (actions == ACTION_UNSET) {
check_argc(argc, 1, 2);
if (argc == 2)
- return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+ return git_config_set_multivar_in_file(given_config_file,
+ argv[0], NULL, argv[1], 0);
else
- return git_config_set(argv[0], NULL);
+ return git_config_set_in_file(given_config_file,
+ argv[0], NULL);
}
else if (actions == ACTION_UNSET_ALL) {
check_argc(argc, 1, 2);
- return git_config_set_multivar(argv[0], NULL, argv[1], 1);
+ return git_config_set_multivar_in_file(given_config_file,
+ argv[0], NULL, argv[1], 1);
}
else if (actions == ACTION_RENAME_SECTION) {
int ret;
check_argc(argc, 2, 2);
- ret = git_config_rename_section(argv[0], argv[1]);
+ ret = git_config_rename_section_in_file(given_config_file,
+ argv[0], argv[1]);
if (ret < 0)
return ret;
if (ret == 0)
@@ -519,7 +529,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
else if (actions == ACTION_REMOVE_SECTION) {
int ret;
check_argc(argc, 1, 1);
- ret = git_config_rename_section(argv[0], NULL);
+ ret = git_config_rename_section_in_file(given_config_file,
+ argv[0], NULL);
if (ret < 0)
return ret;
if (ret == 0)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 6de46bb..5f249f6 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -458,6 +458,14 @@ test_expect_success 'refer config from subdirectory' '
'
+test_expect_success 'refer config from subdirectory via GIT_CONFIG' '
+ (
+ cd x &&
+ GIT_CONFIG=../other-config git config --get ein.bahn >actual &&
+ test_cmp expect actual
+ )
+'
+
cat > expect << EOF
[ein]
bahn = strasse
@@ -960,4 +968,21 @@ test_expect_success 'git -c complains about empty key and value' '
test_must_fail git -c "" rev-parse
'
+test_expect_success 'git config --edit works' '
+ git config -f tmp test.value no &&
+ echo test.value=yes >expect &&
+ GIT_EDITOR="echo [test]value=yes >" git config -f tmp --edit &&
+ git config -f tmp --list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git config --edit respects core.editor' '
+ git config -f tmp test.value no &&
+ echo test.value=yes >expect &&
+ test_config core.editor "echo [test]value=yes >" &&
+ git config -f tmp --edit &&
+ git config -f tmp --list >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 7/8] config: eliminate config_exclusive_filename
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
` (5 preceding siblings ...)
2012-02-16 8:07 ` [PATCH 6/8] config: stop using config_exclusive_filename Jeff King
@ 2012-02-16 8:09 ` Jeff King
2012-02-16 8:10 ` [PATCH 8/8] config: do not respect includes for single-file --list Jeff King
2012-02-16 20:11 ` [PATCH 0/8] config-include fixes Junio C Hamano
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This is a magic global variable that was intended as an
override to the usual git-config lookup process. Once upon a
time, you could specify GIT_CONFIG to any git program, and
it would look only at that file. This turned out to be
confusing and cause a lot of bugs for little gain. As a
result, dc87183 (Only use GIT_CONFIG in "git config", not
other programs, 2008-06-30) took this away for all callers
except git-config.
Since git-config no longer uses it either, the variable can
just go away. As the diff shows, nobody was setting to
anything except NULL, so we can just replace any sites where
it was read with NULL.
Signed-off-by: Jeff King <peff@peff.net>
---
This could be squashed into the last patch (really, all of the last few
patches could be squashed). But I was able to "git grep
config_exclusive_filename" at this state and see that yes, indeed, the
variable is now totally useless.
cache.h | 2 --
config.c | 10 +++-------
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 7cb8874..411c60d 100644
--- a/cache.h
+++ b/cache.h
@@ -1150,8 +1150,6 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-extern const char *config_exclusive_filename;
-
#define MAX_GITNAME (1000)
extern char git_default_email[MAX_GITNAME];
extern char git_default_name[MAX_GITNAME];
diff --git a/config.c b/config.c
index fbf883d..e1d6857 100644
--- a/config.c
+++ b/config.c
@@ -26,8 +26,6 @@ static config_file *cf;
static int zlib_compression_seen;
-const char *config_exclusive_filename = NULL;
-
#define MAX_INCLUDE_DEPTH 10
static const char include_depth_advice[] =
"exceeded maximum include depth (%d) while including\n"
@@ -1005,7 +1003,7 @@ int git_config_with_options(config_fn_t fn, void *data,
int git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, config_exclusive_filename);
+ return git_config_with_options(fn, data, NULL);
}
/*
@@ -1504,8 +1502,7 @@ write_err_out:
int git_config_set_multivar(const char *key, const char *value,
const char *value_regex, int multi_replace)
{
- return git_config_set_multivar_in_file(config_exclusive_filename,
- key, value, value_regex,
+ return git_config_set_multivar_in_file(NULL, key, value, value_regex,
multi_replace);
}
@@ -1631,8 +1628,7 @@ out:
int git_config_rename_section(const char *old_name, const char *new_name)
{
- return git_config_rename_section_in_file(config_exclusive_filename,
- old_name, new_name);
+ return git_config_rename_section_in_file(NULL, old_name, new_name);
}
/*
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 8/8] config: do not respect includes for single-file --list
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
` (6 preceding siblings ...)
2012-02-16 8:09 ` [PATCH 7/8] config: eliminate config_exclusive_filename Jeff King
@ 2012-02-16 8:10 ` Jeff King
2012-02-16 20:11 ` [PATCH 0/8] config-include fixes Junio C Hamano
8 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-16 8:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The original include implementation tried not to impact
calls to "git config" that look at a single file. However,
since we called into git_config in a few places (e.g.,
--list), our respect_includes flag was not supported.
This patch teaches git_config_with_options a flag to
respect includes (instead of doing so by default), and
teaches git-config to use it.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/config.c | 7 ++++---
cache.h | 3 ++-
config.c | 14 ++++++++------
t/t1305-config-include.sh | 8 ++++++++
4 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8901dd9..d41a9bf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -316,7 +316,7 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
- given_config_file);
+ given_config_file, respect_includes);
if (!get_color_found && def_color)
color_parse(def_color, "command line", parsed_color);
@@ -344,7 +344,7 @@ static int get_colorbool(int print)
get_colorbool_found = -1;
get_diff_color_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
- given_config_file);
+ given_config_file, respect_includes);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -441,7 +441,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
if (git_config_with_options(show_all_config, NULL,
- given_config_file) < 0) {
+ given_config_file,
+ respect_includes) < 0) {
if (given_config_file)
die_errno("unable to read config file '%s'",
given_config_file);
diff --git a/cache.h b/cache.h
index 411c60d..ff54d6f 100644
--- a/cache.h
+++ b/cache.h
@@ -1115,7 +1115,8 @@ extern int git_config_from_file(config_fn_t fn, const char *, void *);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
extern int git_config(config_fn_t fn, void *);
-extern int git_config_with_options(config_fn_t fn, void *, const char *filename);
+extern int git_config_with_options(config_fn_t fn, void *,
+ const char *filename, int respect_includes);
extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index e1d6857..ad03908 100644
--- a/config.c
+++ b/config.c
@@ -976,16 +976,18 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
}
int git_config_with_options(config_fn_t fn, void *data,
- const char *filename)
+ const char *filename, int respect_includes)
{
char *repo_config = NULL;
int ret;
struct config_include_data inc = CONFIG_INCLUDE_INIT;
- inc.fn = fn;
- inc.data = data;
- fn = git_config_include;
- data = &inc;
+ if (respect_includes) {
+ inc.fn = fn;
+ inc.data = data;
+ fn = git_config_include;
+ data = &inc;
+ }
/*
* If we have a specific filename, use it. Otherwise, follow the
@@ -1003,7 +1005,7 @@ int git_config_with_options(config_fn_t fn, void *data,
int git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL);
+ return git_config_with_options(fn, data, NULL, 1);
}
/*
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 0a27ec4..f3e03a0 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -59,6 +59,14 @@ test_expect_success 'single file lookup does not expand includes by default' '
test_cmp expect actual
'
+test_expect_success 'single file list does not expand includes by default' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo "include.path=one" >expect &&
+ git config -f .gitconfig --list >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'writing config file does not expand includes' '
echo "[test]one = 1" >one &&
echo "[include]path = one" >.gitconfig &&
--
1.7.9.1.4.g8ffed
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 0/8] config-include fixes
2012-02-16 8:01 ` [PATCH 0/8] config-include fixes Jeff King
` (7 preceding siblings ...)
2012-02-16 8:10 ` [PATCH 8/8] config: do not respect includes for single-file --list Jeff King
@ 2012-02-16 20:11 ` Junio C Hamano
2012-02-17 0:14 ` Jeff King
8 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-02-16 20:11 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> I prepared this on top of what you have queued in jk/config-include.
> However, all of the cleanup is semantically independent of the topic
> (though there are a few minor textual conflicts). If I were re-rolling,
> I would put it all at the front, then squash patch 8 into my prior
> "implement config includes" patch.
Sorry for being late in answering the "revert or build on top" question; I
was mostly offline yesterday afternoon.
> [7/8]: config: eliminate config_exclusive_filename
>
> This is all cleanup which makes config_exclusive_filename go away. It's
> not strictly necessary for this series, but it's something I've been
> wanting to clean up for a while. And it does fix a few minor bugs (see
> patch 6/8). And the refactoring in 5/8 lays the groundwork for 8/8.
>
> [8/8]: config: do not respect includes for single-file --list
>
> The actual fix for the regression in my config-include patch.
Thanks.
Looking at the rebased result, it strikes me that with_options version
Furthermore, by providing a more "advanced" interface, we
now have a a natural place to add new options for callers
like git-config, which care about tweaking the specifics of
config lookup, without disturbing the large number of
"simple" users (i.e., every other part of git).
perhaps wants to get a pointer to struct config_lookup_options, instead of
us having to add a new parameter to all callsites every time a new need
is discovered.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/8] config-include fixes
2012-02-16 20:11 ` [PATCH 0/8] config-include fixes Junio C Hamano
@ 2012-02-17 0:14 ` Jeff King
2012-02-17 2:50 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-02-17 0:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Feb 16, 2012 at 12:11:52PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I prepared this on top of what you have queued in jk/config-include.
> > However, all of the cleanup is semantically independent of the topic
> > (though there are a few minor textual conflicts). If I were re-rolling,
> > I would put it all at the front, then squash patch 8 into my prior
> > "implement config includes" patch.
>
> Sorry for being late in answering the "revert or build on top" question; I
> was mostly offline yesterday afternoon.
No problem. I did it as build-on-top because it's much easier to squash
and reorder the commits later than it is to pick a re-roll apart into
multiple commits.
Which way did you want me to go with it?
> Looking at the rebased result, it strikes me that with_options version
>
> Furthermore, by providing a more "advanced" interface, we
> now have a a natural place to add new options for callers
> like git-config, which care about tweaking the specifics of
> config lookup, without disturbing the large number of
> "simple" users (i.e., every other part of git).
>
> perhaps wants to get a pointer to struct config_lookup_options, instead of
> us having to add a new parameter to all callsites every time a new need
> is discovered.
I considered that, but I noticed that the only callers who are really
going to care about the _with_options version will be in
builtin/config.c, and they all care about every option. And dealing with
creating a struct for each call seemed like more hassle.
OTOH, I could probably just make a single static global
config_lookup_options, and have all of the option parsing tweak it
directly (i.e., replace the given_config_file and respect_includes
static globals).
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/8] config-include fixes
2012-02-17 0:14 ` Jeff King
@ 2012-02-17 2:50 ` Junio C Hamano
2012-02-17 3:17 ` Jeff King
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-02-17 2:50 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> On Thu, Feb 16, 2012 at 12:11:52PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > I prepared this on top of what you have queued in jk/config-include.
>> > However, all of the cleanup is semantically independent of the topic
>> > (though there are a few minor textual conflicts). If I were re-rolling,
>> > I would put it all at the front, then squash patch 8 into my prior
>> > "implement config includes" patch.
>>
>> Sorry for being late in answering the "revert or build on top" question; I
>> was mostly offline yesterday afternoon.
>
> No problem. I did it as build-on-top because it's much easier to squash
> and reorder the commits later than it is to pick a re-roll apart into
> multiple commits.
>
> Which way did you want me to go with it?
I'll push out the re-roll that follows your outline above in 'pu'; I think
I got all the conflict resolved correctly, but please eyeball the result.
>> Looking at the rebased result, it strikes me that with_options version
>>
>> Furthermore, by providing a more "advanced" interface, we
>> now have a a natural place to add new options for callers
>> like git-config, which care about tweaking the specifics of
>> config lookup, without disturbing the large number of
>> "simple" users (i.e., every other part of git).
>>
>> perhaps wants to get a pointer to struct config_lookup_options, instead of
>> us having to add a new parameter to all callsites every time a new need
>> is discovered.
>
> I considered that, but I noticed that the only callers who are really
> going to care about the _with_options version will be in
> builtin/config.c, and they all care about every option.
Ok.
Thanks.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/8] config-include fixes
2012-02-17 2:50 ` Junio C Hamano
@ 2012-02-17 3:17 ` Jeff King
2012-02-17 3:23 ` Jeff King
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-02-17 3:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Feb 16, 2012 at 06:50:40PM -0800, Junio C Hamano wrote:
> I'll push out the re-roll that follows your outline above in 'pu'; I think
> I got all the conflict resolved correctly, but please eyeball the result.
Will do.
Since you are re-rolling, these are the documentation fixes I had
squashed in based on your earlier review (though come to think of it,
the new patches should now also describe `git_config_with_options`).
---
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index f428c5c..01f64d1 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -11,9 +11,9 @@ General Usage
Config files are parsed linearly, and each variable found is passed to a
caller-provided callback function. The callback function is responsible
for any actions to be taken on the config option, and is free to ignore
-some options (it is not uncommon for the configuration to be parsed
+some options. It is not uncommon for the configuration to be parsed
several times during the run of a git program, with different callbacks
-picking out different variables useful to themselves).
+picking out different variables useful to themselves.
A config callback function takes three parameters:
@@ -47,11 +47,12 @@ will first feed the user-wide one to the callback, and then the
repo-specific one; by overwriting, the higher-priority repo-specific
value is left at the end).
-There is a special version of `git_config` called `git_config_early`
-that takes an additional parameter to specify the repository config.
-This should be used early in a git program when the repository location
-has not yet been determined (and calling the usual lazy-evaluation
-lookup rules would yield an incorrect location).
+There is a special version of `git_config` called `git_config_early`.
+This version takes an additional parameter to specify the repository
+config, instead of having it looked up via `git_path`. This is useful
+early in a git program before the repository has been found. Unless
+you're working with early setup code, you probably don't want to use
+this.
Reading Specific Files
----------------------
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 0/8] config-include fixes
2012-02-17 3:17 ` Jeff King
@ 2012-02-17 3:23 ` Jeff King
2012-02-17 8:17 ` [PATCH 0/2] api-config documentation leftovers Jeff King
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Jeff King @ 2012-02-17 3:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Feb 16, 2012 at 10:17:23PM -0500, Jeff King wrote:
> On Thu, Feb 16, 2012 at 06:50:40PM -0800, Junio C Hamano wrote:
>
> > I'll push out the re-roll that follows your outline above in 'pu'; I think
> > I got all the conflict resolved correctly, but please eyeball the result.
>
> Will do.
I see you just pushed it out. The result looks good to me.
> Since you are re-rolling, these are the documentation fixes I had
> squashed in based on your earlier review (though come to think of it,
> the new patches should now also describe `git_config_with_options`).
Looks like you just reverted the "include" patch instead of the merge of
the whole topic. I'll prepare a few documentation fixups on top, then.
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 0/2] api-config documentation leftovers
2012-02-17 3:23 ` Jeff King
@ 2012-02-17 8:17 ` Jeff King
2012-02-17 17:04 ` Junio C Hamano
2012-02-17 8:18 ` [PATCH 1/2] docs/api-config: minor clarifications Jeff King
2012-02-17 8:18 ` [PATCH 2/2] docs/api-config: describe git_config_with_options Jeff King
2 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-02-17 8:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Feb 16, 2012 at 10:23:25PM -0500, Jeff King wrote:
> > Since you are re-rolling, these are the documentation fixes I had
> > squashed in based on your earlier review (though come to think of it,
> > the new patches should now also describe `git_config_with_options`).
>
> Looks like you just reverted the "include" patch instead of the merge of
> the whole topic. I'll prepare a few documentation fixups on top, then.
Here they are, on top of what you have in jk/config-include. Squashing
would involve breaking apart the second one into the "introduce
git_config_with_options" part and the "and now it learns
respect_includes" part. So it's probably not worth the effort.
[1/2]: docs/api-config: minor clarifications
[2/2]: docs/api-config: describe git_config_with_options
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2] docs/api-config: minor clarifications
2012-02-17 3:23 ` Jeff King
2012-02-17 8:17 ` [PATCH 0/2] api-config documentation leftovers Jeff King
@ 2012-02-17 8:18 ` Jeff King
2012-02-17 8:18 ` [PATCH 2/2] docs/api-config: describe git_config_with_options Jeff King
2 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-17 8:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The first change simply drops some parentheses to make a
statement more clear. The seconds clarifies that almost
nobody wants to call git_config_early.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-config.txt | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index c60b6b3..67984da 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -11,9 +11,9 @@ General Usage
Config files are parsed linearly, and each variable found is passed to a
caller-provided callback function. The callback function is responsible
for any actions to be taken on the config option, and is free to ignore
-some options (it is not uncommon for the configuration to be parsed
+some options. It is not uncommon for the configuration to be parsed
several times during the run of a git program, with different callbacks
-picking out different variables useful to themselves).
+picking out different variables useful to themselves.
A config callback function takes three parameters:
@@ -47,11 +47,12 @@ will first feed the user-wide one to the callback, and then the
repo-specific one; by overwriting, the higher-priority repo-specific
value is left at the end).
-There is a special version of `git_config` called `git_config_early`
-that takes an additional parameter to specify the repository config.
-This should be used early in a git program when the repository location
-has not yet been determined (and calling the usual lazy-evaluation
-lookup rules would yield an incorrect location).
+There is a special version of `git_config` called `git_config_early`.
+This version takes an additional parameter to specify the repository
+config, instead of having it looked up via `git_path`. This is useful
+early in a git program before the repository has been found. Unless
+you're working with early setup code, you probably don't want to use
+this.
Reading Specific Files
----------------------
--
1.7.9.9.gcf58
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/2] docs/api-config: describe git_config_with_options
2012-02-17 3:23 ` Jeff King
2012-02-17 8:17 ` [PATCH 0/2] api-config documentation leftovers Jeff King
2012-02-17 8:18 ` [PATCH 1/2] docs/api-config: minor clarifications Jeff King
@ 2012-02-17 8:18 ` Jeff King
2 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-02-17 8:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This function was added recently but not documented.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-config.txt | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 67984da..3dbf9fd 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -47,6 +47,22 @@ will first feed the user-wide one to the callback, and then the
repo-specific one; by overwriting, the higher-priority repo-specific
value is left at the end).
+The `git_config_with_options` function lets the caller examine config
+while adjusting some of the default behavior of `git_config`. It should
+almost never be used by "regular" git code that is looking up
+configuration variables. It is intended for advanced callers like
+`git-config`, which are intentionally tweaking the normal config-lookup
+process. It takes two options:
+
+`filename`::
+If this parameter is non-NULL, it specifies the name of a file to
+parse for configuration, rather than looking in the usual files. Regular
+`git_config` defaults to `NULL`.
+
+`respect_includes`::
+Specify whether include directives should be followed in parsed files.
+Regular `git_config` defaults to `1`.
+
There is a special version of `git_config` called `git_config_early`.
This version takes an additional parameter to specify the repository
config, instead of having it looked up via `git_path`. This is useful
--
1.7.9.9.gcf58
^ permalink raw reply related [flat|nested] 23+ messages in thread