[PATCH 0/2] config includes 3: revenge of the killer includes

[PATCH 0/2] config includes 3: revenge of the killer includes

[Jeff King]

So here's a third version of the config include patches that I think are
sane. For those of you who missed episode 2, our heroes decided to drop
the include-from-ref bits from the series, but otherwise incorporated
all suggestions. There was a surprise cliffhanger ending in which it was
discovered that suppressing duplicate include files was actually the
villain!

This version keeps a simple depth counter to stop cycles from causing
infinite loops, but otherwise allows multiple inclusion of files.

And now, the exciting conclusion...

  [1/2]: docs: add a basic description of the config API
  [2/2]: config: add include directive

-Peff

[PATCH 1/2] docs: add a basic description of the config API

[Jeff King]

This wasn't documented at all; this is pretty bare-bones,
but it should at least give new git hackers a basic idea of
how the reading side works.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-config.txt |  101 ++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/technical/api-config.txt

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
new file mode 100644
index 0000000..f428c5c
--- /dev/null
+++ b/Documentation/technical/api-config.txt
@@ -0,0 +1,101 @@
+config API
+==========
+
+The config API gives callers a way to access git configuration files
+(and files which have the same syntax). See linkgit:git-config[1] for a
+discussion of the config file syntax.
+
+General Usage
+-------------
+
+Config files are parsed linearly, and each variable found is passed to a
+caller-provided callback function. The callback function is responsible
+for any actions to be taken on the config option, and is free to ignore
+some options (it is not uncommon for the configuration to be parsed
+several times during the run of a git program, with different callbacks
+picking out different variables useful to themselves).
+
+A config callback function takes three parameters:
+
+- the name of the parsed variable. This is in canonical "flat" form: the
+  section, subsection, and variable segments will be separated by dots,
+  and the section and variable segments will be all lowercase. E.g.,
+  `core.ignorecase`, `diff.SomeType.textconv`.
+
+- the value of the found variable, as a string. If the variable had no
+  value specified, the value will be NULL (typically this means it
+  should be interpreted as boolean true).
+
+- a void pointer passed in by the caller of the config API; this can
+  contain callback-specific data
+
+A config callback should return 0 for success, or -1 if the variable
+could not be parsed properly.
+
+Basic Config Querying
+---------------------
+
+Most programs will simply want to look up variables in all config files
+that git knows about, using the normal precedence rules. To do this,
+call `git_config` with a callback function and void data pointer.
+
+`git_config` will read all config sources in order of increasing
+priority. Thus a callback should typically overwrite previously-seen
+entries with new ones (e.g., if both the user-wide `~/.gitconfig` and
+repo-specific `.git/config` contain `color.ui`, the config machinery
+will first feed the user-wide one to the callback, and then the
+repo-specific one; by overwriting, the higher-priority repo-specific
+value is left at the end).
+
+There is a special version of `git_config` called `git_config_early`
+that takes an additional parameter to specify the repository config.
+This should be used early in a git program when the repository location
+has not yet been determined (and calling the usual lazy-evaluation
+lookup rules would yield an incorrect location).
+
+Reading Specific Files
+----------------------
+
+To read a specific file in git-config format, use
+`git_config_from_file`. This takes the same callback and data parameters
+as `git_config`.
+
+Value Parsing Helpers
+---------------------
+
+To aid in parsing string values, the config API provides callbacks with
+a number of helper functions, including:
+
+`git_config_int`::
+Parse the string to an integer, including unit factors. Dies on error;
+otherwise, returns the parsed result.
+
+`git_config_ulong`::
+Identical to `git_config_int`, but for unsigned longs.
+
+`git_config_bool`::
+Parse a string into a boolean value, respecting keywords like "true" and
+"false". Integer values are converted into true/false values (when they
+are non-zero or zero, respectively). Other values cause a die(). If
+parsing is successful, the return value is the result.
+
+`git_config_bool_or_int`::
+Same as `git_config_bool`, except that integers are returned as-is, and
+an `is_bool` flag is unset.
+
+`git_config_maybe_bool`::
+Same as `git_config_bool`, except that it returns -1 on error rather
+than dying.
+
+`git_config_string`::
+Allocates and copies the value string into the `dest` parameter; if no
+string is given, prints an error message and returns -1.
+
+`git_config_pathname`::
+Similar to `git_config_string`, but expands `~` or `~user` into the
+user's home directory when found at the beginning of the path.
+
+Writing Config Files
+--------------------
+
+TODO
-- 
1.7.9.rc1.29.g43677

[PATCH 2/2] config: add include directive

[Jeff King]

It can be useful to split your ~/.gitconfig across multiple
files. For example, you might have a "main" file which is
used on many machines, but a small set of per-machine
tweaks. Or you may want to make some of your config public
(e.g., clever aliases) while keeping other data back (e.g.,
your name or other identifying information). Or you may want
to include a number of config options in some subset of your
repos without copying and pasting (e.g., you want to
reference them from the .git/config of participating repos).

This patch introduces an include directive for config files.
It looks like:

  [include]
    path = /path/to/file

This is syntactically backwards-compatible with existing git
config parsers (i.e., they will see it as another config
entry and ignore it unless you are looking up include.path).

The implementation provides a "git_config_include" callback
which wraps regular config callbacks. Callers can pass it to
git_config_from_file, and it will transparently follow any
include directives, passing all of the discovered options to
the real callback.

Include directives are turned on automatically for "regular"
git config parsing. This includes calls to git_config, as
well as calls to the "git config" program that do not
specify a single file (e.g., using "-f", "--global", etc).
They are not turned on in other cases, including:

  1. Parsing of other config-like files, like .gitmodules.
     There isn't a real need, and I'd rather be conservative
     and avoid unnecessary incompatibility or confusion.

  2. Reading single files via "git config". This is for two
     reasons:

       a. backwards compatibility with scripts looking at
          config-like files.

       b. inspection of a specific file probably means you
	  care about just what's in that file, not a general
          lookup for "do we have this value anywhere at
	  all". If that is not the case, the caller can
	  always specify "--includes".

  3. Writing files via "git config"; we want to treat
     include.* variables as literal items to be copied (or
     modified), and not expand them. So "git config
     --unset-all foo.bar" would operate _only_ on
     .git/config, not any of its included files (just as it
     also does not operate on ~/.gitconfig).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt               |   15 ++++
 Documentation/git-config.txt           |    5 +
 Documentation/technical/api-config.txt |   22 ++++++
 builtin/config.c                       |   31 ++++++--
 cache.h                                |    8 ++
 config.c                               |   69 +++++++++++++++++-
 t/t1305-config-include.sh              |  126 ++++++++++++++++++++++++++++++++
 7 files changed, 268 insertions(+), 8 deletions(-)
 create mode 100755 t/t1305-config-include.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..e55dae1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -84,6 +84,17 @@ customary UNIX fashion.
 
 Some variables may require a special value format.
 
+Includes
+~~~~~~~~
+
+You can include one config file from another by setting the special
+`include.path` variable to the name of the file to be included. The
+included file is expanded immediately, as if its contents had been
+found at the location of the include directive. If the value of the
+`include.path` variable is a relative path, the path is considered to be
+relative to the configuration file in which the include directive was
+found. See below for examples.
+
 Example
 ~~~~~~~
 
@@ -106,6 +117,10 @@ Example
 		gitProxy="ssh" for "kernel.org"
 		gitProxy=default-proxy ; for the rest
 
+	[include]
+		path = /path/to/foo.inc ; include by absolute path
+		path = foo ; expand "foo" relative to the current file
+
 Variables
 ~~~~~~~~~
 
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..aa8303b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -178,6 +178,11 @@ See also <<FILES>>.
 	Opens an editor to modify the specified config file; either
 	'--system', '--global', or repository (default).
 
+--includes::
+--no-includes::
+	Respect `include.*` directives in config files when looking up
+	values. Defaults to on.
+
 [[FILES]]
 FILES
 -----
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index f428c5c..c60b6b3 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -95,6 +95,28 @@ string is given, prints an error message and returns -1.
 Similar to `git_config_string`, but expands `~` or `~user` into the
 user's home directory when found at the beginning of the path.
 
+Include Directives
+------------------
+
+By default, the config parser does not respect include directives.
+However, a caller can use the special `git_config_include` wrapper
+callback to support them. To do so, you simply wrap your "real" callback
+function and data pointer in a `struct config_include_data`, and pass
+the wrapper to the regular config-reading functions. For example:
+
+-------------------------------------------
+int read_file_with_include(const char *file, config_fn_t fn, void *data)
+{
+	struct config_include_data inc = CONFIG_INCLUDE_INIT;
+	inc.fn = fn;
+	inc.data = data;
+	return git_config_from_file(git_config_include, file, &inc);
+}
+-------------------------------------------
+
+`git_config` respects includes automatically. The lower-level
+`git_config_from_file` does not.
+
 Writing Config Files
 --------------------
 
diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..09bf778 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,6 +25,7 @@ static const char *given_config_file;
 static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
+static int respect_includes = -1;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -74,6 +75,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH),
 	OPT_GROUP("Other"),
 	OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+	OPT_BOOL(0, "includes", &respect_includes, "respect include directives on lookup"),
 	OPT_END(),
 };
 
@@ -161,6 +163,9 @@ static int get_value(const char *key_, const char *regex_)
 	int ret = -1;
 	char *global = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
+	struct config_include_data inc = CONFIG_INCLUDE_INIT;
+	config_fn_t fn;
+	void *data;
 
 	local = config_exclusive_filename;
 	if (!local) {
@@ -213,19 +218,28 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
+	fn = show_config;
+	data = NULL;
+	if (respect_includes) {
+		inc.fn = fn;
+		inc.data = data;
+		fn = git_config_include;
+		data = &inc;
+	}
+
 	if (do_all && system_wide)
-		git_config_from_file(show_config, system_wide, NULL);
+		git_config_from_file(fn, system_wide, data);
 	if (do_all && global)
-		git_config_from_file(show_config, global, NULL);
+		git_config_from_file(fn, global, data);
 	if (do_all)
-		git_config_from_file(show_config, local, NULL);
-	git_config_from_parameters(show_config, NULL);
+		git_config_from_file(fn, local, data);
+	git_config_from_parameters(fn, data);
 	if (!do_all && !seen)
-		git_config_from_file(show_config, local, NULL);
+		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
-		git_config_from_file(show_config, global, NULL);
+		git_config_from_file(fn, global, data);
 	if (!do_all && !seen && system_wide)
-		git_config_from_file(show_config, system_wide, NULL);
+		git_config_from_file(fn, system_wide, data);
 
 	free(key);
 	if (regexp) {
@@ -384,6 +398,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			config_exclusive_filename = given_config_file;
 	}
 
+	if (respect_includes == -1)
+		respect_includes = !config_exclusive_filename;
+
 	if (end_null) {
 		term = '\0';
 		delim = '\n';
diff --git a/cache.h b/cache.h
index 9bd8c2d..7cf6dc1 100644
--- a/cache.h
+++ b/cache.h
@@ -1139,6 +1139,14 @@ extern const char *get_commit_output_encoding(void);
 
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
 
+struct config_include_data {
+	int depth;
+	config_fn_t fn;
+	void *data;
+};
+#define CONFIG_INCLUDE_INIT { 0 }
+extern int git_config_include(const char *name, const char *value, void *data);
+
 extern const char *config_exclusive_filename;
 
 #define MAX_GITNAME (1000)
diff --git a/config.c b/config.c
index 40f9c6d..e3fcf75 100644
--- a/config.c
+++ b/config.c
@@ -28,6 +28,69 @@ static int zlib_compression_seen;
 
 const char *config_exclusive_filename = NULL;
 
+#define MAX_INCLUDE_DEPTH 10
+static const char include_depth_advice[] =
+"exceeded maximum include depth (%d) while including\n"
+"	%s\n"
+"from\n"
+"	%s\n"
+"Do you have circular includes?";
+static int handle_path_include(const char *path, struct config_include_data *inc)
+{
+	int ret = 0;
+	struct strbuf buf = STRBUF_INIT;
+
+	/*
+	 * Use an absolute path as-is, but interpret relative paths
+	 * based on the including config file.
+	 */
+	if (!is_absolute_path(path)) {
+		char *slash;
+
+		if (!cf || !cf->name)
+			return error("relative config includes must come from files");
+
+		slash = find_last_dir_sep(cf->name);
+		if (slash)
+			strbuf_add(&buf, cf->name, slash - cf->name + 1);
+		strbuf_addstr(&buf, path);
+		path = buf.buf;
+	}
+
+	if (!access(path, R_OK)) {
+		if (++inc->depth > MAX_INCLUDE_DEPTH)
+			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
+			    cf && cf->name ? cf->name : "the command line");
+		ret = git_config_from_file(git_config_include, path, inc);
+		inc->depth--;
+	}
+	strbuf_release(&buf);
+	return ret;
+}
+
+int git_config_include(const char *var, const char *value, void *data)
+{
+	struct config_include_data *inc = data;
+	const char *type;
+	int ret;
+
+	/*
+	 * Pass along all values, including "include" directives; this makes it
+	 * possible to query information on the includes themselves.
+	 */
+	ret = inc->fn(var, value, inc->data);
+	if (ret < 0)
+		return ret;
+
+	type = skip_prefix(var, "include.");
+	if (!type)
+		return ret;
+
+	if (!strcmp(type, "path"))
+		ret = handle_path_include(value, inc);
+	return ret;
+}
+
 static void lowercase(char *p)
 {
 	for (; *p; p++)
@@ -921,9 +984,13 @@ int git_config(config_fn_t fn, void *data)
 {
 	char *repo_config = NULL;
 	int ret;
+	struct config_include_data inc = CONFIG_INCLUDE_INIT;
+
+	inc.fn = fn;
+	inc.data = data;
 
 	repo_config = git_pathdup("config");
-	ret = git_config_early(fn, data, repo_config);
+	ret = git_config_early(git_config_include, &inc, repo_config);
 	if (repo_config)
 		free(repo_config);
 	return ret;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
new file mode 100755
index 0000000..0a27ec4
--- /dev/null
+++ b/t/t1305-config-include.sh
@@ -0,0 +1,126 @@
+#!/bin/sh
+
+test_description='test config file include directives'
+. ./test-lib.sh
+
+test_expect_success 'include file by absolute path' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = \"$PWD/one\"" >.gitconfig &&
+	echo 1 >expect &&
+	git config test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'include file by relative path' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	echo 1 >expect &&
+	git config test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'chained relative paths' '
+	mkdir subdir &&
+	echo "[test]three = 3" >subdir/three &&
+	echo "[include]path = three" >subdir/two &&
+	echo "[include]path = subdir/two" >.gitconfig &&
+	echo 3 >expect &&
+	git config test.three >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'include options can still be examined' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	echo one >expect &&
+	git config include.path >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'listing includes option and expansion' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	cat >expect <<-\EOF &&
+	include.path=one
+	test.one=1
+	EOF
+	git config --list >actual.full &&
+	grep -v ^core actual.full >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'single file lookup does not expand includes by default' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	test_must_fail git config -f .gitconfig test.one &&
+	test_must_fail git config --global test.one &&
+	echo 1 >expect &&
+	git config --includes -f .gitconfig test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'writing config file does not expand includes' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	git config test.two 2 &&
+	echo 2 >expect &&
+	git config --no-includes test.two >actual &&
+	test_cmp expect actual &&
+	test_must_fail git config --no-includes test.one
+'
+
+test_expect_success 'config modification does not affect includes' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	git config test.one 2 &&
+	echo 1 >expect &&
+	git config -f one test.one >actual &&
+	test_cmp expect actual &&
+	cat >expect <<-\EOF &&
+	1
+	2
+	EOF
+	git config --get-all test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'missing include files are ignored' '
+	cat >.gitconfig <<-\EOF &&
+	[include]path = foo
+	[test]value = yes
+	EOF
+	echo yes >expect &&
+	git config test.value >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'absolute includes from command line work' '
+	echo "[test]one = 1" >one &&
+	echo 1 >expect &&
+	git -c include.path="$PWD/one" config test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'relative includes from command line fail' '
+	echo "[test]one = 1" >one &&
+	test_must_fail git -c include.path=one config test.one
+'
+
+test_expect_success 'include cycles are detected' '
+	cat >.gitconfig <<-\EOF &&
+	[test]value = gitconfig
+	[include]path = cycle
+	EOF
+	cat >cycle <<-\EOF &&
+	[test]value = cycle
+	[include]path = .gitconfig
+	EOF
+	cat >expect <<-\EOF &&
+	gitconfig
+	cycle
+	EOF
+	test_must_fail git config --get-all test.value 2>stderr &&
+	grep "exceeded maximum include depth" stderr
+'
+
+test_done
-- 
1.7.9.rc1.29.g43677

Re: [PATCH 1/2] docs: add a basic description of the config API

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This wasn't documented at all; this is pretty bare-bones,
> but it should at least give new git hackers a basic idea of
> how the reading side works.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/technical/api-config.txt |  101 ++++++++++++++++++++++++++++++++
>  1 files changed, 101 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/technical/api-config.txt
>
> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
> new file mode 100644
> index 0000000..f428c5c
> --- /dev/null
> +++ b/Documentation/technical/api-config.txt
> @@ -0,0 +1,101 @@
> +config API
> +==========
> +
> +The config API gives callers a way to access git configuration files
> +(and files which have the same syntax). See linkgit:git-config[1] for a
> +discussion of the config file syntax.
> +
> +General Usage
> +-------------
> +
> +Config files are parsed linearly, and each variable found is passed to a
> +caller-provided callback function. The callback function is responsible
> +for any actions to be taken on the config option, and is free to ignore
> +some options (it is not uncommon for the configuration to be parsed
> +several times during the run of a git program, with different callbacks
> +picking out different variables useful to themselves).

It woud be easeier to read if you stopped the sentence after "some
options" and made the "It is not uncommon..." a first-class sentence
outside the parentheses.

> +A config callback function takes three parameters:
> +
> +- the name of the parsed variable. This is in canonical "flat" form: the
> +  section, subsection, and variable segments will be separated by dots,
> +  and the section and variable segments will be all lowercase. E.g.,
> +  `core.ignorecase`, `diff.SomeType.textconv`.
> +
> +- the value of the found variable, as a string. If the variable had no
> +  value specified, the value will be NULL (typically this means it
> +  should be interpreted as boolean true).
> +
> +- a void pointer passed in by the caller of the config API; this can
> +  contain callback-specific data
> +
> +A config callback should return 0 for success, or -1 if the variable
> +could not be parsed properly.

This matches what I have always thought, but I think I recently saw a
series that adds callbacks that return 1 to mean "I have understood this
variable, so callers should not look at it any more".  It felt wrong, but
I did not find anything in the config.c API framework to prvent such a
local calling convention.

> +Basic Config Querying
> +---------------------
> +
> +Most programs will simply want to look up variables in all config files
> +that git knows about, using the normal precedence rules. To do this,
> +call `git_config` with a callback function and void data pointer.
> +
> +`git_config` will read all config sources in order of increasing
> +priority. Thus a callback should typically overwrite previously-seen
> +entries with new ones (e.g., if both the user-wide `~/.gitconfig` and
> +repo-specific `.git/config` contain `color.ui`, the config machinery
> +will first feed the user-wide one to the callback, and then the
> +repo-specific one; by overwriting, the higher-priority repo-specific
> +value is left at the end).
> +
> +There is a special version of `git_config` called `git_config_early`
> +that takes an additional parameter to specify the repository config.
> +This should be used early in a git program when the repository location
> +has not yet been determined (and calling the usual lazy-evaluation
> +lookup rules would yield an incorrect location).

Do you want to say somethink like "Ordinary programs should not have to
worry about git_config_early()"?  Differently put, if you are learning the
config API by reading this document and cannot tell which one you should
be calling, you are way too inexperienced to call git_config_early() and
you would always want to call git_config()?

Re: [PATCH 2/2] config: add include directive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +Includes
> +~~~~~~~~
> +
> +You can include one config file from another by setting the special
> +`include.path` variable to the name of the file to be included. The
> +included file is expanded immediately, as if its contents had been
> +found at the location of the include directive. If the value of the
> +`include.path` variable is a relative path, the path is considered to be
> +relative to the configuration file in which the include directive was
> +found. See below for examples.

If the file referenced by this directive does not exist, what should
happen?  Should it be signalled as an error?  Should it stop the whole
calling process with die()?

I think "die() when we are honoring the include, ignore when we are not"
would be a good way to handle this, as it allows us to catch mistakes
while allowing the user to fix broken configuration files using "git
config --unset include.path", but I may be overlooking something.

Re: [PATCH 1/2] docs: add a basic description of the config API

[Jeff King]

On Mon, Feb 06, 2012 at 02:31:14PM -0800, Junio C Hamano wrote:

> > +Config files are parsed linearly, and each variable found is passed to a
> > +caller-provided callback function. The callback function is responsible
> > +for any actions to be taken on the config option, and is free to ignore
> > +some options (it is not uncommon for the configuration to be parsed
> > +several times during the run of a git program, with different callbacks
> > +picking out different variables useful to themselves).
> 
> It woud be easeier to read if you stopped the sentence after "some
> options" and made the "It is not uncommon..." a first-class sentence
> outside the parentheses.

Thanks. Putting in too many parenthetical phrases is a bad habit of
mine.  Usually I notice around the time I try nesting a parenthetical
phrase instead of existing parentheses, and go back and at least promote
the outer one to a real sentence. :)

Will fix for the re-roll.

> > +A config callback should return 0 for success, or -1 if the variable
> > +could not be parsed properly.
> 
> This matches what I have always thought, but I think I recently saw a
> series that adds callbacks that return 1 to mean "I have understood this
> variable, so callers should not look at it any more".  It felt wrong, but
> I did not find anything in the config.c API framework to prvent such a
> local calling convention.

The return value is propagated from git_parse_file. I assume the
original intent was that you could actually use it to return an integer
from your callback. In practice, though, callbacks either modify global
data (for older instances), or modify a pointer passed through the void
data pointer.

The "1 means I understood this" convention is used by userdiff_config. I
don't like that it is unlike every other config callback, but I think
it's necessary to deal with the ambiguity of "diff.color.*", which could
be either a userdiff entry or a diff color. E.g., if we see
"diff.color.binary", we know that it is the "binary" variable of the
"color" diff driver, not the color spec for the "binary" slot.

Looking at the code again, though, it seems that there is no overlap
between the userdiff slots and the color slots, and that the
color-parsing code will silently ignore any unknown slots. So it would
be safe to further investigate diff.color.binary as a color, as we would
silently ignore it.

Hmm. Yeah. The userdiff calling convention dates back to late 2008. At
that time, parse_diff_color_slot would die() if it did not understand
the slot, making the "I understood this" flag required. Then later, in
8b8e862 (ignore unknown color configuration, 2009-12-12), it was
relaxed.

So I think we could go back and simplify the userdiff_config code now.

> > +There is a special version of `git_config` called `git_config_early`
> > +that takes an additional parameter to specify the repository config.
> > +This should be used early in a git program when the repository location
> > +has not yet been determined (and calling the usual lazy-evaluation
> > +lookup rules would yield an incorrect location).
> 
> Do you want to say somethink like "Ordinary programs should not have to
> worry about git_config_early()"?  Differently put, if you are learning the
> config API by reading this document and cannot tell which one you should
> be calling, you are way too inexperienced to call git_config_early() and
> you would always want to call git_config()?

Yes, I think that would be sensible.

Though having looked a lot at the config code recently, I wonder if
git_config_early is really necessary. The only caller (besides
git_config) is check_repository_format_early, which just wants to see if
core.repositoryformatversion in a directory is sane.

But why in the world would it want to do the full config lookup?
Shouldn't it be checking git_config_from_file directly on the config
file in the proposed repo?

-Peff

Re: [PATCH 1/2] docs: add a basic description of the config API

[Jeff King]

On Tue, Feb 07, 2012 at 01:06:25PM -0500, Jeff King wrote:

> So I think we could go back and simplify the userdiff_config code now.

That patch would look like this.

-- >8 --
Subject: drop odd return value semantics from userdiff_config

When the userdiff_config function was introduced in be58e70
(diff: unify external diff and funcname parsing code,
2008-10-05), it used a return value convention unlike any
other config callback. Like other callbacks, it used "-1" to
signal error. But it returned "1" to indicate that it found
something, and "0" otherwise; other callbacks simply
returned "0" to indicate that no error occurred.

This distinction was necessary at the time, because the
userdiff namespace overlapped slightly with the color
configuration namespace. So "diff.color.foo" could mean "the
'foo' slot of diff coloring" or "the 'foo' component of the
"color" userdiff driver". Because the color-parsing code
would die on an unknown color slot, we needed the userdiff
code to indicate that it had matched the variable, letting
us bypass the color-parsing code entirely.

Later, in 8b8e862 (ignore unknown color configuration,
2009-12-12), the color-parsing code learned to silently
ignore unknown slots. This means we no longer need to
protect userdiff-matched variables from reaching the
color-parsing code.

We can therefore change the userdiff_config calling
convention to a more normal one. This drops some code from
each caller, which is nice. But more importantly, it reduces
the cognitive load for readers who may wonder why
userdiff_config is unlike every other config callback.

There's no need to add a new test confirming that this
works; t4020 already contains a test that sets
diff.color.external.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c    |    8 +-------
 builtin/cat-file.c |    8 +-------
 builtin/grep.c     |    7 ++-----
 diff.c             |    7 ++-----
 userdiff.c         |   19 ++++++-------------
 5 files changed, 12 insertions(+), 37 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..01956c8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	switch (userdiff_config(var, value)) {
-	case 0:
-		break;
-	case -1:
+	if (userdiff_config(var, value) < 0)
 		return -1;
-	default:
-		return 0;
-	}
 
 	return git_default_config(var, value, cb);
 }
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 07bd984..8ed501f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -226,14 +226,8 @@ static const char * const cat_file_usage[] = {
 
 static int git_cat_file_config(const char *var, const char *value, void *cb)
 {
-	switch (userdiff_config(var, value)) {
-	case 0:
-		break;
-	case -1:
+	if (userdiff_config(var, value) < 0)
 		return -1;
-	default:
-		return 0;
-	}
 
 	return git_default_config(var, value, cb);
 }
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c2ae94..dc6de83 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -325,11 +325,8 @@ static int grep_config(const char *var, const char *value, void *cb)
 	struct grep_opt *opt = cb;
 	char *color = NULL;
 
-	switch (userdiff_config(var, value)) {
-	case 0: break;
-	case -1: return -1;
-	default: return 0;
-	}
+	if (userdiff_config(var, value) < 0)
+		return -1;
 
 	if (!strcmp(var, "grep.extendedregexp")) {
 		if (git_config_bool(var, value))
diff --git a/diff.c b/diff.c
index 7e15426..a656f5e 100644
--- a/diff.c
+++ b/diff.c
@@ -177,11 +177,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	switch (userdiff_config(var, value)) {
-		case 0: break;
-		case -1: return -1;
-		default: return 0;
-	}
+	if (userdiff_config(var, value) < 0)
+		return -1;
 
 	if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
 		int slot = parse_diff_color_slot(var, 11);
diff --git a/userdiff.c b/userdiff.c
index 76109da..1e7184f 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -210,14 +210,7 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,
 	if (git_config_string(&f->pattern, k, v) < 0)
 		return -1;
 	f->cflags = cflags;
-	return 1;
-}
-
-static int parse_string(const char **d, const char *k, const char *v)
-{
-	if (git_config_string(d, k, v) < 0)
-		return -1;
-	return 1;
+	return 0;
 }
 
 static int parse_tristate(int *b, const char *k, const char *v)
@@ -226,13 +219,13 @@ static int parse_tristate(int *b, const char *k, const char *v)
 		*b = -1;
 	else
 		*b = git_config_bool(k, v);
-	return 1;
+	return 0;
 }
 
 static int parse_bool(int *b, const char *k, const char *v)
 {
 	*b = git_config_bool(k, v);
-	return 1;
+	return 0;
 }
 
 int userdiff_config(const char *k, const char *v)
@@ -246,13 +239,13 @@ int userdiff_config(const char *k, const char *v)
 	if ((drv = parse_driver(k, v, "binary")))
 		return parse_tristate(&drv->binary, k, v);
 	if ((drv = parse_driver(k, v, "command")))
-		return parse_string(&drv->external, k, v);
+		return git_config_string(&drv->external, k, v);
 	if ((drv = parse_driver(k, v, "textconv")))
-		return parse_string(&drv->textconv, k, v);
+		return git_config_string(&drv->textconv, k, v);
 	if ((drv = parse_driver(k, v, "cachetextconv")))
 		return parse_bool(&drv->textconv_want_cache, k, v);
 	if ((drv = parse_driver(k, v, "wordregex")))
-		return parse_string(&drv->word_regex, k, v);
+		return git_config_string(&drv->word_regex, k, v);
 
 	return 0;
 }
-- 
1.7.8.4.12.g3a22e3

Re: [PATCH 2/2] config: add include directive

[Jeff King]

On Mon, Feb 06, 2012 at 02:39:41PM -0800, Junio C Hamano wrote:

> > +Includes
> > +~~~~~~~~
> > +
> > +You can include one config file from another by setting the special
> > +`include.path` variable to the name of the file to be included. The
> > +included file is expanded immediately, as if its contents had been
> > +found at the location of the include directive. If the value of the
> > +`include.path` variable is a relative path, the path is considered to be
> > +relative to the configuration file in which the include directive was
> > +found. See below for examples.
> 
> If the file referenced by this directive does not exist, what should
> happen?  Should it be signalled as an error?  Should it stop the whole
> calling process with die()?

I silently ignore it. My thinking was that you might want to have
something like:

  [include]
          path = .gitconfig-local

in a stock .gitconfig that you ship to all of your machines[1]. Then
machines that need it can put things in .gitconfig-local, and those that
don't can just ignore it.

It is a tradeoff, of course, in that typos will be silently ignored. For
this use case, you could also just create an empty .gitconfig-local on
machines that don't have anything to put there.

[1] Actually, a similar use might be a ~/.gitconfig that is shared by a
    mounted home directory (e.g., via NFS) NFS, and a ~/.gitconfig-$HOST
    that is specific to each machine. The current code doesn't expand
    environment variables (nor tildes), but perhaps it should.

> I think "die() when we are honoring the include, ignore when we are not"
> would be a good way to handle this, as it allows us to catch mistakes
> while allowing the user to fix broken configuration files using "git
> config --unset include.path", but I may be overlooking something.

The writing path does not use the include callback wrapper at all; so
include.path can be manipulated just as any other variable, and the
value is not treated specially.

-Peff

Re: [PATCH 1/2] docs: add a basic description of the config API

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > +A config callback should return 0 for success, or -1 if the variable
>> > +could not be parsed properly.
>> 
>> This matches what I have always thought, but I think I recently saw a
>> series that adds callbacks that return 1 to mean "I have understood this
>> variable, so callers should not look at it any more".  It felt wrong, but
>> I did not find anything in the config.c API framework to prvent such a
>> local calling convention.
>
> ...
> The "1 means I understood this" convention is used by userdiff_config. I
> don't like that it is unlike every other config callback,...
> Looking at the code again, though, ...
> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
> So I think we could go back and simplify the userdiff_config code now.

I remembered where I saw the new "offender"; it was nd/columns
topic (Cc'ing Nguyễn).

Re: [PATCH 1/2] docs: add a basic description of the config API

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> -- >8 --
> Subject: drop odd return value semantics from userdiff_config
> ...
>  5 files changed, 12 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..01956c8 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
> -	switch (userdiff_config(var, value)) {
> -	case 0:
> -		break;
> -	case -1:
> +	if (userdiff_config(var, value) < 0)
>  		return -1;
> -	default:
> -		return 0;
> -	}
>  
>  	return git_default_config(var, value, cb);
>  }
> ...

Looks very nice ;-)

Re: [PATCH 1/2] docs: add a basic description of the config API

[Jeff King]

On Tue, Feb 07, 2012 at 01:06:25PM -0500, Jeff King wrote:

> Though having looked a lot at the config code recently, I wonder if
> git_config_early is really necessary. The only caller (besides
> git_config) is check_repository_format_early, which just wants to see if
> core.repositoryformatversion in a directory is sane.
> 
> But why in the world would it want to do the full config lookup?
> Shouldn't it be checking git_config_from_file directly on the config
> file in the proposed repo?

I prepared the patch below to fix this, but it fails t0001. It turns out
that check_repository_format_gently checks not only stuff that should be
in .git/config, but also checks and remembers core.sharedrepository in
the process, which can come from anywhere.

So I think the "most correct" thing to do would be:

  - check_repository_format_gently reads _only_ from .git/config

  - core.sharedrepository should come from git_config_default

But that just creates more headaches. Callers like init_db would need to
call git_config separately. But of course they don't have the repo set
up properly, so they would want git_config_early. So we don't end up
getting to remove git_config_early, anyway.

And though it is slightly insane that you can do:

  git config --global core.repositoryformatversion 0

or even:

  git -c core.repositoryformatversion=0 ...

and it is respected, in practice nobody does this. So it's probably
better to just leave the code as-is.

-Peff

-- >8 --
Subject: don't look for repositoryformatversion outside of repo

In the check_repository_format_gently function, we use
git_config_early to parse the repository format version from
the .git/config file.

However, git_config_early looks in all of the sources of git
config, including /etc/gitconfig, ~/.gitconfig, and the
command line. But we should really only be interested in getting
the value from the repository's config file, since the point
of this function is to check the repository itself.

Therefore we can just feed our repo-config argument directly
to git_config_from_file. As a bonus, this means the subtle
distinction in using git_config_early versus git_config can
just go away.

Signed-off-by: Jeff King <peff@peff.net>
---
 setup.c |   11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/setup.c b/setup.c
index 61c22e6..20509cf 100644
--- a/setup.c
+++ b/setup.c
@@ -319,17 +319,8 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 {
 	char repo_config[PATH_MAX+1];
 
-	/*
-	 * git_config() can't be used here because it calls git_pathdup()
-	 * to get $GIT_CONFIG/config. That call will make setup_git_env()
-	 * set git_dir to ".git".
-	 *
-	 * We are in gitdir setup, no git dir has been found useable yet.
-	 * Use a gentler version of git_config() to check if this repo
-	 * is a good one.
-	 */
 	snprintf(repo_config, PATH_MAX, "%s/config", gitdir);
-	git_config_early(check_repository_format_version, NULL, repo_config);
+	git_config_from_file(check_repository_format_version, repo_config, NULL);
 	if (GIT_REPO_VERSION < repository_format_version) {
 		if (!nongit_ok)
 			die ("Expected git repo version <= %d, found %d",
-- 
1.7.8.4.12.g3a22e3

Re: [PATCH 1/2] docs: add a basic description of the config API

[Nguyen Thai Ngoc Duy]

2012/2/8 Junio C Hamano <gitster@pobox.com>:
> Jeff King <peff@peff.net> writes:
>
>>> > +A config callback should return 0 for success, or -1 if the variable
>>> > +could not be parsed properly.
>>>
>>> This matches what I have always thought, but I think I recently saw a
>>> series that adds callbacks that return 1 to mean "I have understood this
>>> variable, so callers should not look at it any more".  It felt wrong, but
>>> I did not find anything in the config.c API framework to prvent such a
>>> local calling convention.
>>
>> ...
>> The "1 means I understood this" convention is used by userdiff_config. I
>> don't like that it is unlike every other config callback,...
>> Looking at the code again, though, ...
>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
>> So I think we could go back and simplify the userdiff_config code now.
>
> I remembered where I saw the new "offender"; it was nd/columns
> topic (Cc'ing Nguyễn).

nd/columns does use "1" convention in git_column_config(), but the
direct config callback function does not return 1 to config machinery.
All call sites follow this pattern:

int ret = git_column_config(key, var, "command", &colopts);
if (ret <= 0) return ret;

I think it's ok.
-- 
Duy

Re: [PATCH 1/2] docs: add a basic description of the config API

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>>> The "1 means I understood this" convention is used by userdiff_config. I
>>> don't like that it is unlike every other config callback,...
>>> Looking at the code again, though, ...
>>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
>>> So I think we could go back and simplify the userdiff_config code now.
>>
>> I remembered where I saw the new "offender"; it was nd/columns
>> topic (Cc'ing Nguyễn).
>
> nd/columns does use "1" convention in git_column_config(), but the
> direct config callback function does not return 1 to config machinery.
> All call sites follow this pattern:
>
> int ret = git_column_config(key, var, "command", &colopts);
> if (ret <= 0) return ret;
>
> I think it's ok.

I too think this should be acceptable, but that is not the point.

Your excuse that "the toplevel callback in my callchain never returns 1,
so overall the nd/columns series is ok" just muddies the water.  It means
if later somebody wanted to use inner callback functions you use from the
git_column_config() callchain chain as a toplevel callback for whatever
reason, that will violate the "0 for success, or -1 for error" convention.
More importantly, if somebody wants to turn a top-level callback that
currently returns 0 into a sub callback used by his callback callchain, he
cannot change that existing callback to return 1 to tell him to short
circuit, because for other callers returning 1 would be a violation.

What I was getting at is that we probably should officially declare that
returning 1 to signal success is perfectly acceptable (and it probably
should mean the caller who called the callback function as a sub callback
should return immediately, taking it as a signal that the key has already
been handled), as the primary purpose of this thread to discuss Peff's
patch is to write these rules down.

Of course, all that relies on the audit of the git_config() machinery. I
think it is written to accept non-negative as success, and that is why I
said "I too think this should be acceptable" in the first place.

Re: [PATCH 1/2] docs: add a basic description of the config API

[Nguyen Thai Ngoc Duy]

On Wed, Feb 8, 2012 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Your excuse that "the toplevel callback in my callchain never returns 1,
> so overall the nd/columns series is ok" just muddies the water.  It means
> if later somebody wanted to use inner callback functions you use from the
> git_column_config() callchain chain as a toplevel callback for whatever
> reason, that will violate the "0 for success, or -1 for error" convention.
> More importantly, if somebody wants to turn a top-level callback that
> currently returns 0 into a sub callback used by his callback callchain, he
> cannot change that existing callback to return 1 to tell him to short
> circuit, because for other callers returning 1 would be a violation.
>
> What I was getting at is that we probably should officially declare that
> returning 1 to signal success is perfectly acceptable (and it probably
> should mean the caller who called the callback function as a sub callback
> should return immediately, taking it as a signal that the key has already
> been handled), as the primary purpose of this thread to discuss Peff's
> patch is to write these rules down.

Or allow multiple callbacks from git_config(). The problem with is it
adds another common set of config vars. Now it's common to chain var
sets by doing

int config_callback(...)
{
    ....
    return yet_another_callback(...);
}

int main()
{
   git_config(config_callback, ...)
   ...

where yet_another_callback() hard codes another callback (usually
git_default_config). This is inflexible. If we allow multiple
callbacks, git_column_config() could be changed to return just 0 or -1
(i.e. 1 remains unsuccessful error code).

On second thought, I think calling git_config() multiple times, each
time with one callback, may work too. We may want to cache config
content to avoid accessing files too many times though.

> Of course, all that relies on the audit of the git_config() machinery. I
> think it is written to accept non-negative as success, and that is why I
> said "I too think this should be acceptable" in the first place.
-- 
Duy

Re: [PATCH 1/2] docs: add a basic description of the config API

[Jeff King]

On Tue, Feb 07, 2012 at 10:40:14PM -0800, Junio C Hamano wrote:

> What I was getting at is that we probably should officially declare that
> returning 1 to signal success is perfectly acceptable (and it probably
> should mean the caller who called the callback function as a sub callback
> should return immediately, taking it as a signal that the key has already
> been handled), as the primary purpose of this thread to discuss Peff's
> patch is to write these rules down.
> 
> Of course, all that relies on the audit of the git_config() machinery. I
> think it is written to accept non-negative as success, and that is why I
> said "I too think this should be acceptable" in the first place.

I looked through it the other day when this came up, and I believe that
is the case. There is another related rule that should be considered,
though: should the return value from callbacks be propagated via
git_config to its caller?

It is the case already for config files that are read, but not for
config options parsed from the command line. It would not be too hard to
change, but I do not think any current callers care (as I mentioned
earlier, these days with the "void *" data pointer, sane callers will
simply pass in a pointer to a modifiable data area).

-Peff

Re: [PATCH 1/2] docs: add a basic description of the config API

[Jeff King]

On Wed, Feb 08, 2012 at 01:55:30PM +0700, Nguyen Thai Ngoc Duy wrote:

> Or allow multiple callbacks from git_config(). The problem with is it
> adds another common set of config vars. Now it's common to chain var
> sets by doing
> 
> int config_callback(...)
> {
>     ....
>     return yet_another_callback(...);
> }
> 
> int main()
> {
>    git_config(config_callback, ...)
>    ...
> 
> where yet_another_callback() hard codes another callback (usually
> git_default_config). This is inflexible. If we allow multiple
> callbacks, git_column_config() could be changed to return just 0 or -1
> (i.e. 1 remains unsuccessful error code).

I don't think we need multiple callbacks. You could convert any such
call of:

  git_config_multiple(cb1, cb2, cb3, NULL, NULL);

into:

  int combining_callback(const char *var, const char *value, void *data)
  {
          if (cb1(var, value, data) < 0)
                  return -1;
          if (cb2(var, value, data) < 0)
                  return -1;
          if (cb3(var, value, data) < 0)
                  return -1;
          return 0;
  }

But note that you get the same "data" pointer in each case. If you
wanted different data pointers, you would need more support from the
config machinery. However, I'd rather that be spelled:

  git_config(cb1, data1);
  git_config(cb2, data2);
  git_config(cb3, data3);

and if the efficiency isn't acceptable, then looking into caching the
key/value pairs.

> On second thought, I think calling git_config() multiple times, each
> time with one callback, may work too. We may want to cache config
> content to avoid accessing files too many times though.

Exactly. I would do that first, and then worry about efficiency later if
it comes up. The first time I saw that we cached config multiple times
in a program run (several years ago), I had the same thought. But I
don't think the performance impact for reading the config 2 or 3 times
instead of 1 was even measurable, so I stopped looking into it.

If were to adopt something like the "automatically run this program to
get the config value" proposal that has been floating around (and I am
not saying we should), _then_ I think it might make sense to cache the
values, because the sub-program might be expensive to run.

-Peff