Re: [PATCH v9 3/5] http: always warn if libcurl version is too old

2016-12-14 Thread Brandon Williams
On 12/14, Jeff King wrote:
> On Tue, Dec 13, 2016 at 05:40:35PM -0800, Brandon Williams wrote:
> 
> > diff --git a/transport.c b/transport.c
> > index e1ba78b..fbd799d 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -700,11 +700,6 @@ void transport_check_allowed(const char *type)
> > die("transport '%s' not allowed", type);
> >  }
> >  
> > -int transport_restrict_protocols(void)
> > -{
> > -   return !!protocol_whitelist();
> > -}
> > -
> 
> This function was subtly broken as of patch 2 of the series. It's
> probably not a big deal in the long run, but should the series be
> re-ordered to put this one first?
> 
> I think the commit message would need adjusted, but it probably should
> mention the reasons this is a good idea even _without_ the new config
> system. Namely that even when there's no protocol whitelist, newer
> versions of curl have all of the other non-http protocols disabled.
> 
> I wonder if anybody is actually using a version of curl old enough to
> trigger this. If so, they're going to get the warning every time they
> fetch via http. We might need to stick it behind an "advice.*" config
> option, though I'm inclined to leave it as-is and see if anybody
> actually complains.
> 
> -Peff

Yeah you're right, transport_restrict_protocols() is definitely broken
after patch 2.  Since I'm probably going to need to do a reroll based on
some of your comments in the other patches in the series we might as
well reorder patch 2 and 3 so this isn't broken between patches.

-- 
Brandon Williams


Re: [PATCH v9 3/5] http: always warn if libcurl version is too old

2016-12-14 Thread Jeff King
On Tue, Dec 13, 2016 at 05:40:35PM -0800, Brandon Williams wrote:

> diff --git a/transport.c b/transport.c
> index e1ba78b..fbd799d 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -700,11 +700,6 @@ void transport_check_allowed(const char *type)
>   die("transport '%s' not allowed", type);
>  }
>  
> -int transport_restrict_protocols(void)
> -{
> - return !!protocol_whitelist();
> -}
> -

This function was subtly broken as of patch 2 of the series. It's
probably not a big deal in the long run, but should the series be
re-ordered to put this one first?

I think the commit message would need adjusted, but it probably should
mention the reasons this is a good idea even _without_ the new config
system. Namely that even when there's no protocol whitelist, newer
versions of curl have all of the other non-http protocols disabled.

I wonder if anybody is actually using a version of curl old enough to
trigger this. If so, they're going to get the warning every time they
fetch via http. We might need to stick it behind an "advice.*" config
option, though I'm inclined to leave it as-is and see if anybody
actually complains.

-Peff