On Tue, Mar 13, 2012 at 03:02:58PM +0100, Nelson Benitez Leon wrote:
> diff --git a/http.c b/http.c
> index 8ac8eb6..be88acb 100644
> --- a/http.c
> +++ b/http.c
> @@ -42,6 +42,7 @@ static long curl_low_speed_time = -1;
> static int curl_ftp_no_epsv;
> static const char *curl_http_proxy;
> static const char *curl_cookie_file;
> +static struct credential cre_url = CREDENTIAL_INIT;
> static struct credential http_auth = CREDENTIAL_INIT;
> static int http_proactive_auth;
> static const char *user_agent;
This is a minor issue, but if you are re-rolling anyway, would you mind
making this cre_url not global? It is used only in one small code block:
> + if (!curl_http_proxy) {
> + const char *env_proxy, *no_proxy;
> + char *env_proxy_var;
> + int read_http_proxy;
> + struct strbuf buf = STRBUF_INIT;
> + credential_from_url(&cre_url, url);
> + strbuf_addf(&buf, "%s_proxy", cre_url.protocol);
> + env_proxy_var = strbuf_detach(&buf, NULL);
> + env_proxy = getenv(env_proxy_var);
> + if (env_proxy) {
> + read_http_proxy = 1;
> + no_proxy = getenv("no_proxy");
> + if (no_proxy && (!strcmp("*", no_proxy) || strstr(no_proxy, cre_url.host)))
> + read_http_proxy = 0;
> +
> + if (read_http_proxy)
> + curl_http_proxy = xstrdup(env_proxy);
> + }
> + free(env_proxy_var);
> + }
so you could just make it local to this block (also, should you be
checking for the case that there is no protocol?).
I also found this code a little hard to follow because of the number of
variables. You can drop "env_proxy_var" and just access the strbuf's
buffer directly. And you can drop read_http_proxy, as it is used in the
conditional immediately after being set. Plus you can declare a few of
the variables closer to use, and move the variable cleanup closer to
when they are finished being used. So maybe this would be much simpler:
diff --git a/http.c b/http.c
index 8ac8eb6..ea32ecb 100644
--- a/http.c
+++ b/http.c
@@ -295,6 +295,31 @@ static CURL *get_curl_handle(void)
if (curl_ftp_no_epsv)
curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
+ if (!curl_http_proxy) {
+ static struct credential parsed_url = CREDENTIAL_INIT;
+ credential_from_url(&parsed_url, url);
+
+ if (parsed_url.protocol) {
+ const char *env_proxy;
+ struct strbuf var = STRBUF_INIT;
+
+ strbuf_addf(&var, "%s_proxy", parsed_url.protocol);
+ env_proxy = getenv(var.buf);
+ strbuf_release(&var);
+
+ if (env_proxy) {
+ const char *no_proxy = getenv("no_proxy");
+ if (!no_proxy)
+ no_proxy = getenv("NO_PROXY");
+ if (!no_proxy ||
+ (strcmp("*", no_proxy) &&
+ !strstr(no_proxy, parsed_url.host)))
+ curl_http_proxy = xstrdup(env_proxy);
+ }
+ }
+
+ credential_clear(&parsed_url);
+ }
I think it would be even easier to read if it were broken out into a few
functions.
-Peff
--
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