{"thread":{"id":"29559","subject":"[PATCH 0/2] config includes, take 2","startedAt":"2012-02-06T06:27:13Z","lastAt":"2012-02-09T19:33:14Z","messageCount":19,"participants":["Jeff King","Junio C Hamano","Michael Haggerty","David Aguilar","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"183982","messageId":"20120206062713.GA9699@sigill.intra.peff.net","threadId":"29559","inReplyTo":null,"subject":"[PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T06:27:13Z","receivedAt":"2012-02-06T06:27:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I decided to drop the \"read config from a ref\" bits of the series. There\nwas some debate about what the proper workflow should be around projects\nsharing config. And since I am not part of a project that does so, I\ncan't contribute anything empirical to the discussion. So I'll let that\npart live on in the list archive, and somebody whose project really\nwants to experiment with it can feel free to do so.\n\nThat leaves the file-inclusion bits:\n\n  [1/2]: imap-send: remove dead code\n  [2/2]: config: add include directive\n\nThe first patch is new in this round, and is a necessary cleanup for the\nsecond patch (though it might be worth applying regardless).\n\nThe second patch corresponds to patch 1/4 from the previous round. Among\nthe functional changes are:\n\n  1. do not use includes for \"git config\" in single-file mode\n\n  2. do not respect include.foo.path\n\n  3. perform cycle and duplicate detection on included files\n\nDoing (3) actually ended up changing the implementation a fair bit. The\noriginal implementation worked purely as a drop-in callback wrapper. But\nto suppress duplicates, you need to know not just which files you've\nincluded, but which files you started from. So either\ngit_config_from_file has to learn about the include mechanism, or each\ncaller has to manually mark the file it is about to read.\n\nI ended up just teaching the config machinery about the include\nmechanism. It meant more changes (because we now pass an extra \"include\"\nargument between the config functions) but I think turns out much more\nreadable.\n\nThat being said, I'm having doubts about the idea of duplicate\nsuppression at all. Suppose I have a setup like this:\n\n  git config -f foo test.value foo\n  git config test.value repo\n  git config include.path $PWD/foo\n  git config --global test.value global\n  git config --global include.path $PWD/foo\n\nThat is, two config files, each of which sets test.value but also\nincludes \"foo\". One might assume from this that the output of \"git\nconfig test.value\" would be \"foo\". But because of duplicate suppression,\nit's not.\n\nWe first read the global config, which sets the value to \"global\", then\nincludes foo, which overwrites it to \"foo\". Then we read the repo\nconfig, which sets the value to \"repo\", and then does _not_ actually\nread foo. Because git config is read linearly and later values tend to\noverwrite earlier ones, we would want to suppress the _first_ instance\nof a file, not the second (or really, the final if it is included many\ntimes). But that is impossible to do without generating a complete graph\nof includes and then pruning the early ones.\n\nSure, this is a trivial toy example, and by looking at all of the\ninputs, you can see what is happening and fix it. But the point of\nduplicate suppression was that individual config files wouldn't have to\nknow or care what was being included elsewhere. A slightly more\nreal-world example is this:\n\n  1. A config file \"/etc/git-defaults-foo.cfg\" sets a bunch of\n     reasonable defaults for some group of developers.\n\n  2. One of the developers really likes these foo defaults, but wants to\n     override one particular option. So he does this:\n\n       git config --global include.path /etc/git-defaults-foo.cfg\n       git config --global some.option override\n\n  3. On one particular repo, though, this dev wants the stock foo\n     defaults. So he does:\n\n       git config --global include.path /etc/git-defaults-foo.cfg\n\nBut we ignore the final include request; it's suppressed as a duplicate,\neven though it would cancel his override.\n\nSo I'm actually thinking I should drop the duplicate suppression and\njust do some sort of sanity check on include-depth to break cycles\n(i.e., just die because it's a crazy thing to do, and we are really just\ntrying to tell the user their config is broken rather than go into an\ninfinite loop). As a bonus, it makes the code much simpler, too.\n\nI'll post the patch with duplicate suppression for reference, but read\nit with a grain of salt (or don't bother to read it at all if after\nreading the above you think it should be thrown out).\n\n-Peff\n"},{"id":"183983","messageId":"20120206062937.GA9931@sigill.intra.peff.net","threadId":"29559","inReplyTo":"20120206062713.GA9699@sigill.intra.peff.net","subject":"[PATCH 1/2] imap-send: remove dead code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T06:29:37Z","receivedAt":"2012-02-06T06:29:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The imap-send code was adapted from another project, and\nstill contains many unused bits of code. One of these bits\ncontains a type \"struct string_list\" which bears no\nresemblence to the \"struct string_list\" we use elsewhere in\ngit. This causes the compiler to complain if git's\nstring_list ever becomes part of cache.h.\n\nLet's just drop the dead code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is necessary for patch 2, which does include string-list.h in\ncache.h. If you read my cover letter, I think patch 2 might not be the\nbest approach. However, I think it might be worth applying this just as\na cleanup (e.g., even without the build problems, grepping for \"struct\nstring_list\" turns up this useless cruft).\n\n imap-send.c |   23 -----------------------\n 1 files changed, 0 insertions(+), 23 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex e40125a..972ad62 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -42,28 +42,6 @@ struct store_conf {\n \tunsigned trash_remote_new:1, trash_only_new:1;\n };\n \n-struct string_list {\n-\tstruct string_list *next;\n-\tchar string[1];\n-};\n-\n-struct channel_conf {\n-\tstruct channel_conf *next;\n-\tchar *name;\n-\tstruct store_conf *master, *slave;\n-\tchar *master_name, *slave_name;\n-\tchar *sync_state;\n-\tstruct string_list *patterns;\n-\tint mops, sops;\n-\tunsigned max_messages; /* for slave only */\n-};\n-\n-struct group_conf {\n-\tstruct group_conf *next;\n-\tchar *name;\n-\tstruct string_list *channels;\n-};\n-\n /* For message->status */\n #define M_RECENT       (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */\n #define M_DEAD         (1<<1) /* expunged */\n@@ -71,7 +49,6 @@ struct group_conf {\n \n struct message {\n \tstruct message *next;\n-\t/* struct string_list *keywords; */\n \tsize_t size; /* zero implies \"not fetched\" */\n \tint uid;\n \tunsigned char flags, status;\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"183984","messageId":"20120206063154.GB9931@sigill.intra.peff.net","threadId":"29559","inReplyTo":"20120206062713.GA9699@sigill.intra.peff.net","subject":"[PATCH 2/2] config: add include directive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T06:31:54Z","receivedAt":"2012-02-06T06:31:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[If you haven't read my cover letter, do so now. The surprise ending is\n that I think this patch may not be the right approach, and I am posting\n it for reference. I don't want anybody to waste time reading and\n reviewing this without that context.]\n-- >8 --\n\nIt can be useful to split your ~/.gitconfig across multiple\nfiles. For example, you might have a \"main\" file which is\nused on many machines, but a small set of per-machine\ntweaks. Or you may want to make some of your config public\n(e.g., clever aliases) while keeping other data back (e.g.,\nyour name or other identifying information). Or you may want\nto include a number of config options in some subset of your\nrepos without copying and pasting (e.g., you want to\nreference them from the .git/config of participating repos).\n\nThis patch introduces an include directive for config files.\nIt looks like:\n\n  [include]\n    path = /path/to/file\n\nThis is syntactically backwards-compatible with existing git\nconfig parsers (i.e., they will see it as another config\nentry and ignore it unless you are looking up include.path).\n\nThe implementation adds a new \"include\" parameter to\ngit_config_from_file to turn on includes.  Include\ndirectives are turned on for \"regular\" git config parsing:\nwhen you call git_config(), or when you make a call to the\n\"git config\" program without using any single-file options.\n\nIncludes are off in other cases, including:\n\n  1. Parsing of other config-like files, like .gitmodules.\n     There isn't a real need, and I'd rather be conservative\n     and avoid unnecessary incompatibility or confusion.\n\n  2. Reading single files via \"git config\". This is for two\n     reasons:\n\n       a. backwards compatibility with scripts looking at\n          config-like files.\n\n       b. inspection of a specific file probably means you\n\t  care about just what's in that file, not a general\n          lookup for \"do we have this value anywhere at\n\t  all\". If that is not the case, the caller can\n\t  always specify \"--includes\".\n\n  3. Writing files via \"git config\"; we want to treat\n     include.* variables as literal items to be copied (or\n     modified), and not expand them. So \"git config\n     --unset-all foo.bar\" would operate _only_ on\n     .git/config, not any of its included files (just as it\n     also does not operate on ~/.gitconfig).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt     |   15 ++++\n Documentation/git-config.txt |    5 ++\n builtin/clone.c              |    2 +-\n builtin/config.c             |   19 ++++--\n builtin/init-db.c            |    2 +-\n cache.h                      |   19 ++++-\n config.c                     |  118 ++++++++++++++++++++++++++++------\n sequencer.c                  |    2 +-\n setup.c                      |    2 +-\n submodule.c                  |    2 +-\n t/t1305-config-include.sh    |  145 ++++++++++++++++++++++++++++++++++++++++++\n 11 files changed, 295 insertions(+), 36 deletions(-)\n create mode 100755 t/t1305-config-include.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex abeb82b..e55dae1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -84,6 +84,17 @@ customary UNIX fashion.\n \n Some variables may require a special value format.\n \n+Includes\n+~~~~~~~~\n+\n+You can include one config file from another by setting the special\n+`include.path` variable to the name of the file to be included. The\n+included file is expanded immediately, as if its contents had been\n+found at the location of the include directive. If the value of the\n+`include.path` variable is a relative path, the path is considered to be\n+relative to the configuration file in which the include directive was\n+found. See below for examples.\n+\n Example\n ~~~~~~~\n \n@@ -106,6 +117,10 @@ Example\n \t\tgitProxy=\"ssh\" for \"kernel.org\"\n \t\tgitProxy=default-proxy ; for the rest\n \n+\t[include]\n+\t\tpath = /path/to/foo.inc ; include by absolute path\n+\t\tpath = foo ; expand \"foo\" relative to the current file\n+\n Variables\n ~~~~~~~~~\n \ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex e7ecf5d..aa8303b 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -178,6 +178,11 @@ See also <<FILES>>.\n \tOpens an editor to modify the specified config file; either\n \t'--system', '--global', or repository (default).\n \n+--includes::\n+--no-includes::\n+\tRespect `include.*` directives in config files when looking up\n+\tvalues. Defaults to on.\n+\n [[FILES]]\n FILES\n -----\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c62d4b5..32b7808 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -614,7 +614,7 @@ static void write_config(struct string_list *config)\n \n \tfor (i = 0; i < config->nr; i++) {\n \t\tif (git_config_parse_parameter(config->items[i].string,\n-\t\t\t\t\t       write_one_config, NULL) < 0)\n+\t\t\t\t\t       write_one_config, NULL, NULL) < 0)\n \t\t\tdie(\"unable to write parameters to config file\");\n \t}\n }\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d35c06a..be5d0fb 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -25,6 +25,7 @@ static const char *given_config_file;\n static int actions, types;\n static const char *get_color_slot, *get_colorbool_slot;\n static int end_null;\n+static struct config_include_context include = CONFIG_INCLUDE_INIT(-1);\n \n #define ACTION_GET (1<<0)\n #define ACTION_GET_ALL (1<<1)\n@@ -74,6 +75,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"path\", &types, \"value is a path (file or directory name)\", TYPE_PATH),\n \tOPT_GROUP(\"Other\"),\n \tOPT_BOOLEAN('z', \"null\", &end_null, \"terminate values with NUL byte\"),\n+\tOPT_BOOL(0, \"includes\", &include.enabled, \"respect include directives on lookup\"),\n \tOPT_END(),\n };\n \n@@ -214,18 +216,18 @@ static int get_value(const char *key_, const char *regex_)\n \t}\n \n \tif (do_all && system_wide)\n-\t\tgit_config_from_file(show_config, system_wide, NULL);\n+\t\tgit_config_from_file(show_config, system_wide, NULL, &include);\n \tif (do_all && global)\n-\t\tgit_config_from_file(show_config, global, NULL);\n+\t\tgit_config_from_file(show_config, global, NULL, &include);\n \tif (do_all)\n-\t\tgit_config_from_file(show_config, local, NULL);\n-\tgit_config_from_parameters(show_config, NULL);\n+\t\tgit_config_from_file(show_config, local, NULL, &include);\n+\tgit_config_from_parameters(show_config, NULL, &include);\n \tif (!do_all && !seen)\n-\t\tgit_config_from_file(show_config, local, NULL);\n+\t\tgit_config_from_file(show_config, local, NULL, &include);\n \tif (!do_all && !seen && global)\n-\t\tgit_config_from_file(show_config, global, NULL);\n+\t\tgit_config_from_file(show_config, global, NULL, &include);\n \tif (!do_all && !seen && system_wide)\n-\t\tgit_config_from_file(show_config, system_wide, NULL);\n+\t\tgit_config_from_file(show_config, system_wide, NULL, &include);\n \n \tfree(key);\n \tif (regexp) {\n@@ -384,6 +386,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\tconfig_exclusive_filename = given_config_file;\n \t}\n \n+\tif (include.enabled == -1)\n+\t\tinclude.enabled = !config_exclusive_filename;\n+\n \tif (end_null) {\n \t\tterm = '\\0';\n \t\tdelim = '\\n';\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 0dacb8b..de6290f 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -146,7 +146,7 @@ static void copy_templates(const char *template_dir)\n \tstrcpy(template_path + template_len, \"config\");\n \trepository_format_version = 0;\n \tgit_config_from_file(check_repository_format_version,\n-\t\t\t     template_path, NULL);\n+\t\t\t     template_path, NULL, NULL);\n \ttemplate_path[template_len] = 0;\n \n \tif (repository_format_version &&\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..f75923e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -7,6 +7,7 @@\n #include \"advice.h\"\n #include \"gettext.h\"\n #include \"convert.h\"\n+#include \"string-list.h\"\n \n #include SHA1_HEADER\n #ifndef git_SHA_CTX\n@@ -1108,13 +1109,22 @@ extern int update_server_info(int);\n #define CONFIG_NOTHING_SET 5\n #define CONFIG_INVALID_PATTERN 6\n \n+struct config_include_context {\n+\tstruct string_list seen_paths;\n+\tint enabled;\n+};\n+#define CONFIG_INCLUDE_INIT(enable) { STRING_LIST_INIT_DUP, (enable) }\n+\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n-extern int git_config_from_file(config_fn_t fn, const char *, void *);\n+extern int git_config_from_file(config_fn_t fn, const char *, void *,\n+\t\t\t\tstruct config_include_context *);\n extern void git_config_push_parameter(const char *text);\n-extern int git_config_from_parameters(config_fn_t fn, void *data);\n+extern int git_config_from_parameters(config_fn_t fn, void *data,\n+\t\t\t\t      struct config_include_context *inc);\n extern int git_config(config_fn_t fn, void *);\n-extern int git_config_early(config_fn_t fn, void *, const char *repo_config);\n+extern int git_config_early(config_fn_t fn, void *, const char *repo_config,\n+\t\t\t    struct config_include_context *inc);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_config_int(const char *, const char *);\n extern unsigned long git_config_ulong(const char *, const char *);\n@@ -1137,7 +1147,8 @@ extern int config_error_nonbool(const char *);\n extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \n-extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n+extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data,\n+\t\t\t\t      struct config_include_context *inc);\n \n extern const char *config_exclusive_filename;\n \ndiff --git a/config.c b/config.c\nindex 40f9c6d..9b90751 100644\n--- a/config.c\n+++ b/config.c\n@@ -28,6 +28,71 @@ static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n \n+static int check_and_mark_include_path(struct config_include_context *inc,\n+\t\t\t\t       const char *path)\n+{\n+\tconst char *canonical;\n+\n+\tif (!inc || !inc->enabled)\n+\t\treturn 0;\n+\n+\t/*\n+\t * Using real_path would catch more duplicates, but we can't use it\n+\t * here. It will die if some path components don't exist, and we have\n+\t * no promise from the path in question actually exists.\n+\t */\n+\tcanonical = absolute_path(path);\n+\tif (unsorted_string_list_lookup(&inc->seen_paths, canonical))\n+\t\treturn 1;\n+\n+\tstring_list_append(&inc->seen_paths, canonical);\n+\treturn 0;\n+}\n+\n+static int handle_path_include(const char *path, config_fn_t fn, void *data,\n+\t\t\t       struct config_include_context *inc)\n+{\n+\tint ret = 0;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t/*\n+\t * Use an absolute path as-is, but interpret relative paths\n+\t * based on the including config file.\n+\t */\n+\tif (!is_absolute_path(path)) {\n+\t\tchar *slash;\n+\n+\t\tif (!cf || !cf->name)\n+\t\t\treturn error(\"relative config includes must from from files\");\n+\n+\t\tslash = find_last_dir_sep(cf->name);\n+\t\tif (slash)\n+\t\t\tstrbuf_add(&buf, cf->name, slash - cf->name + 1);\n+\t\tstrbuf_addf(&buf, \"%s\", path);\n+\t\tpath = buf.buf;\n+\t}\n+\n+\tif (!access(path, R_OK))\n+\t\tret = git_config_from_file(fn, path, data, inc);\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static int handle_config_variable(const char *var, const char *value,\n+\t\t\t\t  config_fn_t fn, void *data,\n+\t\t\t\t  struct config_include_context *inc)\n+{\n+\tint r;\n+\n+\tr = fn(var, value, data);\n+\tif (!r && inc && inc->enabled && value && !prefixcmp(var, \"include.\")) {\n+\t\tconst char *type = var + 8;\n+\t\tif (!strcmp(type, \"path\"))\n+\t\t\tr = handle_path_include(value, fn, data, inc);\n+\t}\n+\treturn r;\n+}\n+\n static void lowercase(char *p)\n {\n \tfor (; *p; p++)\n@@ -47,8 +112,8 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n-int git_config_parse_parameter(const char *text,\n-\t\t\t       config_fn_t fn, void *data)\n+int git_config_parse_parameter(const char *text, config_fn_t fn, void *data,\n+\t\t\t       struct config_include_context *inc)\n {\n \tstruct strbuf **pair;\n \tpair = strbuf_split_str(text, '=', 2);\n@@ -62,7 +127,10 @@ int git_config_parse_parameter(const char *text,\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n \tlowercase(pair[0]->buf);\n-\tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n+\n+\tif (handle_config_variable(pair[0]->buf,\n+\t\t\t\t   pair[1] ? pair[1]->buf : NULL,\n+\t\t\t\t   fn, data, inc) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n \t}\n@@ -70,7 +138,8 @@ int git_config_parse_parameter(const char *text,\n \treturn 0;\n }\n \n-int git_config_from_parameters(config_fn_t fn, void *data)\n+int git_config_from_parameters(config_fn_t fn, void *data,\n+\t\t\t       struct config_include_context *inc)\n {\n \tconst char *env = getenv(CONFIG_DATA_ENVIRONMENT);\n \tchar *envw;\n@@ -89,7 +158,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n \t}\n \n \tfor (i = 0; i < nr; i++) {\n-\t\tif (git_config_parse_parameter(argv[i], fn, data) < 0) {\n+\t\tif (git_config_parse_parameter(argv[i], fn, data, inc) < 0) {\n \t\t\tfree(argv);\n \t\t\tfree(envw);\n \t\t\treturn -1;\n@@ -191,7 +260,8 @@ static inline int iskeychar(int c)\n \treturn isalnum(c) || c == '-';\n }\n \n-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n+static int get_value(config_fn_t fn, void *data, char *name, unsigned int len,\n+\t\t     struct config_include_context *inc)\n {\n \tint c;\n \tchar *value;\n@@ -219,7 +289,8 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\tif (!value)\n \t\t\treturn -1;\n \t}\n-\treturn fn(name, value, data);\n+\n+\treturn handle_config_variable(name, value, fn, data, inc);\n }\n \n static int get_extended_base_var(char *name, int baselen, int c)\n@@ -277,7 +348,8 @@ static int get_base_var(char *name)\n \t}\n }\n \n-static int git_parse_file(config_fn_t fn, void *data)\n+static int git_parse_file(config_fn_t fn, void *data,\n+\t\t\t  struct config_include_context *inc)\n {\n \tint comment = 0;\n \tint baselen = 0;\n@@ -327,7 +399,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\tif (!isalpha(c))\n \t\t\tbreak;\n \t\tvar[baselen] = tolower(c);\n-\t\tif (get_value(fn, data, var, baselen+1) < 0)\n+\t\tif (get_value(fn, data, var, baselen+1, inc) < 0)\n \t\t\tbreak;\n \t}\n \tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n@@ -826,11 +898,15 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n-int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n+int git_config_from_file(config_fn_t fn, const char *filename, void *data,\n+\t\t\t struct config_include_context *inc)\n {\n \tint ret;\n \tFILE *f = fopen(filename, \"r\");\n \n+\tif (check_and_mark_include_path(inc, filename))\n+\t\treturn 0;\n+\n \tret = -1;\n \tif (f) {\n \t\tconfig_file top;\n@@ -844,7 +920,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \t\tstrbuf_init(&top.value, 1024);\n \t\tcf = &top;\n \n-\t\tret = git_parse_file(fn, data);\n+\t\tret = git_parse_file(fn, data, inc);\n \n \t\t/* pop config-file parsing state stack */\n \t\tstrbuf_release(&top.value);\n@@ -874,17 +950,17 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n-int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n+int git_config_early(config_fn_t fn, void *data, const char *repo_config,\n+\t\t     struct config_include_context *inc)\n {\n \tint ret = 0, found = 0;\n \tconst char *home = NULL;\n \n \t/* Setting $GIT_CONFIG makes git read _only_ the given config file. */\n \tif (config_exclusive_filename)\n-\t\treturn git_config_from_file(fn, config_exclusive_filename, data);\n+\t\treturn git_config_from_file(fn, config_exclusive_filename, data, NULL);\n \tif (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {\n-\t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n-\t\t\t\t\t    data);\n+\t\tret += git_config_from_file(fn, git_etc_gitconfig(), data, inc);\n \t\tfound += 1;\n \t}\n \n@@ -893,17 +969,17 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \t\tchar buf[PATH_MAX];\n \t\tchar *user_config = mksnpath(buf, sizeof(buf), \"%s/.gitconfig\", home);\n \t\tif (!access(user_config, R_OK)) {\n-\t\t\tret += git_config_from_file(fn, user_config, data);\n+\t\t\tret += git_config_from_file(fn, user_config, data, inc);\n \t\t\tfound += 1;\n \t\t}\n \t}\n \n \tif (repo_config && !access(repo_config, R_OK)) {\n-\t\tret += git_config_from_file(fn, repo_config, data);\n+\t\tret += git_config_from_file(fn, repo_config, data, inc);\n \t\tfound += 1;\n \t}\n \n-\tswitch (git_config_from_parameters(fn, data)) {\n+\tswitch (git_config_from_parameters(fn, data, inc)) {\n \tcase -1: /* error */\n \t\tdie(\"unable to parse command-line config\");\n \t\tbreak;\n@@ -921,11 +997,13 @@ int git_config(config_fn_t fn, void *data)\n {\n \tchar *repo_config = NULL;\n \tint ret;\n+\tstruct config_include_context inc = CONFIG_INCLUDE_INIT(1);\n \n \trepo_config = git_pathdup(\"config\");\n-\tret = git_config_early(fn, data, repo_config);\n+\tret = git_config_early(fn, data, repo_config, &inc);\n \tif (repo_config)\n \t\tfree(repo_config);\n+\tstring_list_clear(&inc.seen_paths, 0);\n \treturn ret;\n }\n \n@@ -1313,7 +1391,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t * As a side effect, we make sure to transform only a valid\n \t\t * existing config file.\n \t\t */\n-\t\tif (git_config_from_file(store_aux, config_filename, NULL)) {\n+\t\tif (git_config_from_file(store_aux, config_filename, NULL, NULL)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n \t\t\tfree(store.key);\n \t\t\tif (store.value_regex != NULL) {\ndiff --git a/sequencer.c b/sequencer.c\nindex 5fcbcb8..50562b9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -636,7 +636,7 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n \n \tif (!file_exists(opts_file))\n \t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)\n+\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr, NULL) < 0)\n \t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n }\n \ndiff --git a/setup.c b/setup.c\nindex 61c22e6..947fa51 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -329,7 +329,7 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n \t * is a good one.\n \t */\n \tsnprintf(repo_config, PATH_MAX, \"%s/config\", gitdir);\n-\tgit_config_early(check_repository_format_version, NULL, repo_config);\n+\tgit_config_early(check_repository_format_version, NULL, repo_config, NULL);\n \tif (GIT_REPO_VERSION < repository_format_version) {\n \t\tif (!nongit_ok)\n \t\t\tdie (\"Expected git repo version <= %d, found %d\",\ndiff --git a/submodule.c b/submodule.c\nindex 9a28060..4c5b9be 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -116,7 +116,7 @@ void gitmodules_config(void)\n \t\t}\n \n \t\tif (!gitmodules_is_unmerged)\n-\t\t\tgit_config_from_file(submodule_config, gitmodules_path.buf, NULL);\n+\t\t\tgit_config_from_file(submodule_config, gitmodules_path.buf, NULL, NULL);\n \t\tstrbuf_release(&gitmodules_path);\n \t}\n }\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nnew file mode 100755\nindex 0000000..3d5a46d\n--- /dev/null\n+++ b/t/t1305-config-include.sh\n@@ -0,0 +1,145 @@\n+#!/bin/sh\n+\n+test_description='test config file include directives'\n+. ./test-lib.sh\n+\n+test_expect_success 'include file by absolute path' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = \\\"$PWD/one\\\"\" >.gitconfig &&\n+\techo 1 >expect &&\n+\tgit config test.one >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'include file by relative path' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = one\" >.gitconfig &&\n+\techo 1 >expect &&\n+\tgit config test.one >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'chained relative paths' '\n+\tmkdir subdir &&\n+\techo \"[test]three = 3\" >subdir/three &&\n+\techo \"[include]path = three\" >subdir/two &&\n+\techo \"[include]path = subdir/two\" >.gitconfig &&\n+\techo 3 >expect &&\n+\tgit config test.three >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'include options can still be examined' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = one\" >.gitconfig &&\n+\techo one >expect &&\n+\tgit config include.path >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'listing includes option and expansion' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = one\" >.gitconfig &&\n+\tcat >expect <<-\\EOF &&\n+\tinclude.path=one\n+\ttest.one=1\n+\tEOF\n+\tgit config --list >actual.full &&\n+\tgrep -v ^core actual.full >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'single file lookup does not expand includes by default' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = one\" >.gitconfig &&\n+\ttest_must_fail git config -f .gitconfig test.one &&\n+\ttest_must_fail git config --global test.one &&\n+\techo 1 >expect &&\n+\tgit config --includes -f .gitconfig test.one >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'writing config file does not expand includes' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = one\" >.gitconfig &&\n+\tgit config test.two 2 &&\n+\techo 2 >expect &&\n+\tgit config --no-includes test.two >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config --no-includes test.one\n+'\n+\n+test_expect_success 'config modification does not affect includes' '\n+\techo \"[test]one = 1\" >one &&\n+\techo \"[include]path = one\" >.gitconfig &&\n+\tgit config test.one 2 &&\n+\techo 1 >expect &&\n+\tgit config -f one test.one >actual &&\n+\ttest_cmp expect actual &&\n+\tcat >expect <<-\\EOF &&\n+\t1\n+\t2\n+\tEOF\n+\tgit config --get-all test.one >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'missing include files are ignored' '\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[include]path = foo\n+\t[test]value = yes\n+\tEOF\n+\techo yes >expect &&\n+\tgit config test.value >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'absolute includes from command line work' '\n+\techo \"[test]one = 1\" >one &&\n+\techo 1 >expect &&\n+\tgit -c include.path=\"$PWD/one\" config test.one >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'relative includes from command line fail' '\n+\techo \"[test]one = 1\" >one &&\n+\ttest_must_fail git -c include.path=one config test.one\n+'\n+\n+test_expect_success 'include cycles are detected and broken' '\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[test]value = gitconfig\n+\t[include]path = cycle\n+\tEOF\n+\tcat >cycle <<-\\EOF &&\n+\t[test]value = cycle\n+\t[include]path = .gitconfig\n+\tEOF\n+\tcat >expect <<-\\EOF &&\n+\tgitconfig\n+\tcycle\n+\tEOF\n+\tgit config --get-all test.value >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'multiple includes of the same file are suppressed' '\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[test]\n+\tvalue = base\n+\t[include]\n+\tpath = A\n+\tpath = B\n+\tEOF\n+\techo \"[include]path = C\" >A &&\n+\techo \"[include]path = C\" >B &&\n+\techo \"[test]value = C\" >C &&\n+\tcat >expect <<-\\EOF &&\n+\tbase\n+\tC\n+\tEOF\n+\tgit config --get-all test.value >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"183993","messageId":"7vobtc8l3i.fsf@alter.siamese.dyndns.org","threadId":"29559","inReplyTo":"20120206062713.GA9699@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T07:41:37Z","receivedAt":"2012-02-06T07:41:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... But the point of\n> duplicate suppression was that individual config files wouldn't have to\n> know or care what was being included elsewhere.\n\nI think you wanted to say \"the point of inclusion mechanism\" is that\nindividual configuration files would not have to know, and I think I\nagree.\n\n> So I'm actually thinking I should drop the duplicate suppression and\n> just do some sort of sanity check on include-depth to break cycles\n> (i.e., just die because it's a crazy thing to do, and we are really just\n> trying to tell the user their config is broken rather than go into an\n> infinite loop). As a bonus, it makes the code much simpler, too.\n\nYeah, I stand corrected. It was a bad suggestion.\n\nThanks.\n"},{"id":"184013","messageId":"4F2FA330.7020803@alum.mit.edu","threadId":"29559","inReplyTo":"20120206062713.GA9699@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-02-06T09:53:52Z","receivedAt":"2012-02-06T09:53:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/06/2012 07:27 AM, Jeff King wrote:\n>   3. perform cycle and duplicate detection on included files\n> [...]\n> \n> We first read the global config, which sets the value to \"global\", then\n> includes foo, which overwrites it to \"foo\". Then we read the repo\n> config, which sets the value to \"repo\", and then does _not_ actually\n> read foo. Because git config is read linearly and later values tend to\n> overwrite earlier ones, we would want to suppress the _first_ instance\n> of a file, not the second (or really, the final if it is included many\n> times). But that is impossible to do without generating a complete graph\n> of includes and then pruning the early ones.\n\nISTM that the main goal was to prevent infinite recursion, not avoid a\nlittle bit of redundant reading.  If that is the goal, all you have to\nrecord is the \"include stack\" context that caused the\ncurrently-being-included file to be read and make sure that the\ncurrently-being-included file didn't appear earlier on the stack.  The\nfact that the same file was included earlier (but not as part of the\nsame include chain) needn't be considered an error.\n\n> [...]\n> So I'm actually thinking I should drop the duplicate suppression and\n> just do some sort of sanity check on include-depth to break cycles\n> (i.e., just die because it's a crazy thing to do, and we are really just\n> trying to tell the user their config is broken rather than go into an\n> infinite loop). As a bonus, it makes the code much simpler, too.\n\nI agree that it is more sensible to error out than to ignore a recursive\ninclude.\n\nIt might nevertheless be useful to have an \"include call stack\"\navailable for generating output for errors that occur when parsing\nconfig files; something like:\n\nError when parsing /home/me/bogusconfigfile line 75\n    which was included from /home/me/okconfigfile line 13\n    which was included from /home/me/.gitconfig line 22\n\nor\n\nError: /home/me/bogusconfigfile included recursively by\n/home/me/bogusfile2 line 75\n    which was included from /home/me/bogusconfigfile line 13\n    which was included from /home/me/.gitconfig line 22\n\nOTOH such error stack dumps could be generated when unwinding the C call\nstack without the need for a separate \"include call stack\".\n\nHowever, one could even imagine a command like\n\n    $ git config --where-defined user.email\n    user.email defined by /home/me/myfile2 line 75\n        which was included from /home/me/myfile1 line 13\n        which was included from /home/me/.gitconfig line 22\n    user.email redefined by /home/me/project/.git/companyconfig line 8\n        which was included from /home/me/project/.git/config line 20\n\nThis usage could not be implemented using the C stack, because the C\nstack cannot be unwound multiple times.\n\nBut these are all just wild ideas.  I doubt that people's config files\nwill become so complicated that this much infrastructure is needed.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"184015","messageId":"20120206100622.GC4300@sigill.intra.peff.net","threadId":"29559","inReplyTo":"4F2FA330.7020803@alum.mit.edu","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T10:06:22Z","receivedAt":"2012-02-06T10:06:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 10:53:52AM +0100, Michael Haggerty wrote:\n\n> ISTM that the main goal was to prevent infinite recursion, not avoid a\n> little bit of redundant reading.\n\nIt was both, actually. There was a sense that we should not end up with\nduplicate entries by reading the same file twice. However, entries which\nactually create lists (and could therefore create duplicates) are by far\nthe minority compared to entries which overwrite. And it is the\noverwrite-style entries which are harmed by suppressing duplicates.\n\n> If that is the goal, all you have to record is the \"include stack\"\n> context that caused the currently-being-included file to be read and\n> make sure that the currently-being-included file didn't appear earlier\n> on the stack.  The fact that the same file was included earlier (but\n> not as part of the same include chain) needn't be considered an error.\n\nI considered this, but decided the complexity wasn't worth it,\nespecially because getting it accurate means cooperation from\ngit_config_from_file, which otherwise doesn't know or care about this\nmechanism. Instead I keep a simple depth counter and barf at a\nreasonable maximum, printing the \"from\" and \"to\" files. Yes, it's not\nnearly as elegant as keeping the whole stack, but I really don't expect\npeople to have complex super-deep includes, nor for it to be difficult\nto hunt down the cause of a cycle.\n\nMaybe that is short-sighted or lazy of me, but I'd just really be\nsurprised if people ever went more than 1 or 2 layers deep, or if they\nactually ended up with a cycle at all.\n\nThere is a stack kept already by git_config_from_file, but it doesn't\nrespect the call-chain (so if I start a new git_config() call from\ninside a callback, it is still part of the same stack).\n\nWe could possibly put a marker in the stack for that situation, and then\nit would be usable for include cycle-detection.\n\n> However, one could even imagine a command like\n> \n>     $ git config --where-defined user.email\n>     user.email defined by /home/me/myfile2 line 75\n>         which was included from /home/me/myfile1 line 13\n>         which was included from /home/me/.gitconfig line 22\n>     user.email redefined by /home/me/project/.git/companyconfig line 8\n>         which was included from /home/me/project/.git/config line 20\n\nYou can already implement this with the existing stack by providing a\nsuitable callback (since its your callback, you'd know that there was no\nrecursion of git_config, and therefore the stack refers only to a single\nset of includes).\n\n-Peff\n"},{"id":"184016","messageId":"20120206101630.GA5394@sigill.intra.peff.net","threadId":"29559","inReplyTo":"20120206100622.GC4300@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T10:16:30Z","receivedAt":"2012-02-06T10:16:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 05:06:22AM -0500, Jeff King wrote:\n\n> > If that is the goal, all you have to record is the \"include stack\"\n> > context that caused the currently-being-included file to be read and\n> > make sure that the currently-being-included file didn't appear earlier\n> > on the stack.  The fact that the same file was included earlier (but\n> > not as part of the same include chain) needn't be considered an error.\n> \n> I considered this, but decided the complexity wasn't worth it,\n> especially because getting it accurate means cooperation from\n> git_config_from_file, which otherwise doesn't know or care about this\n> mechanism. Instead I keep a simple depth counter and barf at a\n> reasonable maximum, printing the \"from\" and \"to\" files. Yes, it's not\n> nearly as elegant as keeping the whole stack, but I really don't expect\n> people to have complex super-deep includes, nor for it to be difficult\n> to hunt down the cause of a cycle.\n\nOh, one other thing: to detect cycles, you have to use a canonical\nversion of the filename for comparisons. That makes the existing stack\nthat git_config_from_file keeps useless. Using real_path is hard,\nbecause it will die() if some path components don't exist. Using\nabsolute_path is a reasonable compromise, but it can actually miss some\ncycles if they involve symbolic links.\n\nNot insurmountable if you can accept teaching git_config_from_file to\nreal_path() each file it reads. But again, it ends up getting complex\nfor IMHO not much gain. A stupid depth counter can't show you a stack\ntrace (and it may have false positives), but it's dirt simple and\nprobably good enough.\n\n-Peff\n"},{"id":"184094","messageId":"CAJDDKr6A2UvB3D-Dapw7WCEzWfzLoLd0E8MSDjT0RtdxFeWZAQ@mail.gmail.com","threadId":"29559","inReplyTo":"20120206062713.GA9699@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-02-07T05:01:21Z","receivedAt":"2012-02-07T05:01:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Feb 5, 2012 at 10:27 PM, Jeff King <peff@peff.net> wrote:\n> That leaves the file-inclusion bits:\n>\n>  [1/2]: imap-send: remove dead code\n>  [2/2]: config: add include directive\n>\n> The first patch is new in this round, and is a necessary cleanup for the\n> second patch (though it might be worth applying regardless).\n>\n> The second patch corresponds to patch 1/4 from the previous round. Among\n> the functional changes are:\n>\n>  1. do not use includes for \"git config\" in single-file mode\n\nI have a questions about this.  Let's say I have ~/foo1.gitconfig:\n\n[foo]\n    bar = 1\n\n...and ~/.gitconfig (I forgot the syntax):\n\n[foo]\n    bar = 0\n\n#include \"~/foo1.gitconfig\"\n\n\nDoes that mean that:\n\n    $ git config -f ~/.gitconfig foo.bar\n\n...prints 0 and not 1?\n\nI can see cases where this would be undesirable behavior.\n\nFor example, an application that uses \"git config -z --list -f\n~/.gitconfig\" might expect that the result encompasses all of the\nuser-specific config bits.\n\nFollowing this to its natural conclusion means \"git config\" might\nlearn some kind of --no-include flag for use with e.g. \"git config\n--no-include -f ~/.gitconfig\".  That said, I don't know where I would\never actually use such a flag.\n\nI do know where I would use an `--include` flag (if following includes\nwere not the default behavior when using '-f'), though, and that's why\nI'm asking.  The problem with not having it be the default means we\nhave to use a flag.  This makes it harder to support multiple versions\nof git.\n\nMaybe I'm mis-interpreting what you mean by, 'do not use includes for\n\"git config\" in single-file mode', though.\n--\nDavid\n"},{"id":"184095","messageId":"20120207051719.GA11598@sigill.intra.peff.net","threadId":"29559","inReplyTo":"CAJDDKr6A2UvB3D-Dapw7WCEzWfzLoLd0E8MSDjT0RtdxFeWZAQ@mail.gmail.com","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T05:17:19Z","receivedAt":"2012-02-07T05:17:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote:\n\n> I have a questions about this.  Let's say I have ~/foo1.gitconfig:\n> \n> [foo]\n>     bar = 1\n> \n> ...and ~/.gitconfig (I forgot the syntax):\n> \n> [foo]\n>     bar = 0\n> \n> #include \"~/foo1.gitconfig\"\n> \n> \n> Does that mean that:\n> \n>     $ git config -f ~/.gitconfig foo.bar\n> \n> ...prints 0 and not 1?\n\nYes, though the syntax is:\n\n  [include]\n    path = foo1.gitconfig\n\n(it doesn't respect tilde-expansion, but it probably should). Note that\nthe syntax was specifically selected for backwards compatibility, and to\nallow manipulation with existing tools.\n\n> I can see cases where this would be undesirable behavior.\n> \n> For example, an application that uses \"git config -z --list -f\n> ~/.gitconfig\" might expect that the result encompasses all of the\n> user-specific config bits.\n\nThe problem is that an application might also expect it _not_ to happen\n(e.g., if the application is really interested in knowing what's in\n~/.gitconfig). Because includes aren't respected now, the safe default\nseems to be not to have includes (i.e., don't change behavior for this\ncase).\n\nA bigger question for me is: if you are interested in getting an answer\nfrom anywhere, why are you restricting with \"-f\"? IOW, is this a\nreal-world problem, or a hypothetical one? And if real-world, what is\nthe actual use case?\n\n> Following this to its natural conclusion means \"git config\" might\n> learn some kind of --no-include flag for use with e.g. \"git config\n> --no-include -f ~/.gitconfig\".  That said, I don't know where I would\n> ever actually use such a flag.\n\nIt already learned it as part of my series (and --includes, as well).\n\n> I do know where I would use an `--include` flag (if following includes\n> were not the default behavior when using '-f'), though, and that's why\n> I'm asking.  The problem with not having it be the default means we\n> have to use a flag.  This makes it harder to support multiple versions\n> of git.\n\nYes, that's a general problem of adding new command-line options to turn\nfeatures off or on. We could add an environment variable to control this\nfeature. But I'd really like to hear the compelling use case first\n(i.e., both why it cannot simply be spelled \"git config -z --list\", and\nwhy not following includes is not OK).\n\n> Maybe I'm mis-interpreting what you mean by, 'do not use includes for\n> \"git config\" in single-file mode', though.\n\nNo, I think you understand the described behavior.\n\n-Peff\n"},{"id":"184114","messageId":"CAJDDKr5yiKvNnpVV29jFK1Z1yuUnA-=dn0yMB8iW9y53vRGDHQ@mail.gmail.com","threadId":"29559","inReplyTo":"20120207051719.GA11598@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-02-07T10:05:34Z","receivedAt":"2012-02-07T10:05:34Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Feb 6, 2012 at 9:17 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote:\n>\n>> I have a questions about this.  Let's say I have ~/foo1.gitconfig:\n>>\n>> [foo]\n>>     bar = 1\n>>\n>> ...and ~/.gitconfig (I forgot the syntax):\n>>\n>> [foo]\n>>     bar = 0\n>>\n>> #include \"~/foo1.gitconfig\"\n>>\n>>\n>> Does that mean that:\n>>\n>>     $ git config -f ~/.gitconfig foo.bar\n>>\n>> ...prints 0 and not 1?\n>\n> Yes, though the syntax is:\n>\n>  [include]\n>    path = foo1.gitconfig\n>\n> (it doesn't respect tilde-expansion, but it probably should). Note that\n> the syntax was specifically selected for backwards compatibility, and to\n> allow manipulation with existing tools.\n>\n>> I can see cases where this would be undesirable behavior.\n>>\n>> For example, an application that uses \"git config -z --list -f\n>> ~/.gitconfig\" might expect that the result encompasses all of the\n>> user-specific config bits.\n>\n> The problem is that an application might also expect it _not_ to happen\n> (e.g., if the application is really interested in knowing what's in\n> ~/.gitconfig). Because includes aren't respected now, the safe default\n> seems to be not to have includes (i.e., don't change behavior for this\n> case).\n>\n> A bigger question for me is: if you are interested in getting an answer\n> from anywhere, why are you restricting with \"-f\"? IOW, is this a\n> real-world problem, or a hypothetical one? And if real-world, what is\n> the actual use case?\n\nIt's a real-world problem.  I haven't had to change the code in a\nwhile which is why this thread caught my eye ;-)\n\nI won't claim it's the best solution, but this has worked extremely\nwell in practice:\n\nhttps://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py\n\nHere's the problem.  I need to know which config items are \"user\"\nconfig (~/.gitconfig), which are \"repo\" config (.git/config), and\nwhich are \"system\" config (/etc/gitconfig).\n\nI also need to avoid calling \"git config\" too many times so I query\nthese files once and cache it all in memory.  We need only stat()\nthese files to know whether to redo the \"git config\" call.\n\nFiguring out the repo config is tricky.  You can't just say \"git\nconfig --list\" because that includes the user config stuff in\n~/.gitconfig.\n\nFiguring out the user config is easy because you can say \"git config\n--global --list\".  This is inconsistent with the behavior for \"git\nconfig --list\" because it does not include the --system config, which\none would expect given the overlay semantics.\n\nFiguring out the system config can be done with --system.  That works fine.\n\nThe generic interface for getting a concise listing of values from\nthese sources is to use \"git config -f\" on ~/.gitconfig, .git/config,\nand $(prefix)/etc/gitconfig.\n\ngit config --global and git config --system are both consistent in\nthat they return just the information relevant to them.  Is --global\njust a shortcut for \"-f ~/.gitconfig\"?  If yes, then does that mean\n\"git config --global\" will not honor includes?  If it is not a\nshortcut, does that mean that \"git config --global\" and \"git config -f\n~/.gitconfig\" are not really the same thing?  The documentation does\nlead one to believe that they should be equivalent...\n\nThe takeaway is that querying these files provides a convenient way to\naccess the effective configuration values for the system, user, and\nrepo.\n\n\n>> I do know where I would use an `--include` flag (if following includes\n>> were not the default behavior when using '-f'), though, and that's why\n>> I'm asking.  The problem with not having it be the default means we\n>> have to use a flag.  This makes it harder to support multiple versions\n>> of git.\n>\n> Yes, that's a general problem of adding new command-line options to turn\n> features off or on. We could add an environment variable to control this\n> feature. But I'd really like to hear the compelling use case first\n> (i.e., both why it cannot simply be spelled \"git config -z --list\", and\n> why not following includes is not OK).\n\nHopefully my explanation conveys why \"git config -z --list\" is insufficient.\n\nBasically, I've been relying on the fact that querying each of these\nfiles gives me exactly what users have effectively configured.\nPushing knowledge of includes onto the application is less fun.\nApplications would either have to understand the [include] path = ...\nconstruct and do something with it themselves or they'd have to check\nthe git version at runtime.\n\nI would rather do without the runtime version check and have\neverything \"keep working\".\n\n\nSo let's explore why I think it should follow includes.  Here are a\nfew reasons...\n\nWhen the file format supports including other files then the most\nlogical thing for it to do is to follow those includes, no?  I know,\nthis is a tautology ;-).  I'll try again.\n\nFrom a naive user's POV --\nI am asking \"git config\" to evaluate this file.  I don't care what's\ninside of it; it's a black box to me.  All I know is that I get config\nvalues out the other side.  Sometimes git respects the included files.\nSometimes it does not (when using -f).\n^^^^^^^^^\nI think this subtle difference in behavior is best avoided.\n\nHere's another.  The naive user will do: `git config --list -f file`.\nThey will then edit that file to add an include statement.  They will\nthen run `git config --list -f file` again and be confused as to why\ntheir included configuration is not honored.\n\nRight now these two are synonymous: \"git config --system --list\" and\n\"git config -f $(prefix)/etc/gitconfig --list\".  Having one form honor\nincludes and not the other is inconsistent.\n\nWhat if we frame this the other way around -- when would we not want\nit to follow includes?  I can imagine someone writing a \"git config\"\nediting program, but I think that use case is rare (and they haven't\nspoken up, either ;-).\n\nWell.. okay. My use case is rare too!  The dumb \"cache the values\nbehind a stat() call\" thing has worked really well in practice so I've\nbeen happy with it so far.  It's dumb, and it won't notice when\nincluded files change, but I really don't care because it solves the\n99% case.\n\nI'd be happy to rewrite it using another approach.  The\noverlay-semantics inconsistency is what led me to using the generic\n\"-f\" interface.  Performance issues led me to use a stat cache.  Using\neach file's stat info as the cache key mapped nicely onto the generic\napproach of using \"-f\".\n\nRewriting it (for me) would mean using --global and --system and\nprobably not caring so much about the distinction of repo vs. user\nconfiguration (or rather, deal with the fact that `git config --list`\nreturns both the user config and the repo config). At that point I\nmight as well just write (reuse) a proper .gitconfig parser, but\nwhat's the fun in that when \"git config\" is so nice and easy to parse?\n;-)\n-- \nDavid\n"},{"id":"184142","messageId":"20120207173025.GA22225@sigill.intra.peff.net","threadId":"29559","inReplyTo":"CAJDDKr5yiKvNnpVV29jFK1Z1yuUnA-=dn0yMB8iW9y53vRGDHQ@mail.gmail.com","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T17:30:25Z","receivedAt":"2012-02-07T17:30:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 02:05:34AM -0800, David Aguilar wrote:\n\n> I won't claim it's the best solution, but this has worked extremely\n> well in practice:\n> \n> https://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py\n> \n> Here's the problem.  I need to know which config items are \"user\"\n> config (~/.gitconfig), which are \"repo\" config (.git/config), and\n> which are \"system\" config (/etc/gitconfig).\n\nI'm not a git cola user. But from peeking at that code and a brief\n\"apt-get install git-cola && git cola\", it looks like the point is to\nshow a dialog with per-repo and per-user settings, let the user tweak\nthem, and save them back to the appropriate spot.\n\nBut how does that interact with includes? Let's imagine I do this:\n\n  git config --global include.path .gitidentity\n  git config -f ~/.gitidentity user.name \"Jeff King\"\n\nWithout having git-cola expand includes, your call to \"git config\n--global --list\" will not show the user.name variable, and it will\nappear blank. Which is not great.\n\nIf git-cola does expand includes, then the name will appear. But what\nwill happen when I modify that field and try to save it? It will save it\nusing \"git config --global\", putting it into ~/.gitconfig. So now you'll\nhave two names in your config, one in .gitconfig and one in\n.gitidentity.\n\nAnd which one will take precedence?  That depends on what was in your\nconfig file before. If there was a [user] section before the include\nalready in your .gitconfig, then the newly written value will go in that\nsection _before_ the include, and the include will still take\nprecedence. Otherwise, a new section will be written _after_ the\ninclude, and that will take precedence.\n\nSo...yuck. It's kind of a complex and messy situation. But the important\nthing is that when you are looking at a bidirectional editing situation\nlike this, blindly expanding includes is not going to solve the problem.\nIt's just going to paper over the complexity. In the long term,\nsomething like git-cola that is trying to present and edit config to the\nuser would need to learn about includes, track the sources of each\nvariable, and then edit appropriately.\n\nYou can do that with my current patch by manually following include.path\ndirectives. However, that's a pain.  Git-config could potentially help\nwith that (and even simplify the current code) by allowing something\nlike:\n\n  $ git config --list-with-sources\n  /home/peff/.gitconfig user.name=Jeff King\n  /home/peff/.gitconfig user.email=peff@peff.net\n  .git/config core.repositoryformatversion=0\n  .git/config core.bare=false\n  [etc]\n\n(you would use the \"-z\" form, of course, and the filenames would be\nNUL-separated, but I made up a human-readable output format above for\nillustration purposes).\n\nAnd then it would be natural to mention included files there, and you\ncould actually modify the file that contains the value in question. Of\ncourse, you might not have _access_ to the file in question. You could\nbe pulling in options from a shared config file, and the right thing to\ndo is not to edit the shared file, but to override it. And that raises\nthe question of where, and how to tell git-config to make sure that the\noption goes _after_ the include.\n\nSo fundamentally, includes make the idea of \"overwrite this value\" much\nmore complex. It's possible that git-config should learn all of this\ncomplexity, and that git cola could rely on git-config being smart.  But\nI was rather hoping that most of the automated tools could remain simple\nand stupid, and that people who wanted to do complex things with\nincludes would be left to their own devices (i.e., using $EDITOR, or\nsmartly using \"git config -f\" on their individual files).\n\n> Figuring out the repo config is tricky.  You can't just say \"git\n> config --list\" because that includes the user config stuff in\n> ~/.gitconfig.\n> \n> Figuring out the user config is easy because you can say \"git config\n> --global --list\".  This is inconsistent with the behavior for \"git\n> config --list\" because it does not include the --system config, which\n> one would expect given the overlay semantics.\n\nI think you are mistaking a lack of source options for \"look at the repo\nconfig\". But without providing a source, the request is \"look everywhere\nthat git knows about\". With \"--global\", it's about \"look at this one\nfile\" (just as \"--system\" is about \"look at this other file\"). So I\nthink what you are really lamenting is the lack of \"git config --repo\",\nwhich would basically be:\n\n  git config --list --file=\"$(git rev-parse --git-dir)/config\"\n\nThat might be something we could improve in the documentation.\n\n> The generic interface for getting a concise listing of values from\n> these sources is to use \"git config -f\" on ~/.gitconfig, .git/config,\n> and $(prefix)/etc/gitconfig.\n\nRight. And --global and --system are basically just synonyms for the\nprior two.\n\n> git config --global and git config --system are both consistent in\n> that they return just the information relevant to them.  Is --global\n> just a shortcut for \"-f ~/.gitconfig\"?  If yes, then does that mean\n> \"git config --global\" will not honor includes?  If it is not a\n> shortcut, does that mean that \"git config --global\" and \"git config -f\n> ~/.gitconfig\" are not really the same thing?  The documentation does\n> lead one to believe that they should be equivalent...\n\nYes, it is a shortcut, and it follows the same code path (i.e., we\nexpand \"--global\" into \"-f ~/.gitconfig\" early on).\n\n> > Yes, that's a general problem of adding new command-line options to turn\n> > features off or on. We could add an environment variable to control this\n> > feature. But I'd really like to hear the compelling use case first\n> > (i.e., both why it cannot simply be spelled \"git config -z --list\", and\n> > why not following includes is not OK).\n> \n> Hopefully my explanation conveys why \"git config -z --list\" is insufficient.\n\nYes, I agree it's insufficient in the face of includes. Unfortunately, I\ndon't think simply expanding includes by default is sufficient, either.\n\nProbably the simplest solution for this case would be something like:\n\n  1. Always expand includes on listing.\n\n  2. When editing, always override items in includes. So do _not_ reuse\n     sections, but always put the values at the end of the file (or at\n     least after any includes).\n\nThat's not as nice as doing it \"right\" (you'll end up with duplicated\nvalues in included files and their parents, and sections may appear\nmultiple times). But it's simple and should have the desired semantics.\n\n> From a naive user's POV --\n> I am asking \"git config\" to evaluate this file.  I don't care what's\n> inside of it; it's a black box to me.  All I know is that I get config\n> values out the other side.  Sometimes git respects the included files.\n> Sometimes it does not (when using -f).\n> ^^^^^^^^^\n> I think this subtle difference in behavior is best avoided.\n\nIt is not \"sometimes when I ask git config to evaluate this file\". It is\n\"git never expands includes when you ask it to evaluate a file\".\nIncludes are expanded only when you ask git \"search all files you know\nabout\" (i.e., when you do not provide a file argument).\n\nThere is also a more complex issue with \"-f\", which is that git-config\nis used not just to parse git configuration, but also to parse other\nconfig-like files that should not respect includes (e.g.,\n\".gitmodules\"). So to remain backwards-compatible, we need the default\nfor \"-f\" to not respect includes, and that is not really negotiable.\n\nWe could convert \"--global\" and \"--system\" to respect includes by\ndefault (since we at least know these are actual git config files). But\nthat is introducing a subtle difference, which you complained about\nabove.\n\nBut if scripted uses like \"git cola\" are the ones that need it, I'd\nrather provide a backwards-compatible way of tweaking the behavior (if a\ncommand line option is not sufficient, then I'd rather provide an\nenvironment variable).\n\n> Here's another.  The naive user will do: `git config --list -f file`.\n> They will then edit that file to add an include statement.  They will\n> then run `git config --list -f file` again and be confused as to why\n> their included configuration is not honored.\n\nAs I mentioned above, that one is not really negotiable. But it seems\nlike it is a mismatch of expectations to behavior, and that is something\nwe can at least address with documentation.\n\n> Right now these two are synonymous: \"git config --system --list\" and\n> \"git config -f $(prefix)/etc/gitconfig --list\".  Having one form honor\n> includes and not the other is inconsistent.\n\nWith my patch, they are consistent, and neither should honor includes\n(that being said, there is a bug in my patch which makes --list honor\nincludes in all cases; however, you are responding correctly to the\nintended and advertised behavior of my patch, and that is what we should\nbe discussing).\n\n> What if we frame this the other way around -- when would we not want\n> it to follow includes?  I can imagine someone writing a \"git config\"\n> editing program, but I think that use case is rare (and they haven't\n> spoken up, either ;-).\n\nBut isn't \"git cola\" a git-config editing program?\n\n> Well.. okay. My use case is rare too!  The dumb \"cache the values\n> behind a stat() call\" thing has worked really well in practice so I've\n> been happy with it so far.  It's dumb, and it won't notice when\n> included files change, but I really don't care because it solves the\n> 99% case.\n\nI don't get the caching. How often would you need to call \"git config\"?\nIsn't is only when the edit-config dialog appears? Is it really that\nexpensive to call when opening a user-editable dialog?\n\n> Rewriting it (for me) would mean using --global and --system and\n> probably not caring so much about the distinction of repo vs. user\n> configuration (or rather, deal with the fact that `git config --list`\n> returns both the user config and the repo config). At that point I\n> might as well just write (reuse) a proper .gitconfig parser, but\n> what's the fun in that when \"git config\" is so nice and easy to parse?\n\nI definitely feel your pain. I don't want you to have to rewrite yet\nanother .gitconfig parser. Let's see if we can figure out reasonable\nediting semantics that can go into git-config.\n\n-Peff\n"},{"id":"184145","messageId":"7vpqdq1pxt.fsf@alter.siamese.dyndns.org","threadId":"29559","inReplyTo":"20120207173025.GA22225@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T18:03:26Z","receivedAt":"2012-02-07T18:03:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So fundamentally, includes make the idea of \"overwrite this value\" much\n> more complex.\n\nI do not think it is anything new that include.path brings.  To give the\nuser a complete view of \"what customization applies when working in this\nrepository?\", and to allow the user to edit variables in the right file\namong usual three places, the tool already needs to be aware of possible\nplaces and give users a way to tell where the edit needs to go anyway.\ninclude.path only adds one more thing for the tool to be aware of.\n\nWith your example, the editor can show\n\n\tgit config -f .git/config --list\n\nwith \"include.path=~/.gitident\" listed as one of the key=value pairs\nwithout showing key=value pairs included from that file.  Or it can show\nuser.name in effect is this value from .git/config, and optionally also\nshow that there are other definitions of user.name in ~/.gitconfig (which\nwe use as if we have \"include.path=~/.gitconfig\" at the top of .git/config\nfile) or ~/.gitident specified with include.path.\n\nThe tool needs to make it easy to jump to ~/.gitident; it needs to know\nwhat include.path means.  The user can edit the value in ~/.gitident, or\nafter looking at ~/.gitident, choose to come back to .git/config and add\nan overriding entry there.\n\n> But isn't \"git cola\" a git-config editing program?\n\nYes, that is really the right \"rhetorical\" question to ask.  It needs to\nknow what it is editing, so it at least needs to be _aware_ of the\ninclusion, where each item comes from, and how to edit contents of _one_\nparticular file.\n\nAnd that issue is not something new that was introduced by include.path.\n"},{"id":"184150","messageId":"20120207182953.GA32367@sigill.intra.peff.net","threadId":"29559","inReplyTo":"7vpqdq1pxt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T18:29:53Z","receivedAt":"2012-02-07T18:29:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 10:03:26AM -0800, Junio C Hamano wrote:\n\n> > But isn't \"git cola\" a git-config editing program?\n> \n> Yes, that is really the right \"rhetorical\" question to ask.  It needs to\n> know what it is editing, so it at least needs to be _aware_ of the\n> inclusion, where each item comes from, and how to edit contents of _one_\n> particular file.\n> \n> And that issue is not something new that was introduced by include.path.\n\nTrue, but it was made more complex. David has already dealt with the 3\nsources in his code. Adding includes expands this to N sources, and his\ncode needs to adapt. It would be nice if we could at least make the\nadaptation backwards-compatible and as painless as possible.\n\nBut I just see the \"right\" answer in a complex case needing user input\nthat a simple call to \"git config\" can't provide. That is, pretending\nthat \"git config --global\" is still a single file for reading and\nwriting can get you something that _works_, but it will make a mess of\nthe user's config. The most elegant thing to me is to expand git-cola's\nconfig editor into an N-pane editor instead of a 2-pane editor (not 3,\nbecause it doesn't make sense to edit /etc/gitconfig with it, and nor\ndoes the current version support it). People without includes wouldn't\nnotice the difference, and people with includes might appreciate the\npower and flexibility.\n\n-Peff\n"},{"id":"184156","messageId":"m31uq63143.fsf@localhost.localdomain","threadId":"29559","inReplyTo":"20120207173025.GA22225@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-07T19:16:47Z","receivedAt":"2012-02-07T19:16:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n[...]\n> Git-config could potentially help with that (and even simplify the\n> current code) by allowing something like:\n> \n>   $ git config --list-with-sources\n>   /home/peff/.gitconfig user.name=Jeff King\n>   /home/peff/.gitconfig user.email=peff@peff.net\n>   .git/config core.repositoryformatversion=0\n>   .git/config core.bare=false\n>   [etc]\n> \n> (you would use the \"-z\" form, of course, and the filenames would be\n> NUL-separated, but I made up a human-readable output format above for\n> illustration purposes).\n\nThat would be _very_ nice to have (even without includes support).\n\nFilenames would be git-quoted like in ls-tree / diff-tree output without -z,\nisn't it?  And is that TAB or SPC as a separator?\n-- \nJakub Narebski\n"},{"id":"184157","messageId":"20120207192102.GA887@sigill.intra.peff.net","threadId":"29559","inReplyTo":"m31uq63143.fsf@localhost.localdomain","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T19:21:02Z","receivedAt":"2012-02-07T19:21:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:\n\n> > Git-config could potentially help with that (and even simplify the\n> > current code) by allowing something like:\n> > \n> >   $ git config --list-with-sources\n> >   /home/peff/.gitconfig user.name=Jeff King\n> >   /home/peff/.gitconfig user.email=peff@peff.net\n> >   .git/config core.repositoryformatversion=0\n> >   .git/config core.bare=false\n> >   [etc]\n> > \n> > (you would use the \"-z\" form, of course, and the filenames would be\n> > NUL-separated, but I made up a human-readable output format above for\n> > illustration purposes).\n> \n> That would be _very_ nice to have (even without includes support).\n> \n> Filenames would be git-quoted like in ls-tree / diff-tree output without -z,\n> isn't it?  And is that TAB or SPC as a separator?\n\nYeah, the output above is just illustrative. Without -z, I would do\nquoted filename, TAB, and then $KEY=$VALUE (I didn't check whether we\nquote an \"=\" in the subsection of a key, but we probably should).\n\n-Peff\n"},{"id":"184163","messageId":"CAJDDKr5uTj0Jav=3Kwf4Atd81qn1x9CLTveZOPmWwsnNCT9Lhw@mail.gmail.com","threadId":"29559","inReplyTo":"m31uq63143.fsf@localhost.localdomain","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-02-07T20:15:52Z","receivedAt":"2012-02-07T20:15:52Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Feb 7, 2012 at 11:16 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n> [...]\n>> Git-config could potentially help with that (and even simplify the\n>> current code) by allowing something like:\n>>\n>>   $ git config --list-with-sources\n>>   /home/peff/.gitconfig user.name=Jeff King\n>>   /home/peff/.gitconfig user.email=peff@peff.net\n>>   .git/config core.repositoryformatversion=0\n>>   .git/config core.bare=false\n>>   [etc]\n>>\n>> (you would use the \"-z\" form, of course, and the filenames would be\n>> NUL-separated, but I made up a human-readable output format above for\n>> illustration purposes).\n>\n> That would be _very_ nice to have (even without includes support).\n\nI like this as well.\n\nThanks for digging deep into this one, Jeff.  You've convinced me that\nnot following includes is the better default behavior.\n\nYou are correct in mentioning that what was really missing was\nsomething akin to \"git config --repo --list\".  Since that command\nwould be a shortcut for \"-f .git/config\" then it would be consistent\nin behavior.  The suggestion to have the app understand .[include]\npath = and show separate panes for included files is an elegant\nsolution and definitely helpful for power users.  I do have to write\nmore code but that's fine since it enables new functionality.\n\nJeff, you mentioned possibly adding a \"backwards-compatible way\" of\naccessing this stuff and hiding it behind an environment variable.  I\ndon't want to make us carry around backwards compat code paths just\nfor one particular use case so perhaps the best thing would be for me\nto start preparing for this change now.  I already have various places\nwhere functionality is guarded behind a git version check.  If what\nwe're talking about is git-cola adding \"--include\" when git >= 1.8(?)\nthen that works for me.\n\nIt should be noted that git-gui also uses `git config --global --list`\nso I don't know if this has implications there.  E.g. maybe things\nlike user.name won't be overridden if done via an included config\nthere.\n\nRE: the caching -- we call git config a few times in various places.\nGetting the user.* fields.  Getting the diff.summary settings, etc.  I\nstarted tweaking app startup for speed and noticed all the git-config\ncalls and was able to replace a handful of calls with a single call,\nwhich was nice.  The difference is more pronounced on win32 (I am a\nlinux user but I do try to play nice with the msysgit folks).\n\nThanks Jeff,\n-- \nDavid\n"},{"id":"184214","messageId":"20120209033059.GA4347@sigill.intra.peff.net","threadId":"29559","inReplyTo":"m31uq63143.fsf@localhost.localdomain","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-09T03:30:59Z","receivedAt":"2012-02-09T03:30:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> [...]\n> > Git-config could potentially help with that (and even simplify the\n> > current code) by allowing something like:\n> > \n> >   $ git config --list-with-sources\n> >   /home/peff/.gitconfig user.name=Jeff King\n> >   /home/peff/.gitconfig user.email=peff@peff.net\n> >   .git/config core.repositoryformatversion=0\n> >   .git/config core.bare=false\n> >   [etc]\n> > \n> > (you would use the \"-z\" form, of course, and the filenames would be\n> > NUL-separated, but I made up a human-readable output format above for\n> > illustration purposes).\n> \n> That would be _very_ nice to have (even without includes support).\n> \n> Filenames would be git-quoted like in ls-tree / diff-tree output without -z,\n> isn't it?  And is that TAB or SPC as a separator?\n\nSo the patch would look something like this. However, is the actual\nfilename really what callers want? It seems like in David's case, an\nannotation of \"repo\", \"global\", or \"system\" (possibly in addition to the\nfilename) would be the most useful (because in the git-cola UI, it is\nstill nice to list things as \"repo\" or \"global\" instead of spewing the\nwhole filename at the user -- but you would still want the individual\nfilename for handling updates of includes).\n\n---\n builtin/config.c |   20 ++++++++++++++++++--\n cache.h          |    1 +\n config.c         |    7 +++++++\n 3 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d35c06a..e0333f7 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -2,6 +2,7 @@\n #include \"cache.h\"\n #include \"color.h\"\n #include \"parse-options.h\"\n+#include \"quote.h\"\n \n static const char *const builtin_config_usage[] = {\n \t\"git config [options]\",\n@@ -41,6 +42,7 @@ static int end_null;\n #define ACTION_SET_ALL (1<<12)\n #define ACTION_GET_COLOR (1<<13)\n #define ACTION_GET_COLORBOOL (1<<14)\n+#define ACTION_LIST_WITH_SOURCES (1<<15)\n \n #define TYPE_BOOL (1<<0)\n #define TYPE_INT (1<<1)\n@@ -64,6 +66,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"rename-section\", &actions, \"rename section: old-name new-name\", ACTION_RENAME_SECTION),\n \tOPT_BIT(0, \"remove-section\", &actions, \"remove a section: name\", ACTION_REMOVE_SECTION),\n \tOPT_BIT('l', \"list\", &actions, \"list all\", ACTION_LIST),\n+\tOPT_BIT(0, \"list-with-sources\", &actions, \"list all variables with sources\", ACTION_LIST_WITH_SOURCES),\n \tOPT_BIT('e', \"edit\", &actions, \"opens an editor\", ACTION_EDIT),\n \tOPT_STRING(0, \"get-color\", &get_color_slot, \"slot\", \"find the color configured: [default]\"),\n \tOPT_STRING(0, \"get-colorbool\", &get_colorbool_slot, \"slot\", \"find the color setting: [stdout-is-tty]\"),\n@@ -86,6 +89,18 @@ static void check_argc(int argc, int min, int max) {\n \n static int show_all_config(const char *key_, const char *value_, void *cb)\n {\n+\tint show_sources = *(int *)cb;\n+\n+\tif (show_sources) {\n+\t\tconst char *fn = current_config_filename();\n+\t\tif (!fn)\n+\t\t\tfn = \"\";\n+\t\tif (end_null)\n+\t\t\tprintf(\"%s%c\", fn, '\\0');\n+\t\telse\n+\t\t\twrite_name_quoted(fn, stdout, '\\t');\n+\t}\n+\n \tif (value_)\n \t\tprintf(\"%s%c%s%c\", key_, delim, value_, term);\n \telse\n@@ -418,9 +433,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t\t}\n \n-\tif (actions == ACTION_LIST) {\n+\tif (actions == ACTION_LIST || actions == ACTION_LIST_WITH_SOURCES) {\n+\t\tint show_sources = actions & ACTION_LIST_WITH_SOURCES ? 1 : 0;\n \t\tcheck_argc(argc, 0, 0);\n-\t\tif (git_config(show_all_config, NULL) < 0) {\n+\t\tif (git_config(show_all_config, &show_sources) < 0) {\n \t\t\tif (config_exclusive_filename)\n \t\t\t\tdie_errno(\"unable to read config file '%s'\",\n \t\t\t\t\t  config_exclusive_filename);\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..98b1d09 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1138,6 +1138,7 @@ extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \n extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n+extern const char *current_config_filename(void);\n \n extern const char *config_exclusive_filename;\n \ndiff --git a/config.c b/config.c\nindex 40f9c6d..7b4094b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1564,3 +1564,10 @@ int config_error_nonbool(const char *var)\n {\n \treturn error(\"Missing value for '%s'\", var);\n }\n+\n+const char *current_config_filename(void)\n+{\n+\tif (cf && cf->name)\n+\t\treturn cf->name;\n+\treturn NULL;\n+}\n-- \n1.7.9.rc2.14.g3da2b\n"},{"id":"184252","messageId":"201202092024.43381.jnareb@gmail.com","threadId":"29559","inReplyTo":"20120209033059.GA4347@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-09T19:24:42Z","receivedAt":"2012-02-09T19:24:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 9 Feb 2012, Jeff King wrote:\n> On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:\n> > Jeff King <peff@peff.net> writes:\n> > \n> > [...]\n> > > Git-config could potentially help with that (and even simplify the\n> > > current code) by allowing something like:\n> > > \n> > >   $ git config --list-with-sources\n> > >   /home/peff/.gitconfig user.name=Jeff King\n> > >   /home/peff/.gitconfig user.email=peff@peff.net\n> > >   .git/config core.repositoryformatversion=0\n> > >   .git/config core.bare=false\n> > >   [etc]\n> > > \n> > > (you would use the \"-z\" form, of course, and the filenames would be\n> > > NUL-separated, but I made up a human-readable output format above for\n> > > illustration purposes).\n> > \n> > That would be _very_ nice to have (even without includes support).\n> > \n> > Filenames would be git-quoted like in ls-tree / diff-tree output without -z,\n> > isn't it?  And is that TAB or SPC as a separator?\n> \n> So the patch would look something like this. However, is the actual\n> filename really what callers want? It seems like in David's case, an\n> annotation of \"repo\", \"global\", or \"system\" (possibly in addition to the\n> filename) would be the most useful (because in the git-cola UI, it is\n> still nice to list things as \"repo\" or \"global\" instead of spewing the\n> whole filename at the user -- but you would still want the individual\n> filename for handling updates of includes).\n\nI'm not sure if \"system\" / \"global\" / \"local\" or \"repo\" would be a good\nidea.\n\nFirst, in the case of includes you would have to provide pathnames of\nincluded files.  This would introduce inconsistency.  Is \"system\"\nthe '/etc/gitconfig' file, or 'system' file in '.git' directory?\n\nSecond, people can have different build configuration, e.g. the prefix\nmight differ, so that \"system\" is not always '/etc/gitconfig'.  If you\nwant to edit config you would want to know which file to edit... and though\nthere is \"git config --system --edit\" it depends on having editor\nconfigured correctly.\n\n\nJust my two cents.\n-- \nJakub Narebski\nPoland\n"},{"id":"184254","messageId":"20120209193314.GA19690@sigill.intra.peff.net","threadId":"29559","inReplyTo":"201202092024.43381.jnareb@gmail.com","subject":"Re: [PATCH 0/2] config includes, take 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-09T19:33:14Z","receivedAt":"2012-02-09T19:33:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2012 at 08:24:42PM +0100, Jakub Narebski wrote:\n\n> > So the patch would look something like this. However, is the actual\n> > filename really what callers want? It seems like in David's case, an\n> > annotation of \"repo\", \"global\", or \"system\" (possibly in addition to the\n> > filename) would be the most useful (because in the git-cola UI, it is\n> > still nice to list things as \"repo\" or \"global\" instead of spewing the\n> > whole filename at the user -- but you would still want the individual\n> > filename for handling updates of includes).\n> \n> I'm not sure if \"system\" / \"global\" / \"local\" or \"repo\" would be a good\n> idea.\n> \n> First, in the case of includes you would have to provide pathnames of\n> included files.  This would introduce inconsistency.  Is \"system\"\n> the '/etc/gitconfig' file, or 'system' file in '.git' directory?\n\nYeah, it would have to be syntactically unambiguous with the filename.\nI was thinking something of just including both, like this:\n\n  global:/home/peff/.gitconfig<TAB>include.path=other-file\n  global:/home/peff/other-file<TAB>some.key=value\n\nThat is, give a \"context\" (repo, global, system) to each lookup, and\nthen mention the individual file as well (either because it is the root\nof that context, or because it was included). So a config editor could\npresent the context to the user as a purely decorative thing (i.e., tell\nthe user \"these options affect all of your repos\"), but use the filename\nto actually update the values (i.e., \"git config -f\n/home/peff/other-file some.key newvalue\").\n\n> Second, people can have different build configuration, e.g. the prefix\n> might differ, so that \"system\" is not always '/etc/gitconfig'.  If you\n> want to edit config you would want to know which file to edit... and though\n> there is \"git config --system --edit\" it depends on having editor\n> configured correctly.\n\nWithout includes, something like git-cola could possibly get away with:\n\n  git config --system some.key value\n\nbut that doesn't work for included files; for those, you'd want to have\nthe actual filename.\n\n-Peff\n"}]}