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

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

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi,

a simple `Janitor patch'.

Regards,
 Stephan.

 builtin-apply.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index b5f78ac..ce0a0c3 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2978,12 +2978,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);
 }
 
-- 
1.5.5.rc3.8.g78d1a

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

[Christian Couder]

Le mardi 8 avril 2008, Stephan Beyer a écrit :
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
> Hi,
>
> a simple `Janitor patch'.

Thanks.

>
> Regards,
>  Stephan.
>
>  builtin-apply.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index b5f78ac..ce0a0c3 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -2978,12 +2978,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);
>  }
>
> -- 
> 1.5.5.rc3.8.g78d1a

Tested-by: Christian Couder <chriscool@tuxfamily.org>

Junio, please apply.

Thanks,
Christian.

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

[Junio C Hamano]

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:

alias.c:			return config_error_nonbool(k);
alias.c-		alias_val = xstrdup(v);
--
builtin-apply.c:			return config_error_nonbool(var);
builtin-apply.c-		apply_default_whitespace = xstrdup(value);
--
builtin-commit.c:			return config_error_nonbool(v);
builtin-commit.c-		template_file = xstrdup(v);
--
builtin-log.c:			config_error_nonbool(var);
builtin-log.c-		fmt_patch_subject_prefix = xstrdup(value);
--
builtin-log.c:			return config_error_nonbool(var);
builtin-log.c-		fmt_patch_suffix = xstrdup(value);
--
config.c:		return config_error_nonbool(var);
config.c-	*dest = xstrdup(value);
--
diff.c:			return config_error_nonbool(var);
diff.c-		external_diff_cmd_cfg = xstrdup(value);
--
http.c:				return config_error_nonbool(var);
http.c-			ssl_cert = xstrdup(value);
--
http.c:				return config_error_nonbool(var);
http.c-			ssl_key = xstrdup(value);
--
http.c:				return config_error_nonbool(var);
http.c-			ssl_capath = xstrdup(value);
--
http.c:				return config_error_nonbool(var);
http.c-			ssl_cainfo = xstrdup(value);
--
http.c:				return config_error_nonbool(var);
http.c-			curl_http_proxy = xstrdup(value);
--
remote.c:				return config_error_nonbool(key);
remote.c-			branch->remote_name = xstrdup(value);

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 also have strong doubt about the way http.c handles duplicated
configuration values; everybody else let's the later one override the
previous one, while these take and keep the first occurrence.  I left the
logic of them as they were, because changing them would change the
semantics of the program.

--
 alias.c          |    8 ++------
 builtin-apply.c  |    8 ++------
 builtin-commit.c |    9 ++-------
 builtin-log.c    |   16 ++++------------
 diff.c           |    8 ++------
 http.c           |   44 +++++++++++++++-----------------------------
 6 files changed, 27 insertions(+), 66 deletions(-)

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;
 }
 
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);
 }
 
diff --git a/builtin-commit.c b/builtin-commit.c
index bcb7aaa..995fd73 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -827,13 +827,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 int git_commit_config(const char *k, const char *v)
 {
-	if (!strcmp(k, "commit.template")) {
-		if (!v)
-			return config_error_nonbool(v);
-		template_file = xstrdup(v);
-		return 0;
-	}
-
+	if (!strcmp(k, "commit.template"))
+		return git_config_string((const char **)&template_file, k, v);
 	return git_status_config(k, v);
 }
 
diff --git a/builtin-log.c b/builtin-log.c
index 5c00725..59d8f2e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -226,12 +226,8 @@ static int git_log_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "format.pretty"))
 		return git_config_string(&fmt_pretty, var, value);
-	if (!strcmp(var, "format.subjectprefix")) {
-		if (!value)
-			config_error_nonbool(var);
-		fmt_patch_subject_prefix = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "format.subjectprefix"))
+		return git_config_string(&fmt_patch_subject_prefix, var, value);
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
@@ -478,12 +474,8 @@ static int git_format_config(const char *var, const char *value)
 		add_header(value);
 		return 0;
 	}
-	if (!strcmp(var, "format.suffix")) {
-		if (!value)
-			return config_error_nonbool(var);
-		fmt_patch_suffix = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "format.suffix"))
+		return git_config_string(&fmt_patch_suffix, var, value);
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
 		return 0;
 	}
diff --git a/diff.c b/diff.c
index 8022e67..ce28bbc 100644
--- a/diff.c
+++ b/diff.c
@@ -153,12 +153,8 @@ int git_diff_ui_config(const char *var, const char *value)
 		diff_auto_refresh_index = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.external")) {
-		if (!value)
-			return config_error_nonbool(var);
-		external_diff_cmd_cfg = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "diff.external"))
+		return git_config_string(&external_diff_cmd_cfg, var, value);
 	if (!prefixcmp(var, "diff.")) {
 		const char *ep = strrchr(var, '.');
 
diff --git a/http.c b/http.c
index 256a5f1..9bdf27e 100644
--- a/http.c
+++ b/http.c
@@ -13,18 +13,18 @@ static CURL *curl_default;
 char curl_errorstr[CURL_ERROR_SIZE];
 
 static int curl_ssl_verify = -1;
-static char *ssl_cert = NULL;
+static const char *ssl_cert = NULL;
 #if LIBCURL_VERSION_NUM >= 0x070902
-static char *ssl_key = NULL;
+static const char *ssl_key = NULL;
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
-static char *ssl_capath = NULL;
+static const char *ssl_capath = NULL;
 #endif
-static char *ssl_cainfo = NULL;
+static const char *ssl_cainfo = NULL;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv = 0;
-static char *curl_http_proxy = NULL;
+static const char *curl_http_proxy = NULL;
 
 static struct curl_slist *pragma_header;
 
@@ -100,38 +100,27 @@ static int http_options(const char *var, const char *value)
 	}
 
 	if (!strcmp("http.sslcert", var)) {
-		if (ssl_cert == NULL) {
-			if (!value)
-				return config_error_nonbool(var);
-			ssl_cert = xstrdup(value);
-		}
+		if (ssl_cert == NULL)
+			return git_config_string(&ssl_cert, var, value);
 		return 0;
 	}
 #if LIBCURL_VERSION_NUM >= 0x070902
 	if (!strcmp("http.sslkey", var)) {
-		if (ssl_key == NULL) {
-			if (!value)
-				return config_error_nonbool(var);
-			ssl_key = xstrdup(value);
-		}
+		if (ssl_key == NULL)
+			return git_config_string(&ssl_key, var, value);
 		return 0;
 	}
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
 	if (!strcmp("http.sslcapath", var)) {
-		if (ssl_capath == NULL) {
-			if (!value)
-				return config_error_nonbool(var);
-			ssl_capath = xstrdup(value);
-		}
+		if (ssl_capath == NULL)
+			return git_config_string(&ssl_capath, var, value);
 		return 0;
 	}
 #endif
 	if (!strcmp("http.sslcainfo", var)) {
 		if (ssl_cainfo == NULL) {
-			if (!value)
-				return config_error_nonbool(var);
-			ssl_cainfo = xstrdup(value);
+			return git_config_string(&ssl_cainfo, var, value);
 		}
 		return 0;
 	}
@@ -160,11 +149,8 @@ static int http_options(const char *var, const char *value)
 		return 0;
 	}
 	if (!strcmp("http.proxy", var)) {
-		if (curl_http_proxy == NULL) {
-			if (!value)
-				return config_error_nonbool(var);
-			curl_http_proxy = xstrdup(value);
-		}
+		if (curl_http_proxy == NULL)
+			return git_config_string(&curl_http_proxy, var, value);
 		return 0;
 	}
 
@@ -311,7 +297,7 @@ void http_cleanup(void)
 	pragma_header = NULL;
 
 	if (curl_http_proxy) {
-		free(curl_http_proxy);
+		free((char *)curl_http_proxy);
 		curl_http_proxy = NULL;
 	}
 }

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

[Christian Couder]

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.