Git development
 help / color / mirror / Atom feed
* [RFH Patch 2/2] http_init(): Fix config file parsing
@ 2009-03-10  2:19 Junio C Hamano
  2009-03-10  3:05 ` Jay Soffian
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2009-03-10  2:19 UTC (permalink / raw)
  To: git

We honor the command line options, environment variables, variables in
repository configuration file, variables in user's global configuration
file, variables in the system configuration file, and then finally use
built-in default.  To implement this semantics, the code should:

 - start from built-in default values;

 - call git_config() with the configuration parser callback, which
   implements "later definition overrides earlier ones" logic
   (git_config() reads the system's, user's and then repository's
   configuration file in this order);

 - override the result from the above with environment variables if set;

 - override the result from the above with command line options.

The initialization code http_init() for http transfer got this wrong, and
implemented a "first one wins, ignoring the later ones" in http_options(),
to compensate this mistake, read environment variables before calling
git_config().  This is all wrong.

As a second class citizen, the http codepath hasn't been audited as
closely as other parts of the system, but we should try to bring sanity to
the existing codebase before inviting contributors to improve on it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is what I need help from people who actually do use http transport
   and extra set of eyeballs to spot silly mistakes.

 http.c |   66 +++++++++++++++++++++++----------------------------------------
 1 files changed, 24 insertions(+), 42 deletions(-)

diff --git a/http.c b/http.c
index d1ead66..f4f0bf6 100644
--- a/http.c
+++ b/http.c
@@ -94,52 +94,33 @@ static void process_curl_messages(void)
 static int http_options(const char *var, const char *value, void *cb)
 {
 	if (!strcmp("http.sslverify", var)) {
-		if (curl_ssl_verify == -1)
-			curl_ssl_verify = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp("http.sslcert", var)) {
-		if (ssl_cert == NULL)
-			return git_config_string(&ssl_cert, var, value);
+		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslcert", var))
+		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070902
-	if (!strcmp("http.sslkey", var)) {
-		if (ssl_key == NULL)
-			return git_config_string(&ssl_key, var, value);
-		return 0;
-	}
+	if (!strcmp("http.sslkey", var))
+		return git_config_string(&ssl_key, var, value);
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
-	if (!strcmp("http.sslcapath", var)) {
-		if (ssl_capath == NULL)
-			return git_config_string(&ssl_capath, var, value);
-		return 0;
-	}
+	if (!strcmp("http.sslcapath", var))
+		return git_config_string(&ssl_capath, var, value);
 #endif
-	if (!strcmp("http.sslcainfo", var)) {
-		if (ssl_cainfo == NULL)
-			return git_config_string(&ssl_cainfo, var, value);
-		return 0;
-	}
-
+	if (!strcmp("http.sslcainfo", var))
+		return git_config_string(&ssl_cainfo, var, value);
 #ifdef USE_CURL_MULTI
 	if (!strcmp("http.maxrequests", var)) {
-		if (max_requests == -1)
-			max_requests = git_config_int(var, value);
+		max_requests = git_config_int(var, value);
 		return 0;
 	}
 #endif
-
 	if (!strcmp("http.lowspeedlimit", var)) {
-		if (curl_low_speed_limit == -1)
-			curl_low_speed_limit = (long)git_config_int(var, value);
+		curl_low_speed_limit = (long)git_config_int(var, value);
 		return 0;
 	}
 	if (!strcmp("http.lowspeedtime", var)) {
-		if (curl_low_speed_time == -1)
-			curl_low_speed_time = (long)git_config_int(var, value);
+		curl_low_speed_time = (long)git_config_int(var, value);
 		return 0;
 	}
 
@@ -147,11 +128,8 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ftp_no_epsv = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp("http.proxy", var)) {
-		if (curl_http_proxy == NULL)
-			return git_config_string(&curl_http_proxy, var, value);
-		return 0;
-	}
+	if (!strcmp("http.proxy", var))
+		return git_config_string(&curl_http_proxy, var, value);
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
@@ -217,6 +195,8 @@ void http_init(struct remote *remote)
 	char *low_speed_limit;
 	char *low_speed_time;
 
+	git_config(http_options, NULL);
+
 	curl_global_init(CURL_GLOBAL_ALL);
 
 	if (remote && remote->http_proxy)
@@ -241,14 +221,18 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_SSL_NO_VERIFY"))
 		curl_ssl_verify = 0;
 
-	ssl_cert = getenv("GIT_SSL_CERT");
+	if (getenv("GIT_SSL_CERT"))
+		ssl_cert = getenv("GIT_SSL_CERT");
 #if LIBCURL_VERSION_NUM >= 0x070902
-	ssl_key = getenv("GIT_SSL_KEY");
+	if (getenv("GIT_SSL_KEY"))
+		ssl_key = getenv("GIT_SSL_KEY");
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
-	ssl_capath = getenv("GIT_SSL_CAPATH");
+	if (getenv("GIT_SSL_CAPATH"))
+		ssl_capath = getenv("GIT_SSL_CAPATH");
 #endif
-	ssl_cainfo = getenv("GIT_SSL_CAINFO");
+	if (getenv("GIT_SSL_CAINFO"))
+		ssl_cainfo = getenv("GIT_SSL_CAINFO");
 
 	low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT");
 	if (low_speed_limit != NULL)
@@ -257,8 +241,6 @@ void http_init(struct remote *remote)
 	if (low_speed_time != NULL)
 		curl_low_speed_time = strtol(low_speed_time, NULL, 10);
 
-	git_config(http_options, NULL);
-
 	if (curl_ssl_verify == -1)
 		curl_ssl_verify = 1;
 
-- 
1.6.2.206.g5bda76

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-03-10 11:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-10  2:19 [RFH Patch 2/2] http_init(): Fix config file parsing Junio C Hamano
2009-03-10  3:05 ` Jay Soffian
2009-03-10 11:07   ` Johannes Schindelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox