# [PATCH] http: fix an unused variable warning for 'curl_no_proxy' 5 messages from 2018-03-14 to 2018-03-15. Participants: Ramsay Jones, Jonathan Nieder, Jeff King. Thread: https://gitlist.dev/t/48045 ## Ramsay Jones, 2018-03-14 21:56 Subject: [PATCH] http: fix an unused variable warning for 'curl_no_proxy' Message-ID: <517c4210-c381-899e-b13a-00f8e4caba74@ramsayjones.plus.com> URL: https://gitlist.dev/e/517c4210-c381-899e-b13a-00f8e4caba74%40ramsayjones.plus.com ``` Signed-off-by: Ramsay Jones --- Hi Junio, I happened to be building git on an _old_ laptop earlier this evening and gcc complained, thus: CC http.o http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable] static const char *curl_no_proxy; ^ The version of libcurl installed was 0x070f04. So, while it was fresh in my mind, I applied and tested this patch. ATB, Ramsay Jones http.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/http.c b/http.c index 8c11156ae..a5bd5d62c 100644 --- a/http.c +++ b/http.c @@ -69,6 +69,9 @@ static const char *ssl_key; #if LIBCURL_VERSION_NUM >= 0x070908 static const char *ssl_capath; #endif +#if LIBCURL_VERSION_NUM >= 0x071304 +static const char *curl_no_proxy; +#endif #if LIBCURL_VERSION_NUM >= 0x072c00 static const char *ssl_pinnedkey; #endif @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1; static long curl_low_speed_time = -1; static int curl_ftp_no_epsv; static const char *curl_http_proxy; -static const char *curl_no_proxy; static const char *http_proxy_authmethod; static struct { const char *name; -- 2.16.0 ``` ## Jonathan Nieder, 2018-03-14 22:03 Subject: Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy' Message-ID: <20180314220307.GB136265@aiede.svl.corp.google.com> URL: https://gitlist.dev/e/20180314220307.GB136265%40aiede.svl.corp.google.com In-Reply-To: <517c4210-c381-899e-b13a-00f8e4caba74@ramsayjones.plus.com> ``` Hi, Ramsay Jones wrote: > Signed-off-by: Ramsay Jones > --- > > Hi Junio, > > I happened to be building git on an _old_ laptop earlier this evening > and gcc complained, thus: > > CC http.o > http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable] > static const char *curl_no_proxy; > ^ > The version of libcurl installed was 0x070f04. So, while it was fresh in my > mind, I applied and tested this patch. Mind including this in the commit message? Especially the error message can be very useful. With or without such a commit message tweak, Reviewed-by: Jonathan Nieder This variable has been unused in the old-curl case since it was introduced in v2.8.0-rc2~2^2 (http: honor no_http env variable to bypass proxy, 2016-02-29). Thanks for fixing it. Sincerely, Jonathan > ATB, > Ramsay Jones > > http.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/http.c b/http.c > index 8c11156ae..a5bd5d62c 100644 > --- a/http.c > +++ b/http.c > @@ -69,6 +69,9 @@ static const char *ssl_key; > #if LIBCURL_VERSION_NUM >= 0x070908 > static const char *ssl_capath; > #endif > +#if LIBCURL_VERSION_NUM >= 0x071304 > +static const char *curl_no_proxy; > +#endif > #if LIBCURL_VERSION_NUM >= 0x072c00 > static const char *ssl_pinnedkey; > #endif > @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1; > static long curl_low_speed_time = -1; > static int curl_ftp_no_epsv; > static const char *curl_http_proxy; > -static const char *curl_no_proxy; > static const char *http_proxy_authmethod; > static struct { > const char *name; ``` ## Jeff King, 2018-03-14 22:15 Subject: Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy' Message-ID: <20180314221544.GA20167@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180314221544.GA20167%40sigill.intra.peff.net In-Reply-To: <517c4210-c381-899e-b13a-00f8e4caba74@ramsayjones.plus.com> ``` On Wed, Mar 14, 2018 at 09:56:06PM +0000, Ramsay Jones wrote: > Signed-off-by: Ramsay Jones > --- > > Hi Junio, > > I happened to be building git on an _old_ laptop earlier this evening > and gcc complained, thus: > > CC http.o > http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable] > static const char *curl_no_proxy; > ^ > The version of libcurl installed was 0x070f04. So, while it was fresh in my > mind, I applied and tested this patch. Makes sense. This #if would go away under my "do not support antique curl versions" proposal. I haven't really pushed that forward since Tom Christensen's patches to actually make the thing build (and presumably since he is building on antique versions he can't turn on -Werror anyway, since IIRC it tends to have some false positives). I agree with Jonathan that this explanation should be in the commit message. The patch itself looks OK, although: > diff --git a/http.c b/http.c > index 8c11156ae..a5bd5d62c 100644 > --- a/http.c > +++ b/http.c > @@ -69,6 +69,9 @@ static const char *ssl_key; > #if LIBCURL_VERSION_NUM >= 0x070908 > static const char *ssl_capath; > #endif > +#if LIBCURL_VERSION_NUM >= 0x071304 > +static const char *curl_no_proxy; > +#endif > #if LIBCURL_VERSION_NUM >= 0x072c00 > static const char *ssl_pinnedkey; > #endif > @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1; > static long curl_low_speed_time = -1; > static int curl_ftp_no_epsv; > static const char *curl_http_proxy; > -static const char *curl_no_proxy; I'm not sure whether our ordering of these variables actually means much, but arguably it makes sense to keep the proxy-related variables near each other, even if one of them has to be surrounded by an #if. I guess you were going for ordering the #if's in increasing version order. I'm not sure the existing code follows that pattern very well. -Peff ``` ## Ramsay Jones, 2018-03-14 23:01 Subject: Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy' Message-ID: URL: https://gitlist.dev/e/d290f115-20b8-2c47-79a1-0fc92e046a94%40ramsayjones.plus.com In-Reply-To: <20180314221544.GA20167@sigill.intra.peff.net> ``` On 14/03/18 22:15, Jeff King wrote: > On Wed, Mar 14, 2018 at 09:56:06PM +0000, Ramsay Jones wrote: > >> Signed-off-by: Ramsay Jones >> --- >> >> Hi Junio, >> >> I happened to be building git on an _old_ laptop earlier this evening >> and gcc complained, thus: >> >> CC http.o >> http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable] >> static const char *curl_no_proxy; >> ^ >> The version of libcurl installed was 0x070f04. So, while it was fresh in my >> mind, I applied and tested this patch. > > Makes sense. This #if would go away under my "do not support antique > curl versions" proposal. I haven't really pushed that forward since Tom > Christensen's patches to actually make the thing build (and presumably > since he is building on antique versions he can't turn on -Werror > anyway, since IIRC it tends to have some false positives). Yes, I suspected this would be removed by your proposed update (hence the cc:), but I didn't know what the ETA on your patches was. Is this worth doing, or are you about to re-visit that series? > I agree with Jonathan that this explanation should be in the commit > message. The patch itself looks OK, although: > >> diff --git a/http.c b/http.c >> index 8c11156ae..a5bd5d62c 100644 >> --- a/http.c >> +++ b/http.c >> @@ -69,6 +69,9 @@ static const char *ssl_key; >> #if LIBCURL_VERSION_NUM >= 0x070908 >> static const char *ssl_capath; >> #endif >> +#if LIBCURL_VERSION_NUM >= 0x071304 >> +static const char *curl_no_proxy; >> +#endif >> #if LIBCURL_VERSION_NUM >= 0x072c00 >> static const char *ssl_pinnedkey; >> #endif >> @@ -77,7 +80,6 @@ static long curl_low_speed_limit = -1; >> static long curl_low_speed_time = -1; >> static int curl_ftp_no_epsv; >> static const char *curl_http_proxy; >> -static const char *curl_no_proxy; > > I'm not sure whether our ordering of these variables actually means > much, but arguably it makes sense to keep the proxy-related variables > near each other, even if one of them has to be surrounded by an #if. > > I guess you were going for ordering the #if's in increasing version > order. I'm not sure the existing code follows that pattern very well. Yes, that was the idea, but I was already in two minds about that! In the end I went with this, because not all of the proxy variables are together anyway. ;-) (see, for example, 'proxy_auth' and 'curl_proxyuserpwd' around line #113). So, I don't mind placing the #ifdef around the current definition (rather than moving it up), if you would prefer that. (It will have to be tomorrow, since I have put that laptop away now!). ATB, Ramsay Jones ``` ## Jeff King, 2018-03-15 00:50 Subject: Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy' Message-ID: <20180315005023.GA22967@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180315005023.GA22967%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Mar 14, 2018 at 11:01:01PM +0000, Ramsay Jones wrote: > >> The version of libcurl installed was 0x070f04. So, while it was fresh in my > >> mind, I applied and tested this patch. > > > > Makes sense. This #if would go away under my "do not support antique > > curl versions" proposal. I haven't really pushed that forward since Tom > > Christensen's patches to actually make the thing build (and presumably > > since he is building on antique versions he can't turn on -Werror > > anyway, since IIRC it tends to have some false positives). > > Yes, I suspected this would be removed by your proposed update (hence > the cc:), but I didn't know what the ETA on your patches was. Is this > worth doing, or are you about to re-visit that series? I wasn't about to revisit it. I wasn't sure if we actually wanted to proceed or not, since Git _does_ actually build now with the old versions (and there was some minor grumbling about dropping compatibility). > > I'm not sure whether our ordering of these variables actually means > > much, but arguably it makes sense to keep the proxy-related variables > > near each other, even if one of them has to be surrounded by an #if. > > > > I guess you were going for ordering the #if's in increasing version > > order. I'm not sure the existing code follows that pattern very well. > > Yes, that was the idea, but I was already in two minds about that! > In the end I went with this, because not all of the proxy variables > are together anyway. ;-) (see, for example, 'proxy_auth' and > 'curl_proxyuserpwd' around line #113). > > So, I don't mind placing the #ifdef around the current definition > (rather than moving it up), if you would prefer that. (It will have > to be tomorrow, since I have put that laptop away now!). I'm OK with it either way, then. Thanks. -Peff ```