Re: [PATCH 1/4] config: add include directive

[Michael Haggerty <mhagger@alum.mit.edu>]

On 01/26/2012 08:37 AM, Jeff King wrote:
> [...]
> This patch introduces an include directive for config files.
> It looks like:
> 
>   [include]
>     path = /path/to/file

I like it.

> diff --git a/cache.h b/cache.h
> index 10afd71..21bbb0a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1138,6 +1138,12 @@ extern const char *get_commit_output_encoding(void);
>  
>  extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
>  
> +struct git_config_include_data {
> +	config_fn_t fn;
> +	void *data;
> +};
> +int git_config_include(const char *name, const char *value, void *vdata);
> +
>  extern const char *config_exclusive_filename;
>  
>  #define MAX_GITNAME (1000)

How about a short comment or two?

> diff --git a/config.c b/config.c
> index 40f9c6d..a6966c1 100644
> --- a/config.c
> +++ b/config.c
> [...]
> +int git_config_include(const char *name, const char *value, void *vdata)
> +{
> +	const struct git_config_include_data *data = vdata;
> +	const char *type;
> +	int ret;
> +
> +	/*
> +	 * Pass along all values, including "include" directives; this makes it
> +	 * possible to query information on the includes themselves.
> +	 */
> +	ret = data->fn(name, value, data->data);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (prefixcmp(name, "include."))
> +		return ret;
> +	type = strrchr(name, '.') + 1;
> +
> +	if (!strcmp(type, "path"))
> +		ret = handle_path_include(value, vdata);
> +
> +	return ret;
> +}
> +

Doesn't this code accept all keys of the form "include\.(.*\.)?path"
(e.g., "include.foo.path")?  If that is your intention, then the
documentation should be fixed.  If not, then a single strcmp(name,
"include.path") would seem sufficient.

>  int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  {
>  	int ret = 0, found = 0;
>  	const char *home = NULL;
> +	struct git_config_include_data inc;
> +
> +	inc.fn = fn;
> +	inc.data = data;
> +	fn = git_config_include;
> +	data = &inc;
>  
>  	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
>  	if (config_exclusive_filename)

The comment just after your addition should be adjusted, since now "the
given config file and any files that it includes" are read.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/