* [PATCH 0/2] config includes, take 2
@ 2012-02-06 6:27 Jeff King
2012-02-06 6:29 ` [PATCH 1/2] imap-send: remove dead code Jeff King
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Jeff King @ 2012-02-06 6:27 UTC (permalink / raw)
To: git
I decided to drop the "read config from a ref" bits of the series. There
was some debate about what the proper workflow should be around projects
sharing config. And since I am not part of a project that does so, I
can't contribute anything empirical to the discussion. So I'll let that
part live on in the list archive, and somebody whose project really
wants to experiment with it can feel free to do so.
That leaves the file-inclusion bits:
[1/2]: imap-send: remove dead code
[2/2]: config: add include directive
The first patch is new in this round, and is a necessary cleanup for the
second patch (though it might be worth applying regardless).
The second patch corresponds to patch 1/4 from the previous round. Among
the functional changes are:
1. do not use includes for "git config" in single-file mode
2. do not respect include.foo.path
3. perform cycle and duplicate detection on included files
Doing (3) actually ended up changing the implementation a fair bit. The
original implementation worked purely as a drop-in callback wrapper. But
to suppress duplicates, you need to know not just which files you've
included, but which files you started from. So either
git_config_from_file has to learn about the include mechanism, or each
caller has to manually mark the file it is about to read.
I ended up just teaching the config machinery about the include
mechanism. It meant more changes (because we now pass an extra "include"
argument between the config functions) but I think turns out much more
readable.
That being said, I'm having doubts about the idea of duplicate
suppression at all. Suppose I have a setup like this:
git config -f foo test.value foo
git config test.value repo
git config include.path $PWD/foo
git config --global test.value global
git config --global include.path $PWD/foo
That is, two config files, each of which sets test.value but also
includes "foo". One might assume from this that the output of "git
config test.value" would be "foo". But because of duplicate suppression,
it's not.
We first read the global config, which sets the value to "global", then
includes foo, which overwrites it to "foo". Then we read the repo
config, which sets the value to "repo", and then does _not_ actually
read foo. Because git config is read linearly and later values tend to
overwrite earlier ones, we would want to suppress the _first_ instance
of a file, not the second (or really, the final if it is included many
times). But that is impossible to do without generating a complete graph
of includes and then pruning the early ones.
Sure, this is a trivial toy example, and by looking at all of the
inputs, you can see what is happening and fix it. But the point of
duplicate suppression was that individual config files wouldn't have to
know or care what was being included elsewhere. A slightly more
real-world example is this:
1. A config file "/etc/git-defaults-foo.cfg" sets a bunch of
reasonable defaults for some group of developers.
2. One of the developers really likes these foo defaults, but wants to
override one particular option. So he does this:
git config --global include.path /etc/git-defaults-foo.cfg
git config --global some.option override
3. On one particular repo, though, this dev wants the stock foo
defaults. So he does:
git config --global include.path /etc/git-defaults-foo.cfg
But we ignore the final include request; it's suppressed as a duplicate,
even though it would cancel his override.
So I'm actually thinking I should drop the duplicate suppression and
just do some sort of sanity check on include-depth to break cycles
(i.e., just die because it's a crazy thing to do, and we are really just
trying to tell the user their config is broken rather than go into an
infinite loop). As a bonus, it makes the code much simpler, too.
I'll post the patch with duplicate suppression for reference, but read
it with a grain of salt (or don't bother to read it at all if after
reading the above you think it should be thrown out).
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/2] imap-send: remove dead code 2012-02-06 6:27 [PATCH 0/2] config includes, take 2 Jeff King @ 2012-02-06 6:29 ` Jeff King 2012-02-06 6:31 ` [PATCH 2/2] config: add include directive Jeff King ` (3 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2012-02-06 6:29 UTC (permalink / raw) To: git The imap-send code was adapted from another project, and still contains many unused bits of code. One of these bits contains a type "struct string_list" which bears no resemblence to the "struct string_list" we use elsewhere in git. This causes the compiler to complain if git's string_list ever becomes part of cache.h. Let's just drop the dead code. Signed-off-by: Jeff King <peff@peff.net> --- This is necessary for patch 2, which does include string-list.h in cache.h. If you read my cover letter, I think patch 2 might not be the best approach. However, I think it might be worth applying this just as a cleanup (e.g., even without the build problems, grepping for "struct string_list" turns up this useless cruft). imap-send.c | 23 ----------------------- 1 files changed, 0 insertions(+), 23 deletions(-) diff --git a/imap-send.c b/imap-send.c index e40125a..972ad62 100644 --- a/imap-send.c +++ b/imap-send.c @@ -42,28 +42,6 @@ struct store_conf { unsigned trash_remote_new:1, trash_only_new:1; }; -struct string_list { - struct string_list *next; - char string[1]; -}; - -struct channel_conf { - struct channel_conf *next; - char *name; - struct store_conf *master, *slave; - char *master_name, *slave_name; - char *sync_state; - struct string_list *patterns; - int mops, sops; - unsigned max_messages; /* for slave only */ -}; - -struct group_conf { - struct group_conf *next; - char *name; - struct string_list *channels; -}; - /* For message->status */ #define M_RECENT (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */ #define M_DEAD (1<<1) /* expunged */ @@ -71,7 +49,6 @@ struct group_conf { struct message { struct message *next; - /* struct string_list *keywords; */ size_t size; /* zero implies "not fetched" */ int uid; unsigned char flags, status; -- 1.7.9.rc1.29.g43677 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] config: add include directive 2012-02-06 6:27 [PATCH 0/2] config includes, take 2 Jeff King 2012-02-06 6:29 ` [PATCH 1/2] imap-send: remove dead code Jeff King @ 2012-02-06 6:31 ` Jeff King 2012-02-06 7:41 ` [PATCH 0/2] config includes, take 2 Junio C Hamano ` (2 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2012-02-06 6:31 UTC (permalink / raw) To: git [If you haven't read my cover letter, do so now. The surprise ending is that I think this patch may not be the right approach, and I am posting it for reference. I don't want anybody to waste time reading and reviewing this without that context.] -- >8 -- It can be useful to split your ~/.gitconfig across multiple files. For example, you might have a "main" file which is used on many machines, but a small set of per-machine tweaks. Or you may want to make some of your config public (e.g., clever aliases) while keeping other data back (e.g., your name or other identifying information). Or you may want to include a number of config options in some subset of your repos without copying and pasting (e.g., you want to reference them from the .git/config of participating repos). This patch introduces an include directive for config files. It looks like: [include] path = /path/to/file This is syntactically backwards-compatible with existing git config parsers (i.e., they will see it as another config entry and ignore it unless you are looking up include.path). The implementation adds a new "include" parameter to git_config_from_file to turn on includes. Include directives are turned on for "regular" git config parsing: when you call git_config(), or when you make a call to the "git config" program without using any single-file options. Includes are off in other cases, including: 1. Parsing of other config-like files, like .gitmodules. There isn't a real need, and I'd rather be conservative and avoid unnecessary incompatibility or confusion. 2. Reading single files via "git config". This is for two reasons: a. backwards compatibility with scripts looking at config-like files. b. inspection of a specific file probably means you care about just what's in that file, not a general lookup for "do we have this value anywhere at all". If that is not the case, the caller can always specify "--includes". 3. Writing files via "git config"; we want to treat include.* variables as literal items to be copied (or modified), and not expand them. So "git config --unset-all foo.bar" would operate _only_ on .git/config, not any of its included files (just as it also does not operate on ~/.gitconfig). Signed-off-by: Jeff King <peff@peff.net> --- Documentation/config.txt | 15 ++++ Documentation/git-config.txt | 5 ++ builtin/clone.c | 2 +- builtin/config.c | 19 ++++-- builtin/init-db.c | 2 +- cache.h | 19 ++++- config.c | 118 ++++++++++++++++++++++++++++------ sequencer.c | 2 +- setup.c | 2 +- submodule.c | 2 +- t/t1305-config-include.sh | 145 ++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 295 insertions(+), 36 deletions(-) create mode 100755 t/t1305-config-include.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index abeb82b..e55dae1 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -84,6 +84,17 @@ customary UNIX fashion. Some variables may require a special value format. +Includes +~~~~~~~~ + +You can include one config file from another by setting the special +`include.path` variable to the name of the file to be included. The +included file is expanded immediately, as if its contents had been +found at the location of the include directive. If the value of the +`include.path` variable is a relative path, the path is considered to be +relative to the configuration file in which the include directive was +found. See below for examples. + Example ~~~~~~~ @@ -106,6 +117,10 @@ Example gitProxy="ssh" for "kernel.org" gitProxy=default-proxy ; for the rest + [include] + path = /path/to/foo.inc ; include by absolute path + path = foo ; expand "foo" relative to the current file + Variables ~~~~~~~~~ diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index e7ecf5d..aa8303b 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -178,6 +178,11 @@ See also <<FILES>>. Opens an editor to modify the specified config file; either '--system', '--global', or repository (default). +--includes:: +--no-includes:: + Respect `include.*` directives in config files when looking up + values. Defaults to on. + [[FILES]] FILES ----- diff --git a/builtin/clone.c b/builtin/clone.c index c62d4b5..32b7808 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -614,7 +614,7 @@ static void write_config(struct string_list *config) for (i = 0; i < config->nr; i++) { if (git_config_parse_parameter(config->items[i].string, - write_one_config, NULL) < 0) + write_one_config, NULL, NULL) < 0) die("unable to write parameters to config file"); } } diff --git a/builtin/config.c b/builtin/config.c index d35c06a..be5d0fb 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -25,6 +25,7 @@ static const char *given_config_file; static int actions, types; static const char *get_color_slot, *get_colorbool_slot; static int end_null; +static struct config_include_context include = CONFIG_INCLUDE_INIT(-1); #define ACTION_GET (1<<0) #define ACTION_GET_ALL (1<<1) @@ -74,6 +75,7 @@ static struct option builtin_config_options[] = { OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH), OPT_GROUP("Other"), OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"), + OPT_BOOL(0, "includes", &include.enabled, "respect include directives on lookup"), OPT_END(), }; @@ -214,18 +216,18 @@ static int get_value(const char *key_, const char *regex_) } if (do_all && system_wide) - git_config_from_file(show_config, system_wide, NULL); + git_config_from_file(show_config, system_wide, NULL, &include); if (do_all && global) - git_config_from_file(show_config, global, NULL); + git_config_from_file(show_config, global, NULL, &include); if (do_all) - git_config_from_file(show_config, local, NULL); - git_config_from_parameters(show_config, NULL); + git_config_from_file(show_config, local, NULL, &include); + git_config_from_parameters(show_config, NULL, &include); if (!do_all && !seen) - git_config_from_file(show_config, local, NULL); + git_config_from_file(show_config, local, NULL, &include); if (!do_all && !seen && global) - git_config_from_file(show_config, global, NULL); + git_config_from_file(show_config, global, NULL, &include); if (!do_all && !seen && system_wide) - git_config_from_file(show_config, system_wide, NULL); + git_config_from_file(show_config, system_wide, NULL, &include); free(key); if (regexp) { @@ -384,6 +386,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) config_exclusive_filename = given_config_file; } + if (include.enabled == -1) + include.enabled = !config_exclusive_filename; + if (end_null) { term = '\0'; delim = '\n'; diff --git a/builtin/init-db.c b/builtin/init-db.c index 0dacb8b..de6290f 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -146,7 +146,7 @@ static void copy_templates(const char *template_dir) strcpy(template_path + template_len, "config"); repository_format_version = 0; git_config_from_file(check_repository_format_version, - template_path, NULL); + template_path, NULL, NULL); template_path[template_len] = 0; if (repository_format_version && diff --git a/cache.h b/cache.h index 9bd8c2d..f75923e 100644 --- a/cache.h +++ b/cache.h @@ -7,6 +7,7 @@ #include "advice.h" #include "gettext.h" #include "convert.h" +#include "string-list.h" #include SHA1_HEADER #ifndef git_SHA_CTX @@ -1108,13 +1109,22 @@ extern int update_server_info(int); #define CONFIG_NOTHING_SET 5 #define CONFIG_INVALID_PATTERN 6 +struct config_include_context { + struct string_list seen_paths; + int enabled; +}; +#define CONFIG_INCLUDE_INIT(enable) { STRING_LIST_INIT_DUP, (enable) } + typedef int (*config_fn_t)(const char *, const char *, void *); extern int git_default_config(const char *, const char *, void *); -extern int git_config_from_file(config_fn_t fn, const char *, void *); +extern int git_config_from_file(config_fn_t fn, const char *, void *, + struct config_include_context *); 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_from_parameters(config_fn_t fn, void *data, + struct config_include_context *inc); extern int git_config(config_fn_t fn, void *); -extern int git_config_early(config_fn_t fn, void *, const char *repo_config); +extern int git_config_early(config_fn_t fn, void *, const char *repo_config, + struct config_include_context *inc); extern int git_parse_ulong(const char *, unsigned long *); extern int git_config_int(const char *, const char *); extern unsigned long git_config_ulong(const char *, const char *); @@ -1137,7 +1147,8 @@ extern int config_error_nonbool(const char *); extern const char *get_log_output_encoding(void); extern const char *get_commit_output_encoding(void); -extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data); +extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data, + struct config_include_context *inc); extern const char *config_exclusive_filename; diff --git a/config.c b/config.c index 40f9c6d..9b90751 100644 --- a/config.c +++ b/config.c @@ -28,6 +28,71 @@ static int zlib_compression_seen; const char *config_exclusive_filename = NULL; +static int check_and_mark_include_path(struct config_include_context *inc, + const char *path) +{ + const char *canonical; + + if (!inc || !inc->enabled) + return 0; + + /* + * Using real_path would catch more duplicates, but we can't use it + * here. It will die if some path components don't exist, and we have + * no promise from the path in question actually exists. + */ + canonical = absolute_path(path); + if (unsorted_string_list_lookup(&inc->seen_paths, canonical)) + return 1; + + string_list_append(&inc->seen_paths, canonical); + return 0; +} + +static int handle_path_include(const char *path, config_fn_t fn, void *data, + struct config_include_context *inc) +{ + int ret = 0; + struct strbuf buf = STRBUF_INIT; + + /* + * Use an absolute path as-is, but interpret relative paths + * based on the including config file. + */ + if (!is_absolute_path(path)) { + char *slash; + + if (!cf || !cf->name) + return error("relative config includes must from from files"); + + slash = find_last_dir_sep(cf->name); + if (slash) + strbuf_add(&buf, cf->name, slash - cf->name + 1); + strbuf_addf(&buf, "%s", path); + path = buf.buf; + } + + if (!access(path, R_OK)) + ret = git_config_from_file(fn, path, data, inc); + strbuf_release(&buf); + return ret; +} + +static int handle_config_variable(const char *var, const char *value, + config_fn_t fn, void *data, + struct config_include_context *inc) +{ + int r; + + r = fn(var, value, data); + if (!r && inc && inc->enabled && value && !prefixcmp(var, "include.")) { + const char *type = var + 8; + if (!strcmp(type, "path")) + r = handle_path_include(value, fn, data, inc); + } + return r; +} + static void lowercase(char *p) { for (; *p; p++) @@ -47,8 +112,8 @@ void git_config_push_parameter(const char *text) strbuf_release(&env); } -int git_config_parse_parameter(const char *text, - config_fn_t fn, void *data) +int git_config_parse_parameter(const char *text, config_fn_t fn, void *data, + struct config_include_context *inc) { struct strbuf **pair; pair = strbuf_split_str(text, '=', 2); @@ -62,7 +127,10 @@ int git_config_parse_parameter(const char *text, return error("bogus config parameter: %s", text); } lowercase(pair[0]->buf); - if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) { + + if (handle_config_variable(pair[0]->buf, + pair[1] ? pair[1]->buf : NULL, + fn, data, inc) < 0) { strbuf_list_free(pair); return -1; } @@ -70,7 +138,8 @@ int git_config_parse_parameter(const char *text, return 0; } -int git_config_from_parameters(config_fn_t fn, void *data) +int git_config_from_parameters(config_fn_t fn, void *data, + struct config_include_context *inc) { const char *env = getenv(CONFIG_DATA_ENVIRONMENT); char *envw; @@ -89,7 +158,7 @@ int git_config_from_parameters(config_fn_t fn, void *data) } for (i = 0; i < nr; i++) { - if (git_config_parse_parameter(argv[i], fn, data) < 0) { + if (git_config_parse_parameter(argv[i], fn, data, inc) < 0) { free(argv); free(envw); return -1; @@ -191,7 +260,8 @@ static inline int iskeychar(int c) return isalnum(c) || c == '-'; } -static int get_value(config_fn_t fn, void *data, char *name, unsigned int len) +static int get_value(config_fn_t fn, void *data, char *name, unsigned int len, + struct config_include_context *inc) { int c; char *value; @@ -219,7 +289,8 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len) if (!value) return -1; } - return fn(name, value, data); + + return handle_config_variable(name, value, fn, data, inc); } static int get_extended_base_var(char *name, int baselen, int c) @@ -277,7 +348,8 @@ static int get_base_var(char *name) } } -static int git_parse_file(config_fn_t fn, void *data) +static int git_parse_file(config_fn_t fn, void *data, + struct config_include_context *inc) { int comment = 0; int baselen = 0; @@ -327,7 +399,7 @@ static int git_parse_file(config_fn_t fn, void *data) if (!isalpha(c)) break; var[baselen] = tolower(c); - if (get_value(fn, data, var, baselen+1) < 0) + if (get_value(fn, data, var, baselen+1, inc) < 0) break; } die("bad config file line %d in %s", cf->linenr, cf->name); @@ -826,11 +898,15 @@ int git_default_config(const char *var, const char *value, void *dummy) return 0; } -int git_config_from_file(config_fn_t fn, const char *filename, void *data) +int git_config_from_file(config_fn_t fn, const char *filename, void *data, + struct config_include_context *inc) { int ret; FILE *f = fopen(filename, "r"); + if (check_and_mark_include_path(inc, filename)) + return 0; + ret = -1; if (f) { config_file top; @@ -844,7 +920,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) strbuf_init(&top.value, 1024); cf = ⊤ - ret = git_parse_file(fn, data); + ret = git_parse_file(fn, data, inc); /* pop config-file parsing state stack */ strbuf_release(&top.value); @@ -874,17 +950,17 @@ int git_config_system(void) return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0); } -int git_config_early(config_fn_t fn, void *data, const char *repo_config) +int git_config_early(config_fn_t fn, void *data, const char *repo_config, + struct config_include_context *inc) { 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); + return git_config_from_file(fn, config_exclusive_filename, data, NULL); if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) { - ret += git_config_from_file(fn, git_etc_gitconfig(), - data); + ret += git_config_from_file(fn, git_etc_gitconfig(), data, inc); found += 1; } @@ -893,17 +969,17 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config) char buf[PATH_MAX]; char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home); if (!access(user_config, R_OK)) { - ret += git_config_from_file(fn, user_config, data); + ret += git_config_from_file(fn, user_config, data, inc); found += 1; } } if (repo_config && !access(repo_config, R_OK)) { - ret += git_config_from_file(fn, repo_config, data); + ret += git_config_from_file(fn, repo_config, data, inc); found += 1; } - switch (git_config_from_parameters(fn, data)) { + switch (git_config_from_parameters(fn, data, inc)) { case -1: /* error */ die("unable to parse command-line config"); break; @@ -921,11 +997,13 @@ int git_config(config_fn_t fn, void *data) { char *repo_config = NULL; int ret; + struct config_include_context inc = CONFIG_INCLUDE_INIT(1); repo_config = git_pathdup("config"); - ret = git_config_early(fn, data, repo_config); + ret = git_config_early(fn, data, repo_config, &inc); if (repo_config) free(repo_config); + string_list_clear(&inc.seen_paths, 0); return ret; } @@ -1313,7 +1391,7 @@ int git_config_set_multivar_in_file(const char *config_filename, * As a side effect, we make sure to transform only a valid * existing config file. */ - if (git_config_from_file(store_aux, config_filename, NULL)) { + if (git_config_from_file(store_aux, config_filename, NULL, NULL)) { error("invalid config file %s", config_filename); free(store.key); if (store.value_regex != NULL) { diff --git a/sequencer.c b/sequencer.c index 5fcbcb8..50562b9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -636,7 +636,7 @@ static void read_populate_opts(struct replay_opts **opts_ptr) if (!file_exists(opts_file)) return; - if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0) + if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr, NULL) < 0) die(_("Malformed options sheet: %s"), opts_file); } diff --git a/setup.c b/setup.c index 61c22e6..947fa51 100644 --- a/setup.c +++ b/setup.c @@ -329,7 +329,7 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok) * is a good one. */ snprintf(repo_config, PATH_MAX, "%s/config", gitdir); - git_config_early(check_repository_format_version, NULL, repo_config); + git_config_early(check_repository_format_version, NULL, repo_config, NULL); if (GIT_REPO_VERSION < repository_format_version) { if (!nongit_ok) die ("Expected git repo version <= %d, found %d", diff --git a/submodule.c b/submodule.c index 9a28060..4c5b9be 100644 --- a/submodule.c +++ b/submodule.c @@ -116,7 +116,7 @@ void gitmodules_config(void) } if (!gitmodules_is_unmerged) - git_config_from_file(submodule_config, gitmodules_path.buf, NULL); + git_config_from_file(submodule_config, gitmodules_path.buf, NULL, NULL); strbuf_release(&gitmodules_path); } } diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh new file mode 100755 index 0000000..3d5a46d --- /dev/null +++ b/t/t1305-config-include.sh @@ -0,0 +1,145 @@ +#!/bin/sh + +test_description='test config file include directives' +. ./test-lib.sh + +test_expect_success 'include file by absolute path' ' + echo "[test]one = 1" >one && + echo "[include]path = \"$PWD/one\"" >.gitconfig && + echo 1 >expect && + git config test.one >actual && + test_cmp expect actual +' + +test_expect_success 'include file by relative path' ' + echo "[test]one = 1" >one && + echo "[include]path = one" >.gitconfig && + echo 1 >expect && + git config test.one >actual && + test_cmp expect actual +' + +test_expect_success 'chained relative paths' ' + mkdir subdir && + echo "[test]three = 3" >subdir/three && + echo "[include]path = three" >subdir/two && + echo "[include]path = subdir/two" >.gitconfig && + echo 3 >expect && + git config test.three >actual && + test_cmp expect actual +' + +test_expect_success 'include options can still be examined' ' + echo "[test]one = 1" >one && + echo "[include]path = one" >.gitconfig && + echo one >expect && + git config include.path >actual && + test_cmp expect actual +' + +test_expect_success 'listing includes option and expansion' ' + echo "[test]one = 1" >one && + echo "[include]path = one" >.gitconfig && + cat >expect <<-\EOF && + include.path=one + test.one=1 + EOF + git config --list >actual.full && + grep -v ^core actual.full >actual && + test_cmp expect actual +' + +test_expect_success 'single file lookup does not expand includes by default' ' + echo "[test]one = 1" >one && + echo "[include]path = one" >.gitconfig && + test_must_fail git config -f .gitconfig test.one && + test_must_fail git config --global test.one && + echo 1 >expect && + git config --includes -f .gitconfig test.one >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 && + git config test.two 2 && + echo 2 >expect && + git config --no-includes test.two >actual && + test_cmp expect actual && + test_must_fail git config --no-includes test.one +' + +test_expect_success 'config modification does not affect includes' ' + echo "[test]one = 1" >one && + echo "[include]path = one" >.gitconfig && + git config test.one 2 && + echo 1 >expect && + git config -f one test.one >actual && + test_cmp expect actual && + cat >expect <<-\EOF && + 1 + 2 + EOF + git config --get-all test.one >actual && + test_cmp expect actual +' + +test_expect_success 'missing include files are ignored' ' + cat >.gitconfig <<-\EOF && + [include]path = foo + [test]value = yes + EOF + echo yes >expect && + git config test.value >actual && + test_cmp expect actual +' + +test_expect_success 'absolute includes from command line work' ' + echo "[test]one = 1" >one && + echo 1 >expect && + git -c include.path="$PWD/one" config test.one >actual && + test_cmp expect actual +' + +test_expect_success 'relative includes from command line fail' ' + echo "[test]one = 1" >one && + test_must_fail git -c include.path=one config test.one +' + +test_expect_success 'include cycles are detected and broken' ' + cat >.gitconfig <<-\EOF && + [test]value = gitconfig + [include]path = cycle + EOF + cat >cycle <<-\EOF && + [test]value = cycle + [include]path = .gitconfig + EOF + cat >expect <<-\EOF && + gitconfig + cycle + EOF + git config --get-all test.value >actual && + test_cmp expect actual +' + +test_expect_success 'multiple includes of the same file are suppressed' ' + cat >.gitconfig <<-\EOF && + [test] + value = base + [include] + path = A + path = B + EOF + echo "[include]path = C" >A && + echo "[include]path = C" >B && + echo "[test]value = C" >C && + cat >expect <<-\EOF && + base + C + EOF + git config --get-all test.value >actual && + test_cmp expect actual +' + +test_done -- 1.7.9.rc1.29.g43677 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-06 6:27 [PATCH 0/2] config includes, take 2 Jeff King 2012-02-06 6:29 ` [PATCH 1/2] imap-send: remove dead code Jeff King 2012-02-06 6:31 ` [PATCH 2/2] config: add include directive Jeff King @ 2012-02-06 7:41 ` Junio C Hamano 2012-02-06 9:53 ` Michael Haggerty 2012-02-07 5:01 ` David Aguilar 4 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2012-02-06 7:41 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > ... But the point of > duplicate suppression was that individual config files wouldn't have to > know or care what was being included elsewhere. I think you wanted to say "the point of inclusion mechanism" is that individual configuration files would not have to know, and I think I agree. > So I'm actually thinking I should drop the duplicate suppression and > just do some sort of sanity check on include-depth to break cycles > (i.e., just die because it's a crazy thing to do, and we are really just > trying to tell the user their config is broken rather than go into an > infinite loop). As a bonus, it makes the code much simpler, too. Yeah, I stand corrected. It was a bad suggestion. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-06 6:27 [PATCH 0/2] config includes, take 2 Jeff King ` (2 preceding siblings ...) 2012-02-06 7:41 ` [PATCH 0/2] config includes, take 2 Junio C Hamano @ 2012-02-06 9:53 ` Michael Haggerty 2012-02-06 10:06 ` Jeff King 2012-02-07 5:01 ` David Aguilar 4 siblings, 1 reply; 19+ messages in thread From: Michael Haggerty @ 2012-02-06 9:53 UTC (permalink / raw) To: Jeff King; +Cc: git On 02/06/2012 07:27 AM, Jeff King wrote: > 3. perform cycle and duplicate detection on included files > [...] > > We first read the global config, which sets the value to "global", then > includes foo, which overwrites it to "foo". Then we read the repo > config, which sets the value to "repo", and then does _not_ actually > read foo. Because git config is read linearly and later values tend to > overwrite earlier ones, we would want to suppress the _first_ instance > of a file, not the second (or really, the final if it is included many > times). But that is impossible to do without generating a complete graph > of includes and then pruning the early ones. ISTM that the main goal was to prevent infinite recursion, not avoid a little bit of redundant reading. If that is the goal, all you have to record is the "include stack" context that caused the currently-being-included file to be read and make sure that the currently-being-included file didn't appear earlier on the stack. The fact that the same file was included earlier (but not as part of the same include chain) needn't be considered an error. > [...] > So I'm actually thinking I should drop the duplicate suppression and > just do some sort of sanity check on include-depth to break cycles > (i.e., just die because it's a crazy thing to do, and we are really just > trying to tell the user their config is broken rather than go into an > infinite loop). As a bonus, it makes the code much simpler, too. I agree that it is more sensible to error out than to ignore a recursive include. It might nevertheless be useful to have an "include call stack" available for generating output for errors that occur when parsing config files; something like: Error when parsing /home/me/bogusconfigfile line 75 which was included from /home/me/okconfigfile line 13 which was included from /home/me/.gitconfig line 22 or Error: /home/me/bogusconfigfile included recursively by /home/me/bogusfile2 line 75 which was included from /home/me/bogusconfigfile line 13 which was included from /home/me/.gitconfig line 22 OTOH such error stack dumps could be generated when unwinding the C call stack without the need for a separate "include call stack". However, one could even imagine a command like $ git config --where-defined user.email user.email defined by /home/me/myfile2 line 75 which was included from /home/me/myfile1 line 13 which was included from /home/me/.gitconfig line 22 user.email redefined by /home/me/project/.git/companyconfig line 8 which was included from /home/me/project/.git/config line 20 This usage could not be implemented using the C stack, because the C stack cannot be unwound multiple times. But these are all just wild ideas. I doubt that people's config files will become so complicated that this much infrastructure is needed. Michael -- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-06 9:53 ` Michael Haggerty @ 2012-02-06 10:06 ` Jeff King 2012-02-06 10:16 ` Jeff King 0 siblings, 1 reply; 19+ messages in thread From: Jeff King @ 2012-02-06 10:06 UTC (permalink / raw) To: Michael Haggerty; +Cc: git On Mon, Feb 06, 2012 at 10:53:52AM +0100, Michael Haggerty wrote: > ISTM that the main goal was to prevent infinite recursion, not avoid a > little bit of redundant reading. It was both, actually. There was a sense that we should not end up with duplicate entries by reading the same file twice. However, entries which actually create lists (and could therefore create duplicates) are by far the minority compared to entries which overwrite. And it is the overwrite-style entries which are harmed by suppressing duplicates. > If that is the goal, all you have to record is the "include stack" > context that caused the currently-being-included file to be read and > make sure that the currently-being-included file didn't appear earlier > on the stack. The fact that the same file was included earlier (but > not as part of the same include chain) needn't be considered an error. I considered this, but decided the complexity wasn't worth it, especially because getting it accurate means cooperation from git_config_from_file, which otherwise doesn't know or care about this mechanism. Instead I keep a simple depth counter and barf at a reasonable maximum, printing the "from" and "to" files. Yes, it's not nearly as elegant as keeping the whole stack, but I really don't expect people to have complex super-deep includes, nor for it to be difficult to hunt down the cause of a cycle. Maybe that is short-sighted or lazy of me, but I'd just really be surprised if people ever went more than 1 or 2 layers deep, or if they actually ended up with a cycle at all. There is a stack kept already by git_config_from_file, but it doesn't respect the call-chain (so if I start a new git_config() call from inside a callback, it is still part of the same stack). We could possibly put a marker in the stack for that situation, and then it would be usable for include cycle-detection. > However, one could even imagine a command like > > $ git config --where-defined user.email > user.email defined by /home/me/myfile2 line 75 > which was included from /home/me/myfile1 line 13 > which was included from /home/me/.gitconfig line 22 > user.email redefined by /home/me/project/.git/companyconfig line 8 > which was included from /home/me/project/.git/config line 20 You can already implement this with the existing stack by providing a suitable callback (since its your callback, you'd know that there was no recursion of git_config, and therefore the stack refers only to a single set of includes). -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-06 10:06 ` Jeff King @ 2012-02-06 10:16 ` Jeff King 0 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2012-02-06 10:16 UTC (permalink / raw) To: Michael Haggerty; +Cc: git On Mon, Feb 06, 2012 at 05:06:22AM -0500, Jeff King wrote: > > If that is the goal, all you have to record is the "include stack" > > context that caused the currently-being-included file to be read and > > make sure that the currently-being-included file didn't appear earlier > > on the stack. The fact that the same file was included earlier (but > > not as part of the same include chain) needn't be considered an error. > > I considered this, but decided the complexity wasn't worth it, > especially because getting it accurate means cooperation from > git_config_from_file, which otherwise doesn't know or care about this > mechanism. Instead I keep a simple depth counter and barf at a > reasonable maximum, printing the "from" and "to" files. Yes, it's not > nearly as elegant as keeping the whole stack, but I really don't expect > people to have complex super-deep includes, nor for it to be difficult > to hunt down the cause of a cycle. Oh, one other thing: to detect cycles, you have to use a canonical version of the filename for comparisons. That makes the existing stack that git_config_from_file keeps useless. Using real_path is hard, because it will die() if some path components don't exist. Using absolute_path is a reasonable compromise, but it can actually miss some cycles if they involve symbolic links. Not insurmountable if you can accept teaching git_config_from_file to real_path() each file it reads. But again, it ends up getting complex for IMHO not much gain. A stupid depth counter can't show you a stack trace (and it may have false positives), but it's dirt simple and probably good enough. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-06 6:27 [PATCH 0/2] config includes, take 2 Jeff King ` (3 preceding siblings ...) 2012-02-06 9:53 ` Michael Haggerty @ 2012-02-07 5:01 ` David Aguilar 2012-02-07 5:17 ` Jeff King 4 siblings, 1 reply; 19+ messages in thread From: David Aguilar @ 2012-02-07 5:01 UTC (permalink / raw) To: Jeff King; +Cc: git On Sun, Feb 5, 2012 at 10:27 PM, Jeff King <peff@peff.net> wrote: > That leaves the file-inclusion bits: > > [1/2]: imap-send: remove dead code > [2/2]: config: add include directive > > The first patch is new in this round, and is a necessary cleanup for the > second patch (though it might be worth applying regardless). > > The second patch corresponds to patch 1/4 from the previous round. Among > the functional changes are: > > 1. do not use includes for "git config" in single-file mode I have a questions about this. Let's say I have ~/foo1.gitconfig: [foo] bar = 1 ...and ~/.gitconfig (I forgot the syntax): [foo] bar = 0 #include "~/foo1.gitconfig" Does that mean that: $ git config -f ~/.gitconfig foo.bar ...prints 0 and not 1? I can see cases where this would be undesirable behavior. For example, an application that uses "git config -z --list -f ~/.gitconfig" might expect that the result encompasses all of the user-specific config bits. Following this to its natural conclusion means "git config" might learn some kind of --no-include flag for use with e.g. "git config --no-include -f ~/.gitconfig". That said, I don't know where I would ever actually use such a flag. I do know where I would use an `--include` flag (if following includes were not the default behavior when using '-f'), though, and that's why I'm asking. The problem with not having it be the default means we have to use a flag. This makes it harder to support multiple versions of git. Maybe I'm mis-interpreting what you mean by, 'do not use includes for "git config" in single-file mode', though. -- David ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 5:01 ` David Aguilar @ 2012-02-07 5:17 ` Jeff King 2012-02-07 10:05 ` David Aguilar 0 siblings, 1 reply; 19+ messages in thread From: Jeff King @ 2012-02-07 5:17 UTC (permalink / raw) To: David Aguilar; +Cc: git On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote: > I have a questions about this. Let's say I have ~/foo1.gitconfig: > > [foo] > bar = 1 > > ...and ~/.gitconfig (I forgot the syntax): > > [foo] > bar = 0 > > #include "~/foo1.gitconfig" > > > Does that mean that: > > $ git config -f ~/.gitconfig foo.bar > > ...prints 0 and not 1? Yes, though the syntax is: [include] path = foo1.gitconfig (it doesn't respect tilde-expansion, but it probably should). Note that the syntax was specifically selected for backwards compatibility, and to allow manipulation with existing tools. > I can see cases where this would be undesirable behavior. > > For example, an application that uses "git config -z --list -f > ~/.gitconfig" might expect that the result encompasses all of the > user-specific config bits. The problem is that an application might also expect it _not_ to happen (e.g., if the application is really interested in knowing what's in ~/.gitconfig). Because includes aren't respected now, the safe default seems to be not to have includes (i.e., don't change behavior for this case). A bigger question for me is: if you are interested in getting an answer from anywhere, why are you restricting with "-f"? IOW, is this a real-world problem, or a hypothetical one? And if real-world, what is the actual use case? > Following this to its natural conclusion means "git config" might > learn some kind of --no-include flag for use with e.g. "git config > --no-include -f ~/.gitconfig". That said, I don't know where I would > ever actually use such a flag. It already learned it as part of my series (and --includes, as well). > I do know where I would use an `--include` flag (if following includes > were not the default behavior when using '-f'), though, and that's why > I'm asking. The problem with not having it be the default means we > have to use a flag. This makes it harder to support multiple versions > of git. Yes, that's a general problem of adding new command-line options to turn features off or on. We could add an environment variable to control this feature. But I'd really like to hear the compelling use case first (i.e., both why it cannot simply be spelled "git config -z --list", and why not following includes is not OK). > Maybe I'm mis-interpreting what you mean by, 'do not use includes for > "git config" in single-file mode', though. No, I think you understand the described behavior. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 5:17 ` Jeff King @ 2012-02-07 10:05 ` David Aguilar 2012-02-07 17:30 ` Jeff King 0 siblings, 1 reply; 19+ messages in thread From: David Aguilar @ 2012-02-07 10:05 UTC (permalink / raw) To: Jeff King; +Cc: git On Mon, Feb 6, 2012 at 9:17 PM, Jeff King <peff@peff.net> wrote: > On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote: > >> I have a questions about this. Let's say I have ~/foo1.gitconfig: >> >> [foo] >> bar = 1 >> >> ...and ~/.gitconfig (I forgot the syntax): >> >> [foo] >> bar = 0 >> >> #include "~/foo1.gitconfig" >> >> >> Does that mean that: >> >> $ git config -f ~/.gitconfig foo.bar >> >> ...prints 0 and not 1? > > Yes, though the syntax is: > > [include] > path = foo1.gitconfig > > (it doesn't respect tilde-expansion, but it probably should). Note that > the syntax was specifically selected for backwards compatibility, and to > allow manipulation with existing tools. > >> I can see cases where this would be undesirable behavior. >> >> For example, an application that uses "git config -z --list -f >> ~/.gitconfig" might expect that the result encompasses all of the >> user-specific config bits. > > The problem is that an application might also expect it _not_ to happen > (e.g., if the application is really interested in knowing what's in > ~/.gitconfig). Because includes aren't respected now, the safe default > seems to be not to have includes (i.e., don't change behavior for this > case). > > A bigger question for me is: if you are interested in getting an answer > from anywhere, why are you restricting with "-f"? IOW, is this a > real-world problem, or a hypothetical one? And if real-world, what is > the actual use case? It's a real-world problem. I haven't had to change the code in a while which is why this thread caught my eye ;-) I won't claim it's the best solution, but this has worked extremely well in practice: https://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py Here's the problem. I need to know which config items are "user" config (~/.gitconfig), which are "repo" config (.git/config), and which are "system" config (/etc/gitconfig). I also need to avoid calling "git config" too many times so I query these files once and cache it all in memory. We need only stat() these files to know whether to redo the "git config" call. Figuring out the repo config is tricky. You can't just say "git config --list" because that includes the user config stuff in ~/.gitconfig. Figuring out the user config is easy because you can say "git config --global --list". This is inconsistent with the behavior for "git config --list" because it does not include the --system config, which one would expect given the overlay semantics. Figuring out the system config can be done with --system. That works fine. The generic interface for getting a concise listing of values from these sources is to use "git config -f" on ~/.gitconfig, .git/config, and $(prefix)/etc/gitconfig. git config --global and git config --system are both consistent in that they return just the information relevant to them. Is --global just a shortcut for "-f ~/.gitconfig"? If yes, then does that mean "git config --global" will not honor includes? If it is not a shortcut, does that mean that "git config --global" and "git config -f ~/.gitconfig" are not really the same thing? The documentation does lead one to believe that they should be equivalent... The takeaway is that querying these files provides a convenient way to access the effective configuration values for the system, user, and repo. >> I do know where I would use an `--include` flag (if following includes >> were not the default behavior when using '-f'), though, and that's why >> I'm asking. The problem with not having it be the default means we >> have to use a flag. This makes it harder to support multiple versions >> of git. > > Yes, that's a general problem of adding new command-line options to turn > features off or on. We could add an environment variable to control this > feature. But I'd really like to hear the compelling use case first > (i.e., both why it cannot simply be spelled "git config -z --list", and > why not following includes is not OK). Hopefully my explanation conveys why "git config -z --list" is insufficient. Basically, I've been relying on the fact that querying each of these files gives me exactly what users have effectively configured. Pushing knowledge of includes onto the application is less fun. Applications would either have to understand the [include] path = ... construct and do something with it themselves or they'd have to check the git version at runtime. I would rather do without the runtime version check and have everything "keep working". So let's explore why I think it should follow includes. Here are a few reasons... When the file format supports including other files then the most logical thing for it to do is to follow those includes, no? I know, this is a tautology ;-). I'll try again. From a naive user's POV -- I am asking "git config" to evaluate this file. I don't care what's inside of it; it's a black box to me. All I know is that I get config values out the other side. Sometimes git respects the included files. Sometimes it does not (when using -f). ^^^^^^^^^ I think this subtle difference in behavior is best avoided. Here's another. The naive user will do: `git config --list -f file`. They will then edit that file to add an include statement. They will then run `git config --list -f file` again and be confused as to why their included configuration is not honored. Right now these two are synonymous: "git config --system --list" and "git config -f $(prefix)/etc/gitconfig --list". Having one form honor includes and not the other is inconsistent. What if we frame this the other way around -- when would we not want it to follow includes? I can imagine someone writing a "git config" editing program, but I think that use case is rare (and they haven't spoken up, either ;-). Well.. okay. My use case is rare too! The dumb "cache the values behind a stat() call" thing has worked really well in practice so I've been happy with it so far. It's dumb, and it won't notice when included files change, but I really don't care because it solves the 99% case. I'd be happy to rewrite it using another approach. The overlay-semantics inconsistency is what led me to using the generic "-f" interface. Performance issues led me to use a stat cache. Using each file's stat info as the cache key mapped nicely onto the generic approach of using "-f". Rewriting it (for me) would mean using --global and --system and probably not caring so much about the distinction of repo vs. user configuration (or rather, deal with the fact that `git config --list` returns both the user config and the repo config). At that point I might as well just write (reuse) a proper .gitconfig parser, but what's the fun in that when "git config" is so nice and easy to parse? ;-) -- David ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 10:05 ` David Aguilar @ 2012-02-07 17:30 ` Jeff King 2012-02-07 18:03 ` Junio C Hamano 2012-02-07 19:16 ` Jakub Narebski 0 siblings, 2 replies; 19+ messages in thread From: Jeff King @ 2012-02-07 17:30 UTC (permalink / raw) To: David Aguilar; +Cc: git On Tue, Feb 07, 2012 at 02:05:34AM -0800, David Aguilar wrote: > I won't claim it's the best solution, but this has worked extremely > well in practice: > > https://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py > > Here's the problem. I need to know which config items are "user" > config (~/.gitconfig), which are "repo" config (.git/config), and > which are "system" config (/etc/gitconfig). I'm not a git cola user. But from peeking at that code and a brief "apt-get install git-cola && git cola", it looks like the point is to show a dialog with per-repo and per-user settings, let the user tweak them, and save them back to the appropriate spot. But how does that interact with includes? Let's imagine I do this: git config --global include.path .gitidentity git config -f ~/.gitidentity user.name "Jeff King" Without having git-cola expand includes, your call to "git config --global --list" will not show the user.name variable, and it will appear blank. Which is not great. If git-cola does expand includes, then the name will appear. But what will happen when I modify that field and try to save it? It will save it using "git config --global", putting it into ~/.gitconfig. So now you'll have two names in your config, one in .gitconfig and one in .gitidentity. And which one will take precedence? That depends on what was in your config file before. If there was a [user] section before the include already in your .gitconfig, then the newly written value will go in that section _before_ the include, and the include will still take precedence. Otherwise, a new section will be written _after_ the include, and that will take precedence. So...yuck. It's kind of a complex and messy situation. But the important thing is that when you are looking at a bidirectional editing situation like this, blindly expanding includes is not going to solve the problem. It's just going to paper over the complexity. In the long term, something like git-cola that is trying to present and edit config to the user would need to learn about includes, track the sources of each variable, and then edit appropriately. You can do that with my current patch by manually following include.path directives. However, that's a pain. Git-config could potentially help with that (and even simplify the current code) by allowing something like: $ git config --list-with-sources /home/peff/.gitconfig user.name=Jeff King /home/peff/.gitconfig user.email=peff@peff.net .git/config core.repositoryformatversion=0 .git/config core.bare=false [etc] (you would use the "-z" form, of course, and the filenames would be NUL-separated, but I made up a human-readable output format above for illustration purposes). And then it would be natural to mention included files there, and you could actually modify the file that contains the value in question. Of course, you might not have _access_ to the file in question. You could be pulling in options from a shared config file, and the right thing to do is not to edit the shared file, but to override it. And that raises the question of where, and how to tell git-config to make sure that the option goes _after_ the include. So fundamentally, includes make the idea of "overwrite this value" much more complex. It's possible that git-config should learn all of this complexity, and that git cola could rely on git-config being smart. But I was rather hoping that most of the automated tools could remain simple and stupid, and that people who wanted to do complex things with includes would be left to their own devices (i.e., using $EDITOR, or smartly using "git config -f" on their individual files). > Figuring out the repo config is tricky. You can't just say "git > config --list" because that includes the user config stuff in > ~/.gitconfig. > > Figuring out the user config is easy because you can say "git config > --global --list". This is inconsistent with the behavior for "git > config --list" because it does not include the --system config, which > one would expect given the overlay semantics. I think you are mistaking a lack of source options for "look at the repo config". But without providing a source, the request is "look everywhere that git knows about". With "--global", it's about "look at this one file" (just as "--system" is about "look at this other file"). So I think what you are really lamenting is the lack of "git config --repo", which would basically be: git config --list --file="$(git rev-parse --git-dir)/config" That might be something we could improve in the documentation. > The generic interface for getting a concise listing of values from > these sources is to use "git config -f" on ~/.gitconfig, .git/config, > and $(prefix)/etc/gitconfig. Right. And --global and --system are basically just synonyms for the prior two. > git config --global and git config --system are both consistent in > that they return just the information relevant to them. Is --global > just a shortcut for "-f ~/.gitconfig"? If yes, then does that mean > "git config --global" will not honor includes? If it is not a > shortcut, does that mean that "git config --global" and "git config -f > ~/.gitconfig" are not really the same thing? The documentation does > lead one to believe that they should be equivalent... Yes, it is a shortcut, and it follows the same code path (i.e., we expand "--global" into "-f ~/.gitconfig" early on). > > Yes, that's a general problem of adding new command-line options to turn > > features off or on. We could add an environment variable to control this > > feature. But I'd really like to hear the compelling use case first > > (i.e., both why it cannot simply be spelled "git config -z --list", and > > why not following includes is not OK). > > Hopefully my explanation conveys why "git config -z --list" is insufficient. Yes, I agree it's insufficient in the face of includes. Unfortunately, I don't think simply expanding includes by default is sufficient, either. Probably the simplest solution for this case would be something like: 1. Always expand includes on listing. 2. When editing, always override items in includes. So do _not_ reuse sections, but always put the values at the end of the file (or at least after any includes). That's not as nice as doing it "right" (you'll end up with duplicated values in included files and their parents, and sections may appear multiple times). But it's simple and should have the desired semantics. > From a naive user's POV -- > I am asking "git config" to evaluate this file. I don't care what's > inside of it; it's a black box to me. All I know is that I get config > values out the other side. Sometimes git respects the included files. > Sometimes it does not (when using -f). > ^^^^^^^^^ > I think this subtle difference in behavior is best avoided. It is not "sometimes when I ask git config to evaluate this file". It is "git never expands includes when you ask it to evaluate a file". Includes are expanded only when you ask git "search all files you know about" (i.e., when you do not provide a file argument). There is also a more complex issue with "-f", which is that git-config is used not just to parse git configuration, but also to parse other config-like files that should not respect includes (e.g., ".gitmodules"). So to remain backwards-compatible, we need the default for "-f" to not respect includes, and that is not really negotiable. We could convert "--global" and "--system" to respect includes by default (since we at least know these are actual git config files). But that is introducing a subtle difference, which you complained about above. But if scripted uses like "git cola" are the ones that need it, I'd rather provide a backwards-compatible way of tweaking the behavior (if a command line option is not sufficient, then I'd rather provide an environment variable). > Here's another. The naive user will do: `git config --list -f file`. > They will then edit that file to add an include statement. They will > then run `git config --list -f file` again and be confused as to why > their included configuration is not honored. As I mentioned above, that one is not really negotiable. But it seems like it is a mismatch of expectations to behavior, and that is something we can at least address with documentation. > Right now these two are synonymous: "git config --system --list" and > "git config -f $(prefix)/etc/gitconfig --list". Having one form honor > includes and not the other is inconsistent. With my patch, they are consistent, and neither should honor includes (that being said, there is a bug in my patch which makes --list honor includes in all cases; however, you are responding correctly to the intended and advertised behavior of my patch, and that is what we should be discussing). > What if we frame this the other way around -- when would we not want > it to follow includes? I can imagine someone writing a "git config" > editing program, but I think that use case is rare (and they haven't > spoken up, either ;-). But isn't "git cola" a git-config editing program? > Well.. okay. My use case is rare too! The dumb "cache the values > behind a stat() call" thing has worked really well in practice so I've > been happy with it so far. It's dumb, and it won't notice when > included files change, but I really don't care because it solves the > 99% case. I don't get the caching. How often would you need to call "git config"? Isn't is only when the edit-config dialog appears? Is it really that expensive to call when opening a user-editable dialog? > Rewriting it (for me) would mean using --global and --system and > probably not caring so much about the distinction of repo vs. user > configuration (or rather, deal with the fact that `git config --list` > returns both the user config and the repo config). At that point I > might as well just write (reuse) a proper .gitconfig parser, but > what's the fun in that when "git config" is so nice and easy to parse? I definitely feel your pain. I don't want you to have to rewrite yet another .gitconfig parser. Let's see if we can figure out reasonable editing semantics that can go into git-config. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 17:30 ` Jeff King @ 2012-02-07 18:03 ` Junio C Hamano 2012-02-07 18:29 ` Jeff King 2012-02-07 19:16 ` Jakub Narebski 1 sibling, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2012-02-07 18:03 UTC (permalink / raw) To: Jeff King; +Cc: David Aguilar, git Jeff King <peff@peff.net> writes: > So fundamentally, includes make the idea of "overwrite this value" much > more complex. I do not think it is anything new that include.path brings. To give the user a complete view of "what customization applies when working in this repository?", and to allow the user to edit variables in the right file among usual three places, the tool already needs to be aware of possible places and give users a way to tell where the edit needs to go anyway. include.path only adds one more thing for the tool to be aware of. With your example, the editor can show git config -f .git/config --list with "include.path=~/.gitident" listed as one of the key=value pairs without showing key=value pairs included from that file. Or it can show user.name in effect is this value from .git/config, and optionally also show that there are other definitions of user.name in ~/.gitconfig (which we use as if we have "include.path=~/.gitconfig" at the top of .git/config file) or ~/.gitident specified with include.path. The tool needs to make it easy to jump to ~/.gitident; it needs to know what include.path means. The user can edit the value in ~/.gitident, or after looking at ~/.gitident, choose to come back to .git/config and add an overriding entry there. > But isn't "git cola" a git-config editing program? Yes, that is really the right "rhetorical" question to ask. It needs to know what it is editing, so it at least needs to be _aware_ of the inclusion, where each item comes from, and how to edit contents of _one_ particular file. And that issue is not something new that was introduced by include.path. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 18:03 ` Junio C Hamano @ 2012-02-07 18:29 ` Jeff King 0 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2012-02-07 18:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, git On Tue, Feb 07, 2012 at 10:03:26AM -0800, Junio C Hamano wrote: > > But isn't "git cola" a git-config editing program? > > Yes, that is really the right "rhetorical" question to ask. It needs to > know what it is editing, so it at least needs to be _aware_ of the > inclusion, where each item comes from, and how to edit contents of _one_ > particular file. > > And that issue is not something new that was introduced by include.path. True, but it was made more complex. David has already dealt with the 3 sources in his code. Adding includes expands this to N sources, and his code needs to adapt. It would be nice if we could at least make the adaptation backwards-compatible and as painless as possible. But I just see the "right" answer in a complex case needing user input that a simple call to "git config" can't provide. That is, pretending that "git config --global" is still a single file for reading and writing can get you something that _works_, but it will make a mess of the user's config. The most elegant thing to me is to expand git-cola's config editor into an N-pane editor instead of a 2-pane editor (not 3, because it doesn't make sense to edit /etc/gitconfig with it, and nor does the current version support it). People without includes wouldn't notice the difference, and people with includes might appreciate the power and flexibility. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 17:30 ` Jeff King 2012-02-07 18:03 ` Junio C Hamano @ 2012-02-07 19:16 ` Jakub Narebski 2012-02-07 19:21 ` Jeff King ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: Jakub Narebski @ 2012-02-07 19:16 UTC (permalink / raw) To: Jeff King; +Cc: David Aguilar, git Jeff King <peff@peff.net> writes: [...] > Git-config could potentially help with that (and even simplify the > current code) by allowing something like: > > $ git config --list-with-sources > /home/peff/.gitconfig user.name=Jeff King > /home/peff/.gitconfig user.email=peff@peff.net > .git/config core.repositoryformatversion=0 > .git/config core.bare=false > [etc] > > (you would use the "-z" form, of course, and the filenames would be > NUL-separated, but I made up a human-readable output format above for > illustration purposes). That would be _very_ nice to have (even without includes support). Filenames would be git-quoted like in ls-tree / diff-tree output without -z, isn't it? And is that TAB or SPC as a separator? -- Jakub Narebski ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 19:16 ` Jakub Narebski @ 2012-02-07 19:21 ` Jeff King 2012-02-07 20:15 ` David Aguilar 2012-02-09 3:30 ` Jeff King 2 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2012-02-07 19:21 UTC (permalink / raw) To: Jakub Narebski; +Cc: David Aguilar, git On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote: > > Git-config could potentially help with that (and even simplify the > > current code) by allowing something like: > > > > $ git config --list-with-sources > > /home/peff/.gitconfig user.name=Jeff King > > /home/peff/.gitconfig user.email=peff@peff.net > > .git/config core.repositoryformatversion=0 > > .git/config core.bare=false > > [etc] > > > > (you would use the "-z" form, of course, and the filenames would be > > NUL-separated, but I made up a human-readable output format above for > > illustration purposes). > > That would be _very_ nice to have (even without includes support). > > Filenames would be git-quoted like in ls-tree / diff-tree output without -z, > isn't it? And is that TAB or SPC as a separator? Yeah, the output above is just illustrative. Without -z, I would do quoted filename, TAB, and then $KEY=$VALUE (I didn't check whether we quote an "=" in the subsection of a key, but we probably should). -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 19:16 ` Jakub Narebski 2012-02-07 19:21 ` Jeff King @ 2012-02-07 20:15 ` David Aguilar 2012-02-09 3:30 ` Jeff King 2 siblings, 0 replies; 19+ messages in thread From: David Aguilar @ 2012-02-07 20:15 UTC (permalink / raw) To: Jakub Narebski, Jeff King; +Cc: git On Tue, Feb 7, 2012 at 11:16 AM, Jakub Narebski <jnareb@gmail.com> wrote: > Jeff King <peff@peff.net> writes: > > [...] >> Git-config could potentially help with that (and even simplify the >> current code) by allowing something like: >> >> $ git config --list-with-sources >> /home/peff/.gitconfig user.name=Jeff King >> /home/peff/.gitconfig user.email=peff@peff.net >> .git/config core.repositoryformatversion=0 >> .git/config core.bare=false >> [etc] >> >> (you would use the "-z" form, of course, and the filenames would be >> NUL-separated, but I made up a human-readable output format above for >> illustration purposes). > > That would be _very_ nice to have (even without includes support). I like this as well. Thanks for digging deep into this one, Jeff. You've convinced me that not following includes is the better default behavior. You are correct in mentioning that what was really missing was something akin to "git config --repo --list". Since that command would be a shortcut for "-f .git/config" then it would be consistent in behavior. The suggestion to have the app understand .[include] path = and show separate panes for included files is an elegant solution and definitely helpful for power users. I do have to write more code but that's fine since it enables new functionality. Jeff, you mentioned possibly adding a "backwards-compatible way" of accessing this stuff and hiding it behind an environment variable. I don't want to make us carry around backwards compat code paths just for one particular use case so perhaps the best thing would be for me to start preparing for this change now. I already have various places where functionality is guarded behind a git version check. If what we're talking about is git-cola adding "--include" when git >= 1.8(?) then that works for me. It should be noted that git-gui also uses `git config --global --list` so I don't know if this has implications there. E.g. maybe things like user.name won't be overridden if done via an included config there. RE: the caching -- we call git config a few times in various places. Getting the user.* fields. Getting the diff.summary settings, etc. I started tweaking app startup for speed and noticed all the git-config calls and was able to replace a handful of calls with a single call, which was nice. The difference is more pronounced on win32 (I am a linux user but I do try to play nice with the msysgit folks). Thanks Jeff, -- David ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-07 19:16 ` Jakub Narebski 2012-02-07 19:21 ` Jeff King 2012-02-07 20:15 ` David Aguilar @ 2012-02-09 3:30 ` Jeff King 2012-02-09 19:24 ` Jakub Narebski 2 siblings, 1 reply; 19+ messages in thread From: Jeff King @ 2012-02-09 3:30 UTC (permalink / raw) To: Jakub Narebski; +Cc: David Aguilar, git On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote: > Jeff King <peff@peff.net> writes: > > [...] > > Git-config could potentially help with that (and even simplify the > > current code) by allowing something like: > > > > $ git config --list-with-sources > > /home/peff/.gitconfig user.name=Jeff King > > /home/peff/.gitconfig user.email=peff@peff.net > > .git/config core.repositoryformatversion=0 > > .git/config core.bare=false > > [etc] > > > > (you would use the "-z" form, of course, and the filenames would be > > NUL-separated, but I made up a human-readable output format above for > > illustration purposes). > > That would be _very_ nice to have (even without includes support). > > Filenames would be git-quoted like in ls-tree / diff-tree output without -z, > isn't it? And is that TAB or SPC as a separator? So the patch would look something like this. However, is the actual filename really what callers want? It seems like in David's case, an annotation of "repo", "global", or "system" (possibly in addition to the filename) would be the most useful (because in the git-cola UI, it is still nice to list things as "repo" or "global" instead of spewing the whole filename at the user -- but you would still want the individual filename for handling updates of includes). --- builtin/config.c | 20 ++++++++++++++++++-- cache.h | 1 + config.c | 7 +++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index d35c06a..e0333f7 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -2,6 +2,7 @@ #include "cache.h" #include "color.h" #include "parse-options.h" +#include "quote.h" static const char *const builtin_config_usage[] = { "git config [options]", @@ -41,6 +42,7 @@ static int end_null; #define ACTION_SET_ALL (1<<12) #define ACTION_GET_COLOR (1<<13) #define ACTION_GET_COLORBOOL (1<<14) +#define ACTION_LIST_WITH_SOURCES (1<<15) #define TYPE_BOOL (1<<0) #define TYPE_INT (1<<1) @@ -64,6 +66,7 @@ static struct option builtin_config_options[] = { OPT_BIT(0, "rename-section", &actions, "rename section: old-name new-name", ACTION_RENAME_SECTION), OPT_BIT(0, "remove-section", &actions, "remove a section: name", ACTION_REMOVE_SECTION), OPT_BIT('l', "list", &actions, "list all", ACTION_LIST), + OPT_BIT(0, "list-with-sources", &actions, "list all variables with sources", ACTION_LIST_WITH_SOURCES), OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT), OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"), OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"), @@ -86,6 +89,18 @@ static void check_argc(int argc, int min, int max) { static int show_all_config(const char *key_, const char *value_, void *cb) { + int show_sources = *(int *)cb; + + if (show_sources) { + const char *fn = current_config_filename(); + if (!fn) + fn = ""; + if (end_null) + printf("%s%c", fn, '\0'); + else + write_name_quoted(fn, stdout, '\t'); + } + if (value_) printf("%s%c%s%c", key_, delim, value_, term); else @@ -418,9 +433,10 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_with_options(builtin_config_usage, builtin_config_options); } - if (actions == ACTION_LIST) { + if (actions == ACTION_LIST || actions == ACTION_LIST_WITH_SOURCES) { + int show_sources = actions & ACTION_LIST_WITH_SOURCES ? 1 : 0; check_argc(argc, 0, 0); - if (git_config(show_all_config, NULL) < 0) { + if (git_config(show_all_config, &show_sources) < 0) { if (config_exclusive_filename) die_errno("unable to read config file '%s'", config_exclusive_filename); diff --git a/cache.h b/cache.h index 9bd8c2d..98b1d09 100644 --- a/cache.h +++ b/cache.h @@ -1138,6 +1138,7 @@ extern const char *get_log_output_encoding(void); extern const char *get_commit_output_encoding(void); extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data); +extern const char *current_config_filename(void); extern const char *config_exclusive_filename; diff --git a/config.c b/config.c index 40f9c6d..7b4094b 100644 --- a/config.c +++ b/config.c @@ -1564,3 +1564,10 @@ int config_error_nonbool(const char *var) { return error("Missing value for '%s'", var); } + +const char *current_config_filename(void) +{ + if (cf && cf->name) + return cf->name; + return NULL; +} -- 1.7.9.rc2.14.g3da2b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-09 3:30 ` Jeff King @ 2012-02-09 19:24 ` Jakub Narebski 2012-02-09 19:33 ` Jeff King 0 siblings, 1 reply; 19+ messages in thread From: Jakub Narebski @ 2012-02-09 19:24 UTC (permalink / raw) To: Jeff King; +Cc: David Aguilar, git On Thu, 9 Feb 2012, Jeff King wrote: > On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote: > > Jeff King <peff@peff.net> writes: > > > > [...] > > > Git-config could potentially help with that (and even simplify the > > > current code) by allowing something like: > > > > > > $ git config --list-with-sources > > > /home/peff/.gitconfig user.name=Jeff King > > > /home/peff/.gitconfig user.email=peff@peff.net > > > .git/config core.repositoryformatversion=0 > > > .git/config core.bare=false > > > [etc] > > > > > > (you would use the "-z" form, of course, and the filenames would be > > > NUL-separated, but I made up a human-readable output format above for > > > illustration purposes). > > > > That would be _very_ nice to have (even without includes support). > > > > Filenames would be git-quoted like in ls-tree / diff-tree output without -z, > > isn't it? And is that TAB or SPC as a separator? > > So the patch would look something like this. However, is the actual > filename really what callers want? It seems like in David's case, an > annotation of "repo", "global", or "system" (possibly in addition to the > filename) would be the most useful (because in the git-cola UI, it is > still nice to list things as "repo" or "global" instead of spewing the > whole filename at the user -- but you would still want the individual > filename for handling updates of includes). I'm not sure if "system" / "global" / "local" or "repo" would be a good idea. First, in the case of includes you would have to provide pathnames of included files. This would introduce inconsistency. Is "system" the '/etc/gitconfig' file, or 'system' file in '.git' directory? Second, people can have different build configuration, e.g. the prefix might differ, so that "system" is not always '/etc/gitconfig'. If you want to edit config you would want to know which file to edit... and though there is "git config --system --edit" it depends on having editor configured correctly. Just my two cents. -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] config includes, take 2 2012-02-09 19:24 ` Jakub Narebski @ 2012-02-09 19:33 ` Jeff King 0 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2012-02-09 19:33 UTC (permalink / raw) To: Jakub Narebski; +Cc: David Aguilar, git On Thu, Feb 09, 2012 at 08:24:42PM +0100, Jakub Narebski wrote: > > So the patch would look something like this. However, is the actual > > filename really what callers want? It seems like in David's case, an > > annotation of "repo", "global", or "system" (possibly in addition to the > > filename) would be the most useful (because in the git-cola UI, it is > > still nice to list things as "repo" or "global" instead of spewing the > > whole filename at the user -- but you would still want the individual > > filename for handling updates of includes). > > I'm not sure if "system" / "global" / "local" or "repo" would be a good > idea. > > First, in the case of includes you would have to provide pathnames of > included files. This would introduce inconsistency. Is "system" > the '/etc/gitconfig' file, or 'system' file in '.git' directory? Yeah, it would have to be syntactically unambiguous with the filename. I was thinking something of just including both, like this: global:/home/peff/.gitconfig<TAB>include.path=other-file global:/home/peff/other-file<TAB>some.key=value That is, give a "context" (repo, global, system) to each lookup, and then mention the individual file as well (either because it is the root of that context, or because it was included). So a config editor could present the context to the user as a purely decorative thing (i.e., tell the user "these options affect all of your repos"), but use the filename to actually update the values (i.e., "git config -f /home/peff/other-file some.key newvalue"). > Second, people can have different build configuration, e.g. the prefix > might differ, so that "system" is not always '/etc/gitconfig'. If you > want to edit config you would want to know which file to edit... and though > there is "git config --system --edit" it depends on having editor > configured correctly. Without includes, something like git-cola could possibly get away with: git config --system some.key value but that doesn't work for included files; for those, you'd want to have the actual filename. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-02-09 19:33 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-06 6:27 [PATCH 0/2] config includes, take 2 Jeff King 2012-02-06 6:29 ` [PATCH 1/2] imap-send: remove dead code Jeff King 2012-02-06 6:31 ` [PATCH 2/2] config: add include directive Jeff King 2012-02-06 7:41 ` [PATCH 0/2] config includes, take 2 Junio C Hamano 2012-02-06 9:53 ` Michael Haggerty 2012-02-06 10:06 ` Jeff King 2012-02-06 10:16 ` Jeff King 2012-02-07 5:01 ` David Aguilar 2012-02-07 5:17 ` Jeff King 2012-02-07 10:05 ` David Aguilar 2012-02-07 17:30 ` Jeff King 2012-02-07 18:03 ` Junio C Hamano 2012-02-07 18:29 ` Jeff King 2012-02-07 19:16 ` Jakub Narebski 2012-02-07 19:21 ` Jeff King 2012-02-07 20:15 ` David Aguilar 2012-02-09 3:30 ` Jeff King 2012-02-09 19:24 ` Jakub Narebski 2012-02-09 19:33 ` Jeff King
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 NNTP newsgroup(s).