{"thread":{"id":"48045","subject":"[PATCH] http: fix an unused variable warning for 'curl_no_proxy'","startedAt":"2018-03-14T21:56:13Z","lastAt":"2018-03-15T00:50:33Z","messageCount":5,"participants":["Ramsay Jones","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"341719","messageId":"517c4210-c381-899e-b13a-00f8e4caba74@ramsayjones.plus.com","threadId":"48045","inReplyTo":null,"subject":"[PATCH] http: fix an unused variable warning for 'curl_no_proxy'","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-14T21:56:06Z","receivedAt":"2018-03-14T21:56:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nI happened to be building git on an _old_ laptop earlier this evening\nand gcc complained, thus:\n\n CC http.o\n http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable]\n static const char *curl_no_proxy;\n ^\nThe version of libcurl installed was 0x070f04. So, while it was fresh in my\nmind, I applied and tested this patch.\n\nATB,\nRamsay Jones\n\n http.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 8c11156ae..a5bd5d62c 100644\n--- a/http.c\n+++ b/http.c\n@@ -69,6 +69,9 @@ static const char *ssl_key;\n #if LIBCURL_VERSION_NUM >= 0x070908\n static const char *ssl_capath;\n #endif\n+#if LIBCURL_VERSION_NUM >= 0x071304\n+static const char *curl_no_proxy;\n+#endif\n #if LIBCURL_VERSION_NUM >= 0x072c00\n static const char *ssl_pinnedkey;\n #endif\n@@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n-static const char *curl_no_proxy;\n static const char *http_proxy_authmethod;\n static struct {\n \tconst char *name;\n-- \n2.16.0\n"},{"id":"341721","messageId":"20180314220307.GB136265@aiede.svl.corp.google.com","threadId":"48045","inReplyTo":"517c4210-c381-899e-b13a-00f8e4caba74@ramsayjones.plus.com","subject":"Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-14T22:03:07Z","receivedAt":"2018-03-14T22:03:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamsay Jones wrote:\n\n> Signed-off-by: Ramsay Jones \n> ---\n>\n> Hi Junio,\n>\n> I happened to be building git on an _old_ laptop earlier this evening\n> and gcc complained, thus:\n>\n> CC http.o\n> http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable]\n> static const char *curl_no_proxy;\n> ^\n> The version of libcurl installed was 0x070f04. So, while it was fresh in my\n> mind, I applied and tested this patch.\n\nMind including this in the commit message? Especially the error message\ncan be very useful.\n\nWith or without such a commit message tweak,\nReviewed-by: Jonathan Nieder \n\nThis variable has been unused in the old-curl case since it was\nintroduced in v2.8.0-rc2~2^2 (http: honor no_http env variable to\nbypass proxy, 2016-02-29). Thanks for fixing it.\n\nSincerely,\nJonathan\n\n> ATB,\n> Ramsay Jones\n> \n> http.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/http.c b/http.c\n> index 8c11156ae..a5bd5d62c 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -69,6 +69,9 @@ static const char *ssl_key;\n> #if LIBCURL_VERSION_NUM >= 0x070908\n> static const char *ssl_capath;\n> #endif\n> +#if LIBCURL_VERSION_NUM >= 0x071304\n> +static const char *curl_no_proxy;\n> +#endif\n> #if LIBCURL_VERSION_NUM >= 0x072c00\n> static const char *ssl_pinnedkey;\n> #endif\n> @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1;\n> static long curl_low_speed_time = -1;\n> static int curl_ftp_no_epsv;\n> static const char *curl_http_proxy;\n> -static const char *curl_no_proxy;\n> static const char *http_proxy_authmethod;\n> static struct {\n> \tconst char *name;\n"},{"id":"341722","messageId":"20180314221544.GA20167@sigill.intra.peff.net","threadId":"48045","inReplyTo":"517c4210-c381-899e-b13a-00f8e4caba74@ramsayjones.plus.com","subject":"Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-14T22:15:44Z","receivedAt":"2018-03-14T22:15:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 14, 2018 at 09:56:06PM +0000, Ramsay Jones wrote:\n\n> Signed-off-by: Ramsay Jones \n> ---\n> \n> Hi Junio,\n> \n> I happened to be building git on an _old_ laptop earlier this evening\n> and gcc complained, thus:\n> \n> CC http.o\n> http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable]\n> static const char *curl_no_proxy;\n> ^\n> The version of libcurl installed was 0x070f04. So, while it was fresh in my\n> mind, I applied and tested this patch.\n\nMakes sense. This #if would go away under my \"do not support antique\ncurl versions\" proposal. I haven't really pushed that forward since Tom\nChristensen's patches to actually make the thing build (and presumably\nsince he is building on antique versions he can't turn on -Werror\nanyway, since IIRC it tends to have some false positives).\n\nI agree with Jonathan that this explanation should be in the commit\nmessage. The patch itself looks OK, although:\n\n> diff --git a/http.c b/http.c\n> index 8c11156ae..a5bd5d62c 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -69,6 +69,9 @@ static const char *ssl_key;\n> #if LIBCURL_VERSION_NUM >= 0x070908\n> static const char *ssl_capath;\n> #endif\n> +#if LIBCURL_VERSION_NUM >= 0x071304\n> +static const char *curl_no_proxy;\n> +#endif\n> #if LIBCURL_VERSION_NUM >= 0x072c00\n> static const char *ssl_pinnedkey;\n> #endif\n> @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1;\n> static long curl_low_speed_time = -1;\n> static int curl_ftp_no_epsv;\n> static const char *curl_http_proxy;\n> -static const char *curl_no_proxy;\n\nI'm not sure whether our ordering of these variables actually means\nmuch, but arguably it makes sense to keep the proxy-related variables\nnear each other, even if one of them has to be surrounded by an #if.\n\nI guess you were going for ordering the #if's in increasing version\norder. I'm not sure the existing code follows that pattern very well.\n\n-Peff\n"},{"id":"341726","messageId":"d290f115-20b8-2c47-79a1-0fc92e046a94@ramsayjones.plus.com","threadId":"48045","inReplyTo":"20180314221544.GA20167@sigill.intra.peff.net","subject":"Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-14T23:01:01Z","receivedAt":"2018-03-14T23:01:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 14/03/18 22:15, Jeff King wrote:\n> On Wed, Mar 14, 2018 at 09:56:06PM +0000, Ramsay Jones wrote:\n> \n>> Signed-off-by: Ramsay Jones \n>> ---\n>>\n>> Hi Junio,\n>>\n>> I happened to be building git on an _old_ laptop earlier this evening\n>> and gcc complained, thus:\n>>\n>> CC http.o\n>> http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable]\n>> static const char *curl_no_proxy;\n>> ^\n>> The version of libcurl installed was 0x070f04. So, while it was fresh in my\n>> mind, I applied and tested this patch.\n> \n> Makes sense. This #if would go away under my \"do not support antique\n> curl versions\" proposal. I haven't really pushed that forward since Tom\n> Christensen's patches to actually make the thing build (and presumably\n> since he is building on antique versions he can't turn on -Werror\n> anyway, since IIRC it tends to have some false positives).\n\nYes, I suspected this would be removed by your proposed update (hence\nthe cc:), but I didn't know what the ETA on your patches was. Is this\nworth doing, or are you about to re-visit that series?\n\n> I agree with Jonathan that this explanation should be in the commit\n> message. The patch itself looks OK, although:\n> \n>> diff --git a/http.c b/http.c\n>> index 8c11156ae..a5bd5d62c 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -69,6 +69,9 @@ static const char *ssl_key;\n>> #if LIBCURL_VERSION_NUM >= 0x070908\n>> static const char *ssl_capath;\n>> #endif\n>> +#if LIBCURL_VERSION_NUM >= 0x071304\n>> +static const char *curl_no_proxy;\n>> +#endif\n>> #if LIBCURL_VERSION_NUM >= 0x072c00\n>> static const char *ssl_pinnedkey;\n>> #endif\n>> @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1;\n>> static long curl_low_speed_time = -1;\n>> static int curl_ftp_no_epsv;\n>> static const char *curl_http_proxy;\n>> -static const char *curl_no_proxy;\n> \n> I'm not sure whether our ordering of these variables actually means\n> much, but arguably it makes sense to keep the proxy-related variables\n> near each other, even if one of them has to be surrounded by an #if.\n> \n> I guess you were going for ordering the #if's in increasing version\n> order. I'm not sure the existing code follows that pattern very well.\n\nYes, that was the idea, but I was already in two minds about that!\nIn the end I went with this, because not all of the proxy variables\nare together anyway. ;-) (see, for example, 'proxy_auth' and \n'curl_proxyuserpwd' around line #113).\n\nSo, I don't mind placing the #ifdef around the current definition\n(rather than moving it up), if you would prefer that. (It will have\nto be tomorrow, since I have put that laptop away now!).\n\nATB,\nRamsay Jones\n\n"},{"id":"341729","messageId":"20180315005023.GA22967@sigill.intra.peff.net","threadId":"48045","inReplyTo":"d290f115-20b8-2c47-79a1-0fc92e046a94@ramsayjones.plus.com","subject":"Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-15T00:50:24Z","receivedAt":"2018-03-15T00:50:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 14, 2018 at 11:01:01PM +0000, Ramsay Jones wrote:\n\n> >> The version of libcurl installed was 0x070f04. So, while it was fresh in my\n> >> mind, I applied and tested this patch.\n> > \n> > Makes sense. This #if would go away under my \"do not support antique\n> > curl versions\" proposal. I haven't really pushed that forward since Tom\n> > Christensen's patches to actually make the thing build (and presumably\n> > since he is building on antique versions he can't turn on -Werror\n> > anyway, since IIRC it tends to have some false positives).\n> \n> Yes, I suspected this would be removed by your proposed update (hence\n> the cc:), but I didn't know what the ETA on your patches was. Is this\n> worth doing, or are you about to re-visit that series?\n\nI wasn't about to revisit it. I wasn't sure if we actually wanted to\nproceed or not, since Git _does_ actually build now with the old\nversions (and there was some minor grumbling about dropping\ncompatibility).\n\n> > I'm not sure whether our ordering of these variables actually means\n> > much, but arguably it makes sense to keep the proxy-related variables\n> > near each other, even if one of them has to be surrounded by an #if.\n> > \n> > I guess you were going for ordering the #if's in increasing version\n> > order. I'm not sure the existing code follows that pattern very well.\n> \n> Yes, that was the idea, but I was already in two minds about that!\n> In the end I went with this, because not all of the proxy variables\n> are together anyway. ;-) (see, for example, 'proxy_auth' and \n> 'curl_proxyuserpwd' around line #113).\n> \n> So, I don't mind placing the #ifdef around the current definition\n> (rather than moving it up), if you would prefer that. (It will have\n> to be tomorrow, since I have put that laptop away now!).\n\nI'm OK with it either way, then. Thanks.\n\n-Peff\n"}]}