Re: [PATCH 1/4] config: add include directive
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Jan 27, 2012, 05:07 UTC
- Message-ID
- <4F223115.3050004@alum.mit.edu>
- In-Reply-To
- <20120126073752.GA30474@sigill.intra.peff.net>
On 01/26/2012 08:37 AM, Jeff King wrote:
Show 6 quoted lines
> [...] > This patch introduces an include directive for config files. > It looks like: > > [include] > path = /path/to/file
I like it.
Show 17 quoted lines
> 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?
Show 29 quoted lines
> 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.
Show 13 quoted lines
> 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/