Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'
On Wed, Mar 14, 2018 at 11:01:01PM +, 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
Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'
On 14/03/18 22:15, Jeff King wrote: > On Wed, Mar 14, 2018 at 09:56:06PM +, 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
Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'
On Wed, Mar 14, 2018 at 09:56:06PM +, 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
Re: [PATCH] http: fix an unused variable warning for 'curl_no_proxy'
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;
