Re: [PATCH 1/2] Add support for a shared config file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Some minor nits (style) below.

On Thu, Nov 25, 2010 at 3:21 PM, Nathan W. Panike
<nathan.panike@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> Â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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]