Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR

[Jeff King <peff@peff.net>]

On Sun, Jan 15, 2023 at 09:37:07PM +0000, Ramsay Jones wrote:

> > +/**
> > + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,
> > + * released in August 2022.
> > + */
> > +#if LIBCURL_VERSION_NUM >= 0x075500
> > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1
> > +#endif
> 
> Ah, I haven't really grokked what this file is about ... but this
> looks simple enough. ;)

It's newish from the cleanups in e4ff3b67c2 (http: centralize the
accounting of libcurl dependencies, 2021-09-13). I mostly just
cargo-culted this part. ;)

> > +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> > +	{
> > +		struct strbuf buf = STRBUF_INIT;
> > +
> > +		get_curl_allowed_protocols(0, &buf);
> > +		curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);
> > +		strbuf_reset(&buf);
> > +
> > +		get_curl_allowed_protocols(-1, &buf);
> > +		curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);
> > +		strbuf_release(&buf);
> 
> I used two static char arrays to accumulate the strings before
> passing them to curl. I was unsure of the lifetime/ownership
> semantics - I still haven't got around to looking them up!

Really old versions of curl had lifetime issues, but for a long now
(since before the oldest version we'd support), the rule is generally
that curl will copy any opt strings as necessary.

The allocations do feel heavyweight for setting an option. And I think
this get_curl_handle() is really called once per request, so we _could_
probably just generate them once and cache the result. But in general
I've been trying to avoid hidden static variables, etc, as they make
later libification efforts harder. And an extra malloc() on top of an
HTTP request is probably not noticeable.

> > +#else
> >  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> > -			 get_curl_allowed_protocols(0));
> > +			 get_curl_allowed_protocols(0, NULL));
> >  	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> > -			 get_curl_allowed_protocols(-1));
> > +			 get_curl_allowed_protocols(-1, NULL));
> > +#endif
> > +
> >  	if (getenv("GIT_CURL_VERBOSE"))
> >  		http_trace_curl_no_data();
> >  	setup_curl_trace(result);
> 
> (another reason for not completing these patches - I don't
> know what the test coverage is like for these changes; are
> more tests required? dunno).

I had wondered that, too. ;) It's covered by t5812 (my quick and dirty
check was to just drop these lines and see what broke in the test
suite).

-Peff