Re: Re* [PATCH] builtin-apply.c: use git_config_string() to get apply_default_whitespace

[Christian Couder <chriscool@tuxfamily.org>]

Le dimanche 13 avril 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Le mardi 8 avril 2008, Stephan Beyer a écrit :
> >> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> >> ---
> >> Hi,
> >>
> >> a simple `Janitor patch'.
> >
> > Thanks.
> > ...
> > Tested-by: Christian Couder <chriscool@tuxfamily.org>
> >
> > Junio, please apply.
>
> Hmmmm.
>
> $ git grep -A1 config_error_nonbool | grep -B1 '^[^ ]*-.* = xstrdup'
>
> shows these 13:

Yes, there are many potential places.

> Among these, obviously the one in config.c cannot be replaced (it is the
> implementation of git_config_string() itself ;-), and the one in remote.c
> has a different pattern (it does not return immediately after that, but
> everything else can be mechanically replaced.

I think that each case should be looked at closely. But as it's not very 
difficult either in many cases, I think it's a good thing to let newbies 
try their skills at it. That's why I set up the Janitor page on the wiki:

http://git.or.cz/gitwiki/Janitor

describing this task.

>
> diff --git a/alias.c b/alias.c
> index 116cac8..1513681 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -4,12 +4,8 @@ static const char *alias_key;
>  static char *alias_val;
>  static int alias_lookup_cb(const char *k, const char *v)
>  {
> -	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key)) {
> -		if (!v)
> -			return config_error_nonbool(k);
> -		alias_val = xstrdup(v);
> -		return 0;
> -	}
> +	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key))
> +		return git_config_string((const char **)&alias_val, k, v);
>  	return 0;
>  }

That's also the patch that Stephan sent to the list before, see:

http://thread.gmane.org/gmane.comp.version-control.git/78845

And I told him that casting to (const char **) was ugly, and it was better 
to leave this file as it is.

> diff --git a/builtin-apply.c b/builtin-apply.c
> index abe73a0..c8ca41b 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -2981,12 +2981,8 @@ static int apply_patch(int fd, const char
> *filename, int inaccurate_eof)
>
>  static int git_apply_config(const char *var, const char *value)
>  {
> -	if (!strcmp(var, "apply.whitespace")) {
> -		if (!value)
> -			return config_error_nonbool(var);
> -		apply_default_whitespace = xstrdup(value);
> -		return 0;
> -	}
> +	if (!strcmp(var, "apply.whitespace"))
> +		return git_config_string(&apply_default_whitespace, var, value);
>  	return git_default_config(var, value);
>  }

That's the next patch Stephan sent, and it is indeed good, so please apply 
Stephan's. Thanks.

About other files, I don't care much. That's also why I left it to Janitors.

If I see that you fixed everything I will just remove the Janitor page on 
the wiki.

Regards,
Christian.