{"thread":{"id":"29560","subject":"[PATCH 0/2] config includes 3: revenge of the killer includes","startedAt":"2012-02-06T09:53:06Z","lastAt":"2012-02-08T15:59:50Z","messageCount":16,"participants":["Jeff King","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"184011","messageId":"20120206095306.GA2404@sigill.intra.peff.net","threadId":"29560","inReplyTo":null,"subject":"[PATCH 0/2] config includes 3: revenge of the killer includes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T09:53:06Z","receivedAt":"2012-02-06T09:53:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"So here's a third version of the config include patches that I think are\nsane. For those of you who missed episode 2, our heroes decided to drop\nthe include-from-ref bits from the series, but otherwise incorporated\nall suggestions. There was a surprise cliffhanger ending in which it was\ndiscovered that suppressing duplicate include files was actually the\nvillain!\n\nThis version keeps a simple depth counter to stop cycles from causing\ninfinite loops, but otherwise allows multiple inclusion of files.\n\nAnd now, the exciting conclusion...\n\n  [1/2]: docs: add a basic description of the config API\n  [2/2]: config: add include directive\n\n-Peff\n"},{"id":"184012","messageId":"20120206095346.GA4300@sigill.intra.peff.net","threadId":"29560","inReplyTo":"20120206095306.GA2404@sigill.intra.peff.net","subject":"[PATCH 1/2] docs: add a basic description of the config API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T09:53:46Z","receivedAt":"2012-02-06T09:53:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This wasn't documented at all; this is pretty bare-bones,\nbut it should at least give new git hackers a basic idea of\nhow the reading side works.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/api-config.txt |  101 ++++++++++++++++++++++++++++++++\n 1 files changed, 101 insertions(+), 0 deletions(-)\n create mode 100644 Documentation/technical/api-config.txt\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nnew file mode 100644\nindex 0000000..f428c5c\n--- /dev/null\n+++ b/Documentation/technical/api-config.txt\n@@ -0,0 +1,101 @@\n+config API\n+==========\n+\n+The config API gives callers a way to access git configuration files\n+(and files which have the same syntax). See linkgit:git-config[1] for a\n+discussion of the config file syntax.\n+\n+General Usage\n+-------------\n+\n+Config files are parsed linearly, and each variable found is passed to a\n+caller-provided callback function. The callback function is responsible\n+for any actions to be taken on the config option, and is free to ignore\n+some options (it is not uncommon for the configuration to be parsed\n+several times during the run of a git program, with different callbacks\n+picking out different variables useful to themselves).\n+\n+A config callback function takes three parameters:\n+\n+- the name of the parsed variable. This is in canonical \"flat\" form: the\n+  section, subsection, and variable segments will be separated by dots,\n+  and the section and variable segments will be all lowercase. E.g.,\n+  `core.ignorecase`, `diff.SomeType.textconv`.\n+\n+- the value of the found variable, as a string. If the variable had no\n+  value specified, the value will be NULL (typically this means it\n+  should be interpreted as boolean true).\n+\n+- a void pointer passed in by the caller of the config API; this can\n+  contain callback-specific data\n+\n+A config callback should return 0 for success, or -1 if the variable\n+could not be parsed properly.\n+\n+Basic Config Querying\n+---------------------\n+\n+Most programs will simply want to look up variables in all config files\n+that git knows about, using the normal precedence rules. To do this,\n+call `git_config` with a callback function and void data pointer.\n+\n+`git_config` will read all config sources in order of increasing\n+priority. Thus a callback should typically overwrite previously-seen\n+entries with new ones (e.g., if both the user-wide `~/.gitconfig` and\n+repo-specific `.git/config` contain `color.ui`, the config machinery\n+will first feed the user-wide one to the callback, and then the\n+repo-specific one; by overwriting, the higher-priority repo-specific\n+value is left at the end).\n+\n+There is a special version of `git_config` called `git_config_early`\n+that takes an additional parameter to specify the repository config.\n+This should be used early in a git program when the repository location\n+has not yet been determined (and calling the usual lazy-evaluation\n+lookup rules would yield an incorrect location).\n+\n+Reading Specific Files\n+----------------------\n+\n+To read a specific file in git-config format, use\n+`git_config_from_file`. This takes the same callback and data parameters\n+as `git_config`.\n+\n+Value Parsing Helpers\n+---------------------\n+\n+To aid in parsing string values, the config API provides callbacks with\n+a number of helper functions, including:\n+\n+`git_config_int`::\n+Parse the string to an integer, including unit factors. Dies on error;\n+otherwise, returns the parsed result.\n+\n+`git_config_ulong`::\n+Identical to `git_config_int`, but for unsigned longs.\n+\n+`git_config_bool`::\n+Parse a string into a boolean value, respecting keywords like \"true\" and\n+\"false\". Integer values are converted into true/false values (when they\n+are non-zero or zero, respectively). Other values cause a die(). If\n+parsing is successful, the return value is the result.\n+\n+`git_config_bool_or_int`::\n+Same as `git_config_bool`, except that integers are returned as-is, and\n+an `is_bool` flag is unset.\n+\n+`git_config_maybe_bool`::\n+Same as `git_config_bool`, except that it returns -1 on error rather\n+than dying.\n+\n+`git_config_string`::\n+Allocates and copies the value string into the `dest` parameter; if no\n+string is given, prints an error message and returns -1.\n+\n+`git_config_pathname`::\n+Similar to `git_config_string`, but expands `~` or `~user` into the\n+user's home directory when found at the beginning of the path.\n+\n+Writing Config Files\n+--------------------\n+\n+TODO\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"184014","messageId":"20120206095404.GB4300@sigill.intra.peff.net","threadId":"29560","inReplyTo":"20120206095306.GA2404@sigill.intra.peff.net","subject":"[PATCH 2/2] config: add include directive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T09:54:04Z","receivedAt":"2012-02-06T09:54:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It 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 provides a \"git_config_include\" callback\nwhich wraps regular config callbacks. Callers can pass it to\ngit_config_from_file, and it will transparently follow any\ninclude directives, passing all of the discovered options to\nthe real callback.\n\nInclude directives are turned on automatically for \"regular\"\ngit config parsing. This includes calls to git_config, as\nwell as calls to the \"git config\" program that do not\nspecify a single file (e.g., using \"-f\", \"--global\", etc).\nThey are not turned on 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 Documentation/technical/api-config.txt |   22 ++++++\n builtin/config.c                       |   31 ++++++--\n cache.h                                |    8 ++\n config.c                               |   69 +++++++++++++++++-\n t/t1305-config-include.sh              |  126 ++++++++++++++++++++++++++++++++\n 7 files changed, 268 insertions(+), 8 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/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex f428c5c..c60b6b3 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -95,6 +95,28 @@ string is given, prints an error message and returns -1.\n Similar to `git_config_string`, but expands `~` or `~user` into the\n user's home directory when found at the beginning of the path.\n \n+Include Directives\n+------------------\n+\n+By default, the config parser does not respect include directives.\n+However, a caller can use the special `git_config_include` wrapper\n+callback to support them. To do so, you simply wrap your \"real\" callback\n+function and data pointer in a `struct config_include_data`, and pass\n+the wrapper to the regular config-reading functions. For example:\n+\n+-------------------------------------------\n+int read_file_with_include(const char *file, config_fn_t fn, void *data)\n+{\n+\tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n+\tinc.fn = fn;\n+\tinc.data = data;\n+\treturn git_config_from_file(git_config_include, file, &inc);\n+}\n+-------------------------------------------\n+\n+`git_config` respects includes automatically. The lower-level\n+`git_config_from_file` does not.\n+\n Writing Config Files\n --------------------\n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex d35c06a..09bf778 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 int respect_includes = -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\", &respect_includes, \"respect include directives on lookup\"),\n \tOPT_END(),\n };\n \n@@ -161,6 +163,9 @@ static int get_value(const char *key_, const char *regex_)\n \tint ret = -1;\n \tchar *global = NULL, *repo_config = NULL;\n \tconst char *system_wide = NULL, *local;\n+\tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n+\tconfig_fn_t fn;\n+\tvoid *data;\n \n \tlocal = config_exclusive_filename;\n \tif (!local) {\n@@ -213,19 +218,28 @@ static int get_value(const char *key_, const char *regex_)\n \t\t}\n \t}\n \n+\tfn = show_config;\n+\tdata = NULL;\n+\tif (respect_includes) {\n+\t\tinc.fn = fn;\n+\t\tinc.data = data;\n+\t\tfn = git_config_include;\n+\t\tdata = &inc;\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(fn, system_wide, data);\n \tif (do_all && global)\n-\t\tgit_config_from_file(show_config, global, NULL);\n+\t\tgit_config_from_file(fn, global, data);\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(fn, local, data);\n+\tgit_config_from_parameters(fn, data);\n \tif (!do_all && !seen)\n-\t\tgit_config_from_file(show_config, local, NULL);\n+\t\tgit_config_from_file(fn, local, data);\n \tif (!do_all && !seen && global)\n-\t\tgit_config_from_file(show_config, global, NULL);\n+\t\tgit_config_from_file(fn, global, data);\n \tif (!do_all && !seen && system_wide)\n-\t\tgit_config_from_file(show_config, system_wide, NULL);\n+\t\tgit_config_from_file(fn, system_wide, data);\n \n \tfree(key);\n \tif (regexp) {\n@@ -384,6 +398,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 (respect_includes == -1)\n+\t\trespect_includes = !config_exclusive_filename;\n+\n \tif (end_null) {\n \t\tterm = '\\0';\n \t\tdelim = '\\n';\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..7cf6dc1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1139,6 +1139,14 @@ 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 \n+struct config_include_data {\n+\tint depth;\n+\tconfig_fn_t fn;\n+\tvoid *data;\n+};\n+#define CONFIG_INCLUDE_INIT { 0 }\n+extern int git_config_include(const char *name, const char *value, void *data);\n+\n extern const char *config_exclusive_filename;\n \n #define MAX_GITNAME (1000)\ndiff --git a/config.c b/config.c\nindex 40f9c6d..e3fcf75 100644\n--- a/config.c\n+++ b/config.c\n@@ -28,6 +28,69 @@ static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n \n+#define MAX_INCLUDE_DEPTH 10\n+static const char include_depth_advice[] =\n+\"exceeded maximum include depth (%d) while including\\n\"\n+\"\t%s\\n\"\n+\"from\\n\"\n+\"\t%s\\n\"\n+\"Do you have circular includes?\";\n+static int handle_path_include(const char *path, struct config_include_data *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 come 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_addstr(&buf, path);\n+\t\tpath = buf.buf;\n+\t}\n+\n+\tif (!access(path, R_OK)) {\n+\t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n+\t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n+\t\t\t    cf && cf->name ? cf->name : \"the command line\");\n+\t\tret = git_config_from_file(git_config_include, path, inc);\n+\t\tinc->depth--;\n+\t}\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+int git_config_include(const char *var, const char *value, void *data)\n+{\n+\tstruct config_include_data *inc = data;\n+\tconst char *type;\n+\tint ret;\n+\n+\t/*\n+\t * Pass along all values, including \"include\" directives; this makes it\n+\t * possible to query information on the includes themselves.\n+\t */\n+\tret = inc->fn(var, value, inc->data);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\n+\ttype = skip_prefix(var, \"include.\");\n+\tif (!type)\n+\t\treturn ret;\n+\n+\tif (!strcmp(type, \"path\"))\n+\t\tret = handle_path_include(value, inc);\n+\treturn ret;\n+}\n+\n static void lowercase(char *p)\n {\n \tfor (; *p; p++)\n@@ -921,9 +984,13 @@ int git_config(config_fn_t fn, void *data)\n {\n \tchar *repo_config = NULL;\n \tint ret;\n+\tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n+\n+\tinc.fn = fn;\n+\tinc.data = data;\n \n \trepo_config = git_pathdup(\"config\");\n-\tret = git_config_early(fn, data, repo_config);\n+\tret = git_config_early(git_config_include, &inc, repo_config);\n \tif (repo_config)\n \t\tfree(repo_config);\n \treturn ret;\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nnew file mode 100755\nindex 0000000..0a27ec4\n--- /dev/null\n+++ b/t/t1305-config-include.sh\n@@ -0,0 +1,126 @@\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' '\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+\ttest_must_fail git config --get-all test.value 2>stderr &&\n+\tgrep \"exceeded maximum include depth\" stderr\n+'\n+\n+test_done\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"184070","messageId":"7vbopb61cd.fsf@alter.siamese.dyndns.org","threadId":"29560","inReplyTo":"20120206095346.GA4300@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T22:31:14Z","receivedAt":"2012-02-06T22:31:14Z","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> This wasn't documented at all; this is pretty bare-bones,\n> but it should at least give new git hackers a basic idea of\n> how the reading side works.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Documentation/technical/api-config.txt |  101 ++++++++++++++++++++++++++++++++\n>  1 files changed, 101 insertions(+), 0 deletions(-)\n>  create mode 100644 Documentation/technical/api-config.txt\n>\n> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n> new file mode 100644\n> index 0000000..f428c5c\n> --- /dev/null\n> +++ b/Documentation/technical/api-config.txt\n> @@ -0,0 +1,101 @@\n> +config API\n> +==========\n> +\n> +The config API gives callers a way to access git configuration files\n> +(and files which have the same syntax). See linkgit:git-config[1] for a\n> +discussion of the config file syntax.\n> +\n> +General Usage\n> +-------------\n> +\n> +Config files are parsed linearly, and each variable found is passed to a\n> +caller-provided callback function. The callback function is responsible\n> +for any actions to be taken on the config option, and is free to ignore\n> +some options (it is not uncommon for the configuration to be parsed\n> +several times during the run of a git program, with different callbacks\n> +picking out different variables useful to themselves).\n\nIt woud be easeier to read if you stopped the sentence after \"some\noptions\" and made the \"It is not uncommon...\" a first-class sentence\noutside the parentheses.\n\n> +A config callback function takes three parameters:\n> +\n> +- the name of the parsed variable. This is in canonical \"flat\" form: the\n> +  section, subsection, and variable segments will be separated by dots,\n> +  and the section and variable segments will be all lowercase. E.g.,\n> +  `core.ignorecase`, `diff.SomeType.textconv`.\n> +\n> +- the value of the found variable, as a string. If the variable had no\n> +  value specified, the value will be NULL (typically this means it\n> +  should be interpreted as boolean true).\n> +\n> +- a void pointer passed in by the caller of the config API; this can\n> +  contain callback-specific data\n> +\n> +A config callback should return 0 for success, or -1 if the variable\n> +could not be parsed properly.\n\nThis matches what I have always thought, but I think I recently saw a\nseries that adds callbacks that return 1 to mean \"I have understood this\nvariable, so callers should not look at it any more\".  It felt wrong, but\nI did not find anything in the config.c API framework to prvent such a\nlocal calling convention.\n\n> +Basic Config Querying\n> +---------------------\n> +\n> +Most programs will simply want to look up variables in all config files\n> +that git knows about, using the normal precedence rules. To do this,\n> +call `git_config` with a callback function and void data pointer.\n> +\n> +`git_config` will read all config sources in order of increasing\n> +priority. Thus a callback should typically overwrite previously-seen\n> +entries with new ones (e.g., if both the user-wide `~/.gitconfig` and\n> +repo-specific `.git/config` contain `color.ui`, the config machinery\n> +will first feed the user-wide one to the callback, and then the\n> +repo-specific one; by overwriting, the higher-priority repo-specific\n> +value is left at the end).\n> +\n> +There is a special version of `git_config` called `git_config_early`\n> +that takes an additional parameter to specify the repository config.\n> +This should be used early in a git program when the repository location\n> +has not yet been determined (and calling the usual lazy-evaluation\n> +lookup rules would yield an incorrect location).\n\nDo you want to say somethink like \"Ordinary programs should not have to\nworry about git_config_early()\"?  Differently put, if you are learning the\nconfig API by reading this document and cannot tell which one you should\nbe calling, you are way too inexperienced to call git_config_early() and\nyou would always want to call git_config()?\n"},{"id":"184071","messageId":"7v62fj60ya.fsf@alter.siamese.dyndns.org","threadId":"29560","inReplyTo":"20120206095404.GB4300@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] config: add include directive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T22:39:41Z","receivedAt":"2012-02-06T22:39:41Z","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> +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\nIf the file referenced by this directive does not exist, what should\nhappen?  Should it be signalled as an error?  Should it stop the whole\ncalling process with die()?\n\nI think \"die() when we are honoring the include, ignore when we are not\"\nwould be a good way to handle this, as it allows us to catch mistakes\nwhile allowing the user to fix broken configuration files using \"git\nconfig --unset include.path\", but I may be overlooking something.\n"},{"id":"184147","messageId":"20120207180625.GA27189@sigill.intra.peff.net","threadId":"29560","inReplyTo":"7vbopb61cd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T18:06:25Z","receivedAt":"2012-02-07T18:06:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 02:31:14PM -0800, Junio C Hamano wrote:\n\n> > +Config files are parsed linearly, and each variable found is passed to a\n> > +caller-provided callback function. The callback function is responsible\n> > +for any actions to be taken on the config option, and is free to ignore\n> > +some options (it is not uncommon for the configuration to be parsed\n> > +several times during the run of a git program, with different callbacks\n> > +picking out different variables useful to themselves).\n> \n> It woud be easeier to read if you stopped the sentence after \"some\n> options\" and made the \"It is not uncommon...\" a first-class sentence\n> outside the parentheses.\n\nThanks. Putting in too many parenthetical phrases is a bad habit of\nmine.  Usually I notice around the time I try nesting a parenthetical\nphrase instead of existing parentheses, and go back and at least promote\nthe outer one to a real sentence. :)\n\nWill fix for the re-roll.\n\n> > +A config callback should return 0 for success, or -1 if the variable\n> > +could not be parsed properly.\n> \n> This matches what I have always thought, but I think I recently saw a\n> series that adds callbacks that return 1 to mean \"I have understood this\n> variable, so callers should not look at it any more\".  It felt wrong, but\n> I did not find anything in the config.c API framework to prvent such a\n> local calling convention.\n\nThe return value is propagated from git_parse_file. I assume the\noriginal intent was that you could actually use it to return an integer\nfrom your callback. In practice, though, callbacks either modify global\ndata (for older instances), or modify a pointer passed through the void\ndata pointer.\n\nThe \"1 means I understood this\" convention is used by userdiff_config. I\ndon't like that it is unlike every other config callback, but I think\nit's necessary to deal with the ambiguity of \"diff.color.*\", which could\nbe either a userdiff entry or a diff color. E.g., if we see\n\"diff.color.binary\", we know that it is the \"binary\" variable of the\n\"color\" diff driver, not the color spec for the \"binary\" slot.\n\nLooking at the code again, though, it seems that there is no overlap\nbetween the userdiff slots and the color slots, and that the\ncolor-parsing code will silently ignore any unknown slots. So it would\nbe safe to further investigate diff.color.binary as a color, as we would\nsilently ignore it.\n\nHmm. Yeah. The userdiff calling convention dates back to late 2008. At\nthat time, parse_diff_color_slot would die() if it did not understand\nthe slot, making the \"I understood this\" flag required. Then later, in\n8b8e862 (ignore unknown color configuration, 2009-12-12), it was\nrelaxed.\n\nSo I think we could go back and simplify the userdiff_config code now.\n\n> > +There is a special version of `git_config` called `git_config_early`\n> > +that takes an additional parameter to specify the repository config.\n> > +This should be used early in a git program when the repository location\n> > +has not yet been determined (and calling the usual lazy-evaluation\n> > +lookup rules would yield an incorrect location).\n> \n> Do you want to say somethink like \"Ordinary programs should not have to\n> worry about git_config_early()\"?  Differently put, if you are learning the\n> config API by reading this document and cannot tell which one you should\n> be calling, you are way too inexperienced to call git_config_early() and\n> you would always want to call git_config()?\n\nYes, I think that would be sensible.\n\nThough having looked a lot at the config code recently, I wonder if\ngit_config_early is really necessary. The only caller (besides\ngit_config) is check_repository_format_early, which just wants to see if\ncore.repositoryformatversion in a directory is sane.\n\nBut why in the world would it want to do the full config lookup?\nShouldn't it be checking git_config_from_file directly on the config\nfile in the proposed repo?\n\n-Peff\n"},{"id":"184148","messageId":"20120207182302.GA31059@sigill.intra.peff.net","threadId":"29560","inReplyTo":"20120207180625.GA27189@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T18:23:02Z","receivedAt":"2012-02-07T18:23:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 01:06:25PM -0500, Jeff King wrote:\n\n> So I think we could go back and simplify the userdiff_config code now.\n\nThat patch would look like this.\n\n-- >8 --\nSubject: drop odd return value semantics from userdiff_config\n\nWhen the userdiff_config function was introduced in be58e70\n(diff: unify external diff and funcname parsing code,\n2008-10-05), it used a return value convention unlike any\nother config callback. Like other callbacks, it used \"-1\" to\nsignal error. But it returned \"1\" to indicate that it found\nsomething, and \"0\" otherwise; other callbacks simply\nreturned \"0\" to indicate that no error occurred.\n\nThis distinction was necessary at the time, because the\nuserdiff namespace overlapped slightly with the color\nconfiguration namespace. So \"diff.color.foo\" could mean \"the\n'foo' slot of diff coloring\" or \"the 'foo' component of the\n\"color\" userdiff driver\". Because the color-parsing code\nwould die on an unknown color slot, we needed the userdiff\ncode to indicate that it had matched the variable, letting\nus bypass the color-parsing code entirely.\n\nLater, in 8b8e862 (ignore unknown color configuration,\n2009-12-12), the color-parsing code learned to silently\nignore unknown slots. This means we no longer need to\nprotect userdiff-matched variables from reaching the\ncolor-parsing code.\n\nWe can therefore change the userdiff_config calling\nconvention to a more normal one. This drops some code from\neach caller, which is nice. But more importantly, it reduces\nthe cognitive load for readers who may wonder why\nuserdiff_config is unlike every other config callback.\n\nThere's no need to add a new test confirming that this\nworks; t4020 already contains a test that sets\ndiff.color.external.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c    |    8 +-------\n builtin/cat-file.c |    8 +-------\n builtin/grep.c     |    7 ++-----\n diff.c             |    7 ++-----\n userdiff.c         |   19 ++++++-------------\n 5 files changed, 12 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 5a67c20..01956c8 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tswitch (userdiff_config(var, value)) {\n-\tcase 0:\n-\t\tbreak;\n-\tcase -1:\n+\tif (userdiff_config(var, value) < 0)\n \t\treturn -1;\n-\tdefault:\n-\t\treturn 0;\n-\t}\n \n \treturn git_default_config(var, value, cb);\n }\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 07bd984..8ed501f 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -226,14 +226,8 @@ static const char * const cat_file_usage[] = {\n \n static int git_cat_file_config(const char *var, const char *value, void *cb)\n {\n-\tswitch (userdiff_config(var, value)) {\n-\tcase 0:\n-\t\tbreak;\n-\tcase -1:\n+\tif (userdiff_config(var, value) < 0)\n \t\treturn -1;\n-\tdefault:\n-\t\treturn 0;\n-\t}\n \n \treturn git_default_config(var, value, cb);\n }\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5c2ae94..dc6de83 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -325,11 +325,8 @@ static int grep_config(const char *var, const char *value, void *cb)\n \tstruct grep_opt *opt = cb;\n \tchar *color = NULL;\n \n-\tswitch (userdiff_config(var, value)) {\n-\tcase 0: break;\n-\tcase -1: return -1;\n-\tdefault: return 0;\n-\t}\n+\tif (userdiff_config(var, value) < 0)\n+\t\treturn -1;\n \n \tif (!strcmp(var, \"grep.extendedregexp\")) {\n \t\tif (git_config_bool(var, value))\ndiff --git a/diff.c b/diff.c\nindex 7e15426..a656f5e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,11 +177,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tswitch (userdiff_config(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n+\tif (userdiff_config(var, value) < 0)\n+\t\treturn -1;\n \n \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n \t\tint slot = parse_diff_color_slot(var, 11);\ndiff --git a/userdiff.c b/userdiff.c\nindex 76109da..1e7184f 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -210,14 +210,7 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,\n \tif (git_config_string(&f->pattern, k, v) < 0)\n \t\treturn -1;\n \tf->cflags = cflags;\n-\treturn 1;\n-}\n-\n-static int parse_string(const char **d, const char *k, const char *v)\n-{\n-\tif (git_config_string(d, k, v) < 0)\n-\t\treturn -1;\n-\treturn 1;\n+\treturn 0;\n }\n \n static int parse_tristate(int *b, const char *k, const char *v)\n@@ -226,13 +219,13 @@ static int parse_tristate(int *b, const char *k, const char *v)\n \t\t*b = -1;\n \telse\n \t\t*b = git_config_bool(k, v);\n-\treturn 1;\n+\treturn 0;\n }\n \n static int parse_bool(int *b, const char *k, const char *v)\n {\n \t*b = git_config_bool(k, v);\n-\treturn 1;\n+\treturn 0;\n }\n \n int userdiff_config(const char *k, const char *v)\n@@ -246,13 +239,13 @@ int userdiff_config(const char *k, const char *v)\n \tif ((drv = parse_driver(k, v, \"binary\")))\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif ((drv = parse_driver(k, v, \"command\")))\n-\t\treturn parse_string(&drv->external, k, v);\n+\t\treturn git_config_string(&drv->external, k, v);\n \tif ((drv = parse_driver(k, v, \"textconv\")))\n-\t\treturn parse_string(&drv->textconv, k, v);\n+\t\treturn git_config_string(&drv->textconv, k, v);\n \tif ((drv = parse_driver(k, v, \"cachetextconv\")))\n \t\treturn parse_bool(&drv->textconv_want_cache, k, v);\n \tif ((drv = parse_driver(k, v, \"wordregex\")))\n-\t\treturn parse_string(&drv->word_regex, k, v);\n+\t\treturn git_config_string(&drv->word_regex, k, v);\n \n \treturn 0;\n }\n-- \n1.7.8.4.12.g3a22e3\n"},{"id":"184151","messageId":"20120207183644.GB32367@sigill.intra.peff.net","threadId":"29560","inReplyTo":"7v62fj60ya.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] config: add include directive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T18:36:44Z","receivedAt":"2012-02-07T18:36:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 02:39:41PM -0800, Junio C Hamano wrote:\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> If the file referenced by this directive does not exist, what should\n> happen?  Should it be signalled as an error?  Should it stop the whole\n> calling process with die()?\n\nI silently ignore it. My thinking was that you might want to have\nsomething like:\n\n  [include]\n          path = .gitconfig-local\n\nin a stock .gitconfig that you ship to all of your machines[1]. Then\nmachines that need it can put things in .gitconfig-local, and those that\ndon't can just ignore it.\n\nIt is a tradeoff, of course, in that typos will be silently ignored. For\nthis use case, you could also just create an empty .gitconfig-local on\nmachines that don't have anything to put there.\n\n[1] Actually, a similar use might be a ~/.gitconfig that is shared by a\n    mounted home directory (e.g., via NFS) NFS, and a ~/.gitconfig-$HOST\n    that is specific to each machine. The current code doesn't expand\n    environment variables (nor tildes), but perhaps it should.\n\n> I think \"die() when we are honoring the include, ignore when we are not\"\n> would be a good way to handle this, as it allows us to catch mistakes\n> while allowing the user to fix broken configuration files using \"git\n> config --unset include.path\", but I may be overlooking something.\n\nThe writing path does not use the include callback wrapper at all; so\ninclude.path can be manipulated just as any other variable, and the\nvalue is not treated specially.\n\n-Peff\n"},{"id":"184152","messageId":"7vlioe1o1g.fsf@alter.siamese.dyndns.org","threadId":"29560","inReplyTo":"20120207180625.GA27189@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T18:44:27Z","receivedAt":"2012-02-07T18:44:27Z","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>> > +A config callback should return 0 for success, or -1 if the variable\n>> > +could not be parsed properly.\n>> \n>> This matches what I have always thought, but I think I recently saw a\n>> series that adds callbacks that return 1 to mean \"I have understood this\n>> variable, so callers should not look at it any more\".  It felt wrong, but\n>> I did not find anything in the config.c API framework to prvent such a\n>> local calling convention.\n>\n> ...\n> The \"1 means I understood this\" convention is used by userdiff_config. I\n> don't like that it is unlike every other config callback,...\n> Looking at the code again, though, ...\n> Hmm. Yeah. The userdiff calling convention dates back to late 2008....\n> So I think we could go back and simplify the userdiff_config code now.\n\nI remembered where I saw the new \"offender\"; it was nd/columns\ntopic (Cc'ing Nguyễn).\n"},{"id":"184153","messageId":"7vhaz21nzk.fsf@alter.siamese.dyndns.org","threadId":"29560","inReplyTo":"20120207182302.GA31059@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T18:45:35Z","receivedAt":"2012-02-07T18:45:35Z","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> -- >8 --\n> Subject: drop odd return value semantics from userdiff_config\n> ...\n>  5 files changed, 12 insertions(+), 37 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 5a67c20..01956c8 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tswitch (userdiff_config(var, value)) {\n> -\tcase 0:\n> -\t\tbreak;\n> -\tcase -1:\n> +\tif (userdiff_config(var, value) < 0)\n>  \t\treturn -1;\n> -\tdefault:\n> -\t\treturn 0;\n> -\t}\n>  \n>  \treturn git_default_config(var, value, cb);\n>  }\n> ...\n\nLooks very nice ;-)\n"},{"id":"184160","messageId":"20120207194635.GA4136@sigill.intra.peff.net","threadId":"29560","inReplyTo":"20120207180625.GA27189@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T19:46:35Z","receivedAt":"2012-02-07T19:46:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 01:06:25PM -0500, Jeff King wrote:\n\n> Though having looked a lot at the config code recently, I wonder if\n> git_config_early is really necessary. The only caller (besides\n> git_config) is check_repository_format_early, which just wants to see if\n> core.repositoryformatversion in a directory is sane.\n> \n> But why in the world would it want to do the full config lookup?\n> Shouldn't it be checking git_config_from_file directly on the config\n> file in the proposed repo?\n\nI prepared the patch below to fix this, but it fails t0001. It turns out\nthat check_repository_format_gently checks not only stuff that should be\nin .git/config, but also checks and remembers core.sharedrepository in\nthe process, which can come from anywhere.\n\nSo I think the \"most correct\" thing to do would be:\n\n  - check_repository_format_gently reads _only_ from .git/config\n\n  - core.sharedrepository should come from git_config_default\n\nBut that just creates more headaches. Callers like init_db would need to\ncall git_config separately. But of course they don't have the repo set\nup properly, so they would want git_config_early. So we don't end up\ngetting to remove git_config_early, anyway.\n\nAnd though it is slightly insane that you can do:\n\n  git config --global core.repositoryformatversion 0\n\nor even:\n\n  git -c core.repositoryformatversion=0 ...\n\nand it is respected, in practice nobody does this. So it's probably\nbetter to just leave the code as-is.\n\n-Peff\n\n-- >8 --\nSubject: don't look for repositoryformatversion outside of repo\n\nIn the check_repository_format_gently function, we use\ngit_config_early to parse the repository format version from\nthe .git/config file.\n\nHowever, git_config_early looks in all of the sources of git\nconfig, including /etc/gitconfig, ~/.gitconfig, and the\ncommand line. But we should really only be interested in getting\nthe value from the repository's config file, since the point\nof this function is to check the repository itself.\n\nTherefore we can just feed our repo-config argument directly\nto git_config_from_file. As a bonus, this means the subtle\ndistinction in using git_config_early versus git_config can\njust go away.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n setup.c |   11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 61c22e6..20509cf 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -319,17 +319,8 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n {\n \tchar repo_config[PATH_MAX+1];\n \n-\t/*\n-\t * git_config() can't be used here because it calls git_pathdup()\n-\t * to get $GIT_CONFIG/config. That call will make setup_git_env()\n-\t * set git_dir to \".git\".\n-\t *\n-\t * We are in gitdir setup, no git dir has been found useable yet.\n-\t * Use a gentler version of git_config() to check if this repo\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_from_file(check_repository_format_version, 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\",\n-- \n1.7.8.4.12.g3a22e3\n"},{"id":"184172","messageId":"CACsJy8AU3ZA1=Q3vahhP6Nr=FZNKd7oRJ04mtKVs+uvNqJeVaw@mail.gmail.com","threadId":"29560","inReplyTo":"7vlioe1o1g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-08T04:01:10Z","receivedAt":"2012-02-08T04:01:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/2/8 Junio C Hamano <gitster@pobox.com>:\n> Jeff King <peff@peff.net> writes:\n>\n>>> > +A config callback should return 0 for success, or -1 if the variable\n>>> > +could not be parsed properly.\n>>>\n>>> This matches what I have always thought, but I think I recently saw a\n>>> series that adds callbacks that return 1 to mean \"I have understood this\n>>> variable, so callers should not look at it any more\".  It felt wrong, but\n>>> I did not find anything in the config.c API framework to prvent such a\n>>> local calling convention.\n>>\n>> ...\n>> The \"1 means I understood this\" convention is used by userdiff_config. I\n>> don't like that it is unlike every other config callback,...\n>> Looking at the code again, though, ...\n>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....\n>> So I think we could go back and simplify the userdiff_config code now.\n>\n> I remembered where I saw the new \"offender\"; it was nd/columns\n> topic (Cc'ing Nguyễn).\n\nnd/columns does use \"1\" convention in git_column_config(), but the\ndirect config callback function does not return 1 to config machinery.\nAll call sites follow this pattern:\n\nint ret = git_column_config(key, var, \"command\", &colopts);\nif (ret <= 0) return ret;\n\nI think it's ok.\n-- \nDuy\n"},{"id":"184177","messageId":"7vmx8tzv3l.fsf@alter.siamese.dyndns.org","threadId":"29560","inReplyTo":"CACsJy8AU3ZA1=Q3vahhP6Nr=FZNKd7oRJ04mtKVs+uvNqJeVaw@mail.gmail.com","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-08T06:40:14Z","receivedAt":"2012-02-08T06:40:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n>>> The \"1 means I understood this\" convention is used by userdiff_config. I\n>>> don't like that it is unlike every other config callback,...\n>>> Looking at the code again, though, ...\n>>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....\n>>> So I think we could go back and simplify the userdiff_config code now.\n>>\n>> I remembered where I saw the new \"offender\"; it was nd/columns\n>> topic (Cc'ing Nguyễn).\n>\n> nd/columns does use \"1\" convention in git_column_config(), but the\n> direct config callback function does not return 1 to config machinery.\n> All call sites follow this pattern:\n>\n> int ret = git_column_config(key, var, \"command\", &colopts);\n> if (ret <= 0) return ret;\n>\n> I think it's ok.\n\nI too think this should be acceptable, but that is not the point.\n\nYour excuse that \"the toplevel callback in my callchain never returns 1,\nso overall the nd/columns series is ok\" just muddies the water.  It means\nif later somebody wanted to use inner callback functions you use from the\ngit_column_config() callchain chain as a toplevel callback for whatever\nreason, that will violate the \"0 for success, or -1 for error\" convention.\nMore importantly, if somebody wants to turn a top-level callback that\ncurrently returns 0 into a sub callback used by his callback callchain, he\ncannot change that existing callback to return 1 to tell him to short\ncircuit, because for other callers returning 1 would be a violation.\n\nWhat I was getting at is that we probably should officially declare that\nreturning 1 to signal success is perfectly acceptable (and it probably\nshould mean the caller who called the callback function as a sub callback\nshould return immediately, taking it as a signal that the key has already\nbeen handled), as the primary purpose of this thread to discuss Peff's\npatch is to write these rules down.\n\nOf course, all that relies on the audit of the git_config() machinery. I\nthink it is written to accept non-negative as success, and that is why I\nsaid \"I too think this should be acceptable\" in the first place.\n"},{"id":"184178","messageId":"CACsJy8B1cMBbrUznOp95u8YmfLf5bbzox8g9cuQUwgf-vY1XrQ@mail.gmail.com","threadId":"29560","inReplyTo":"7vmx8tzv3l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-08T06:55:30Z","receivedAt":"2012-02-08T06:55:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 8, 2012 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Your excuse that \"the toplevel callback in my callchain never returns 1,\n> so overall the nd/columns series is ok\" just muddies the water.  It means\n> if later somebody wanted to use inner callback functions you use from the\n> git_column_config() callchain chain as a toplevel callback for whatever\n> reason, that will violate the \"0 for success, or -1 for error\" convention.\n> More importantly, if somebody wants to turn a top-level callback that\n> currently returns 0 into a sub callback used by his callback callchain, he\n> cannot change that existing callback to return 1 to tell him to short\n> circuit, because for other callers returning 1 would be a violation.\n>\n> What I was getting at is that we probably should officially declare that\n> returning 1 to signal success is perfectly acceptable (and it probably\n> should mean the caller who called the callback function as a sub callback\n> should return immediately, taking it as a signal that the key has already\n> been handled), as the primary purpose of this thread to discuss Peff's\n> patch is to write these rules down.\n\nOr allow multiple callbacks from git_config(). The problem with is it\nadds another common set of config vars. Now it's common to chain var\nsets by doing\n\nint config_callback(...)\n{\n    ....\n    return yet_another_callback(...);\n}\n\nint main()\n{\n   git_config(config_callback, ...)\n   ...\n\nwhere yet_another_callback() hard codes another callback (usually\ngit_default_config). This is inflexible. If we allow multiple\ncallbacks, git_column_config() could be changed to return just 0 or -1\n(i.e. 1 remains unsuccessful error code).\n\nOn second thought, I think calling git_config() multiple times, each\ntime with one callback, may work too. We may want to cache config\ncontent to avoid accessing files too many times though.\n\n> Of course, all that relies on the audit of the git_config() machinery. I\n> think it is written to accept non-negative as success, and that is why I\n> said \"I too think this should be acceptable\" in the first place.\n-- \nDuy\n"},{"id":"184189","messageId":"20120208154855.GC8773@sigill.intra.peff.net","threadId":"29560","inReplyTo":"7vmx8tzv3l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T15:48:55Z","receivedAt":"2012-02-08T15:48:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 10:40:14PM -0800, Junio C Hamano wrote:\n\n> What I was getting at is that we probably should officially declare that\n> returning 1 to signal success is perfectly acceptable (and it probably\n> should mean the caller who called the callback function as a sub callback\n> should return immediately, taking it as a signal that the key has already\n> been handled), as the primary purpose of this thread to discuss Peff's\n> patch is to write these rules down.\n> \n> Of course, all that relies on the audit of the git_config() machinery. I\n> think it is written to accept non-negative as success, and that is why I\n> said \"I too think this should be acceptable\" in the first place.\n\nI looked through it the other day when this came up, and I believe that\nis the case. There is another related rule that should be considered,\nthough: should the return value from callbacks be propagated via\ngit_config to its caller?\n\nIt is the case already for config files that are read, but not for\nconfig options parsed from the command line. It would not be too hard to\nchange, but I do not think any current callers care (as I mentioned\nearlier, these days with the \"void *\" data pointer, sane callers will\nsimply pass in a pointer to a modifiable data area).\n\n-Peff\n"},{"id":"184190","messageId":"20120208155950.GD8773@sigill.intra.peff.net","threadId":"29560","inReplyTo":"CACsJy8B1cMBbrUznOp95u8YmfLf5bbzox8g9cuQUwgf-vY1XrQ@mail.gmail.com","subject":"Re: [PATCH 1/2] docs: add a basic description of the config API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T15:59:50Z","receivedAt":"2012-02-08T15:59:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2012 at 01:55:30PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> Or allow multiple callbacks from git_config(). The problem with is it\n> adds another common set of config vars. Now it's common to chain var\n> sets by doing\n> \n> int config_callback(...)\n> {\n>     ....\n>     return yet_another_callback(...);\n> }\n> \n> int main()\n> {\n>    git_config(config_callback, ...)\n>    ...\n> \n> where yet_another_callback() hard codes another callback (usually\n> git_default_config). This is inflexible. If we allow multiple\n> callbacks, git_column_config() could be changed to return just 0 or -1\n> (i.e. 1 remains unsuccessful error code).\n\nI don't think we need multiple callbacks. You could convert any such\ncall of:\n\n  git_config_multiple(cb1, cb2, cb3, NULL, NULL);\n\ninto:\n\n  int combining_callback(const char *var, const char *value, void *data)\n  {\n          if (cb1(var, value, data) < 0)\n                  return -1;\n          if (cb2(var, value, data) < 0)\n                  return -1;\n          if (cb3(var, value, data) < 0)\n                  return -1;\n          return 0;\n  }\n\nBut note that you get the same \"data\" pointer in each case. If you\nwanted different data pointers, you would need more support from the\nconfig machinery. However, I'd rather that be spelled:\n\n  git_config(cb1, data1);\n  git_config(cb2, data2);\n  git_config(cb3, data3);\n\nand if the efficiency isn't acceptable, then looking into caching the\nkey/value pairs.\n\n> On second thought, I think calling git_config() multiple times, each\n> time with one callback, may work too. We may want to cache config\n> content to avoid accessing files too many times though.\n\nExactly. I would do that first, and then worry about efficiency later if\nit comes up. The first time I saw that we cached config multiple times\nin a program run (several years ago), I had the same thought. But I\ndon't think the performance impact for reading the config 2 or 3 times\ninstead of 1 was even measurable, so I stopped looking into it.\n\nIf were to adopt something like the \"automatically run this program to\nget the config value\" proposal that has been floating around (and I am\nnot saying we should), _then_ I think it might make sense to cache the\nvalues, because the sub-program might be expensive to run.\n\n-Peff\n"}]}