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

From: Jeff King <peff@peff.net>
Date: Jan 15, 2023, 23:22 UTC
Message-ID: <Y8SKxZyh6xn2npbh@coredump.intra.peff.net>
In-Reply-To: <8f175d26-3d84-3019-031d-e358390f2de4@ramsayjones.plus.com>

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

Show 10 quoted lines

> > +/**
> > + * 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. ;)

Show 15 quoted lines

> > +#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.

Show 16 quoted lines

> > +#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