Re: [PATCH 1/2] Add support for a shared config file
Some minor nits (style) below.
On Thu, Nov 25, 2010 at 3:21 PM, Nathan W. Panike <nathan.panike@gmail.com> wrote:
Show 31 quoted lines
> The idea is that a project develops indigenous aliases that
> should be shared project-wide. The only way to communicate this
> now is by channels outside of git--email or IRC or such. We add
> support for the case where a project configuration can be in
> .gitconfig in the top level of the repository.
>
> Signed-off-by: Nathan W. Panike <nathan.panike@gmail.com>
> ---
> builtin/config.c | 9 ++++++++-
> config.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index ca4a0db..26c679d 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -154,7 +154,7 @@ static int get_value(const char *key_, const char *regex_)
> {
> int ret = -1;
> char *tl;
> - char *global = NULL, *repo_config = NULL;
> + char *global = NULL, *repo_config = NULL, *shared = NULL;
> const char *system_wide = NULL, *local;
>
> local = config_exclusive_filename;
> @@ -165,6 +165,8 @@ static int get_value(const char *key_, const char *regex_)
> global = xstrdup(mkpath("%s/.gitconfig", home));
> if (git_config_system())
> system_wide = git_etc_gitconfig();
> + if(git_config_shared())
> + shared=xstrdup(".gitconfig");nit: add a space before and after the =
Show 40 quoted lines
> }
>
> key = xstrdup(key_);
> @@ -198,11 +200,15 @@ static int get_value(const char *key_, const char *regex_)
> git_config_from_file(show_config, system_wide, NULL);
> if (do_all && global)
> git_config_from_file(show_config, global, NULL);
> + if (do_all && shared)
> + git_config_from_shared_file(show_config, shared, NULL);
> if (do_all)
> git_config_from_file(show_config, local, NULL);
> git_config_from_parameters(show_config, NULL);
> if (!do_all && !seen)
> git_config_from_file(show_config, local, NULL);
> + if (!do_all && !seen && shared)
> + git_config_from_shared_file(show_config, shared, NULL);
> if (!do_all && !seen && global)
> git_config_from_file(show_config, global, NULL);
> if (!do_all && !seen && system_wide)
> @@ -222,6 +228,7 @@ static int get_value(const char *key_, const char *regex_)
> free_strings:
> free(repo_config);
> free(global);
> + free(shared);
> return ret;
> }
>
> diff --git a/config.c b/config.c
> index c63d683..a9e6bec 100644
> --- a/config.c
> +++ b/config.c
> @@ -795,6 +795,38 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
> return ret;
> }
>
> +struct config_interceptor {
> + config_fn_t fn;
> + void* data;
> +};
> +Would be good to add a description about what is this structure and what is for?
Show 10 quoted lines
> +/*
> + * The purpose of this function is to keep a malicious contributor from
> + * poisoning our configuration. The idea of a shared configuration it to
> + * pass around helpful stuff like aliases, but we do not want to allow someone
> + * to say, change our email address or the url of the remote.
> + */
> +
> +static int config_interceptor_fn(const char*name, const char* value, void* data)
> +{
> + int ret=0;nit: add space before and after the =
> + struct config_interceptor *ci = (struct config_interceptor*)data;
> + if( !ci ) {wrong, wrong.
I think the style used here is:
if (!ci) {So, add a space after the if, and no spaces in the () please.
> + return -1;
> + }
> + if(!prefixcmp(name,"alias."))
same thing here about the space after the if
Show 8 quoted lines
> + ret = (*ci->fn)(name,value,ci->data);
> + return ret;
> +}
> +
> +int git_config_from_shared_file(config_fn_t fn,const char* filename, void* data)
> +{
> + struct config_interceptor ci;
> + ci.fn=fn;nit: fix this too and the other occurrences below.
Nathan W. Panike
