From: Thiago Farina <tfransosi@gmail.com>
Date: Sat, 27 Nov 2010 16:29:43 GMT
Subject: Re: [PATCH 1/2] Add support for a shared config file
Message-ID: <AANLkTi=0MUfdXppze4bPdMnE--koRTvMfRJ=L=cvieOX@mail.gmail.com>
In-Reply-To: <f687e6c4830e21db606db99b584864a61e5cf8c3.1290870042.git.nathan.panike@gmail.com>

Some minor nits (style) below.

On Thu, Nov 25, 2010 at 3:21 PM, Nathan W. Panike
<nathan.panike@gmail.com> wrote:
> 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 =

>        }
>
>        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?

> +/*
> + * 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

> +               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.