* [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
2023-01-15 20:09 ` Jeff King
@ 2023-01-15 20:10 ` Jeff King
2023-01-15 20:54 ` Ramsay Jones
2023-01-15 20:10 ` [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION Jeff King
` (2 subsequent siblings)
3 siblings, 1 reply; 42+ messages in thread
From: Jeff King @ 2023-01-15 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
The two options do exactly the same thing, but the latter has been
deprecated and in recent versions of curl may produce a compiler
warning. Since the UPLOAD form is available everywhere (it was
introduced in the year 2000 by curl 7.1), we can just switch to it.
Signed-off-by: Jeff King <peff@peff.net>
---
http-push.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http-push.c b/http-push.c
index 5f4340a36e..1b18e775d0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,
const char *custom_req, struct buffer *buffer,
curl_write_callback write_fn)
{
- curl_easy_setopt(curl, CURLOPT_PUT, 1);
+ curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
curl_easy_setopt(curl, CURLOPT_URL, url);
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
2023-01-15 20:10 ` [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT Jeff King
@ 2023-01-15 20:54 ` Ramsay Jones
2023-01-15 23:13 ` Jeff King
0 siblings, 1 reply; 42+ messages in thread
From: Ramsay Jones @ 2023-01-15 20:54 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
On 15/01/2023 20:10, Jeff King wrote:
> The two options do exactly the same thing, but the latter has been
> deprecated and in recent versions of curl may produce a compiler
> warning. Since the UPLOAD form is available everywhere (it was
> introduced in the year 2000 by curl 7.1), we can just switch to it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http-push.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/http-push.c b/http-push.c
> index 5f4340a36e..1b18e775d0 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,
> const char *custom_req, struct buffer *buffer,
> curl_write_callback write_fn)
> {
> - curl_easy_setopt(curl, CURLOPT_PUT, 1);
> + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
My version of this patch had '1L' rather than just '1' - but it
doesn't really matter (and was probably because all the curl
examples did so!).
LGTM
ATB,
Ramsay Jones
> curl_easy_setopt(curl, CURLOPT_URL, url);
> curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
> curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
2023-01-15 20:54 ` Ramsay Jones
@ 2023-01-15 23:13 ` Jeff King
2023-01-15 23:49 ` Jeff King
0 siblings, 1 reply; 42+ messages in thread
From: Jeff King @ 2023-01-15 23:13 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, git
On Sun, Jan 15, 2023 at 08:54:59PM +0000, Ramsay Jones wrote:
> > - curl_easy_setopt(curl, CURLOPT_PUT, 1);
> > + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
>
> My version of this patch had '1L' rather than just '1' - but it
> doesn't really matter (and was probably because all the curl
> examples did so!).
Yeah, I think it probably ought to be 1L because it's going to a
variadic function (and it's been a while, but I think the regular
integer promotions make small things into int, but not into long).
So I don't mind fixing it, but I think we'd want it in a separate patch,
since it's orthogonal to what's happening here.
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
2023-01-15 23:13 ` Jeff King
@ 2023-01-15 23:49 ` Jeff King
0 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-15 23:49 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, git
On Sun, Jan 15, 2023 at 06:13:04PM -0500, Jeff King wrote:
> On Sun, Jan 15, 2023 at 08:54:59PM +0000, Ramsay Jones wrote:
>
> > > - curl_easy_setopt(curl, CURLOPT_PUT, 1);
> > > + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
> >
> > My version of this patch had '1L' rather than just '1' - but it
> > doesn't really matter (and was probably because all the curl
> > examples did so!).
>
> Yeah, I think it probably ought to be 1L because it's going to a
> variadic function (and it's been a while, but I think the regular
> integer promotions make small things into int, but not into long).
>
> So I don't mind fixing it, but I think we'd want it in a separate patch,
> since it's orthogonal to what's happening here.
I dug a little bit more and as far as I can tell, yes, "1" is
technically violating the standard but will often work depending on the
ABI and endianness of the platform.
That said, there are other cases besides this one, too, so if we want to
tackle it, let's make it a separate topic (but I'm happy to leave it if
nobody's platform is broken).
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
2023-01-15 20:09 ` Jeff King
2023-01-15 20:10 ` [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT Jeff King
@ 2023-01-15 20:10 ` Jeff King
2023-01-15 21:11 ` Ramsay Jones
2023-01-15 21:45 ` Junio C Hamano
2023-01-15 20:12 ` [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR Jeff King
2023-01-17 3:03 ` [PATCH v2] avoiding deprecated curl options Jeff King
3 siblings, 2 replies; 42+ messages in thread
From: Jeff King @ 2023-01-15 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
The IOCTLFUNCTION option has been deprecated, and generates a compiler
warning in recent versions of curl. We can switch to using SEEKFUNCTION
instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
indicates we require at least curl 7.19.4.
We have to rewrite the ioctl functions into seek functions. In some ways
they are simpler (seeking is the only operation), but in some ways more
complex (the ioctl allowed only a full rewind, but now we can seek to
arbitrary offsets).
Curl will only ever use SEEK_SET (per their documentation), so I didn't
bother implementing anything else, since it would naturally be
completely untested. This seems unlikely to change, but I added an
assertion just in case.
Likewise, I doubt curl will ever try to seek outside of the buffer sizes
we've told it, but I erred on the defensive side here, rather than do an
out-of-bounds read.
Signed-off-by: Jeff King <peff@peff.net>
---
http-push.c | 4 ++--
http.c | 20 +++++++++-----------
http.h | 2 +-
remote-curl.c | 28 +++++++++++++---------------
4 files changed, 25 insertions(+), 29 deletions(-)
diff --git a/http-push.c b/http-push.c
index 1b18e775d0..7f71316456 100644
--- a/http-push.c
+++ b/http-push.c
@@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
- curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
- curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
+ curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
+ curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
diff --git a/http.c b/http.c
index 8a5ba3f477..ca0fe80ddb 100644
--- a/http.c
+++ b/http.c
@@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
return size / eltsize;
}
-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
+int seek_buffer(void *clientp, curl_off_t offset, int origin)
{
struct buffer *buffer = clientp;
- switch (cmd) {
- case CURLIOCMD_NOP:
- return CURLIOE_OK;
-
- case CURLIOCMD_RESTARTREAD:
- buffer->posn = 0;
- return CURLIOE_OK;
-
- default:
- return CURLIOE_UNKNOWNCMD;
+ if (origin != SEEK_SET)
+ BUG("seek_buffer only handles SEEK_SET");
+ if (offset < 0 || offset >= buffer->buf.len) {
+ error("curl seek would be outside of buffer");
+ return CURL_SEEKFUNC_FAIL;
}
+
+ buffer->posn = offset;
+ return CURL_SEEKFUNC_OK;
}
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
diff --git a/http.h b/http.h
index 3c94c47910..77c042706c 100644
--- a/http.h
+++ b/http.h
@@ -40,7 +40,7 @@ struct buffer {
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
+int seek_buffer(void *clientp, curl_off_t offset, int origin);
/* Slot lifecycle functions */
struct active_request_slot *get_active_slot(void);
diff --git a/remote-curl.c b/remote-curl.c
index 72dfb8fb86..a76b6405eb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,
return avail;
}
-static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
+static int rpc_seek(void *clientp, curl_off_t offset, int origin)
{
struct rpc_state *rpc = clientp;
- switch (cmd) {
- case CURLIOCMD_NOP:
- return CURLIOE_OK;
+ if (origin != SEEK_SET)
+ BUG("rpc_seek only handles SEEK_SET, not %d", origin);
- case CURLIOCMD_RESTARTREAD:
- if (rpc->initial_buffer) {
- rpc->pos = 0;
- return CURLIOE_OK;
+ if (rpc->initial_buffer) {
+ if (offset < 0 || offset > rpc->len) {
+ error("curl seek would be outside of rpc buffer");
+ return CURL_SEEKFUNC_FAIL;
}
- error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
- return CURLIOE_FAILRESTART;
-
- default:
- return CURLIOE_UNKNOWNCMD;
+ rpc->pos = offset;
+ return CURL_SEEKFUNC_OK;
}
+ error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
+ return CURL_SEEKFUNC_FAIL;
}
struct check_pktline_state {
@@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
rpc->initial_buffer = 1;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
- curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
- curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
+ curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
+ curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);
if (options.verbosity > 1) {
fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
fflush(stderr);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
2023-01-15 20:10 ` [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION Jeff King
@ 2023-01-15 21:11 ` Ramsay Jones
2023-01-15 21:45 ` Junio C Hamano
1 sibling, 0 replies; 42+ messages in thread
From: Ramsay Jones @ 2023-01-15 21:11 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
On 15/01/2023 20:10, Jeff King wrote:
> The IOCTLFUNCTION option has been deprecated, and generates a compiler
> warning in recent versions of curl. We can switch to using SEEKFUNCTION
> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> indicates we require at least curl 7.19.4.
Ah, I didn't even think about this! I knew they were implemented in 7.18.0
and (as I mentioned previously) used the curl version number to switch
between the two sets of 'options'/implementations. Duh! :(
This is much better!
> We have to rewrite the ioctl functions into seek functions. In some ways
> they are simpler (seeking is the only operation), but in some ways more
> complex (the ioctl allowed only a full rewind, but now we can seek to
> arbitrary offsets).
>
> Curl will only ever use SEEK_SET (per their documentation), so I didn't
> bother implementing anything else, since it would naturally be
> completely untested. This seems unlikely to change, but I added an
> assertion just in case.
>
> Likewise, I doubt curl will ever try to seek outside of the buffer sizes
> we've told it, but I erred on the defensive side here, rather than do an
> out-of-bounds read.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http-push.c | 4 ++--
> http.c | 20 +++++++++-----------
> http.h | 2 +-
> remote-curl.c | 28 +++++++++++++---------------
> 4 files changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index 1b18e775d0..7f71316456 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
> curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
> curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
> curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
> - curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
> - curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
> + curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
> + curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);
> curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
> curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
> curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
> diff --git a/http.c b/http.c
> index 8a5ba3f477..ca0fe80ddb 100644
> --- a/http.c
> +++ b/http.c
> @@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> return size / eltsize;
> }
>
> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
> +int seek_buffer(void *clientp, curl_off_t offset, int origin)
> {
> struct buffer *buffer = clientp;
>
> - switch (cmd) {
> - case CURLIOCMD_NOP:
> - return CURLIOE_OK;
> -
> - case CURLIOCMD_RESTARTREAD:
> - buffer->posn = 0;
> - return CURLIOE_OK;
> -
> - default:
> - return CURLIOE_UNKNOWNCMD;
> + if (origin != SEEK_SET)
> + BUG("seek_buffer only handles SEEK_SET");
I didn't even think to check this; as you say, the documentation
claims only to send SEEK_SET, so ... (but this is obviously a
good idea).
> + if (offset < 0 || offset >= buffer->buf.len) {
> + error("curl seek would be outside of buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
I did at least do this! :)
> +
> + buffer->posn = offset;
> + return CURL_SEEKFUNC_OK;
> }
>
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> diff --git a/http.h b/http.h
> index 3c94c47910..77c042706c 100644
> --- a/http.h
> +++ b/http.h
> @@ -40,7 +40,7 @@ struct buffer {
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
> +int seek_buffer(void *clientp, curl_off_t offset, int origin);
>
> /* Slot lifecycle functions */
> struct active_request_slot *get_active_slot(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 72dfb8fb86..a76b6405eb 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,
> return avail;
> }
>
> -static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> +static int rpc_seek(void *clientp, curl_off_t offset, int origin)
> {
> struct rpc_state *rpc = clientp;
>
> - switch (cmd) {
> - case CURLIOCMD_NOP:
> - return CURLIOE_OK;
> + if (origin != SEEK_SET)
> + BUG("rpc_seek only handles SEEK_SET, not %d", origin);
>
> - case CURLIOCMD_RESTARTREAD:
> - if (rpc->initial_buffer) {
> - rpc->pos = 0;
> - return CURLIOE_OK;
> + if (rpc->initial_buffer) {
> + if (offset < 0 || offset > rpc->len) {
> + error("curl seek would be outside of rpc buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
> - error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
> - return CURLIOE_FAILRESTART;
> -
> - default:
> - return CURLIOE_UNKNOWNCMD;
> + rpc->pos = offset;
> + return CURL_SEEKFUNC_OK;
> }
> + error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
> + return CURL_SEEKFUNC_FAIL;
> }
>
> struct check_pktline_state {
> @@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
> rpc->initial_buffer = 1;
> curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
> curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
> - curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
> - curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
> + curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
> + curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);
> if (options.verbosity > 1) {
> fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
> fflush(stderr);
It looks so much better without #ifdef's (or having to worry about
the git-curl-compat.h header file)!
LGTM
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
2023-01-15 20:10 ` [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION Jeff King
2023-01-15 21:11 ` Ramsay Jones
@ 2023-01-15 21:45 ` Junio C Hamano
2023-01-15 23:17 ` Jeff King
1 sibling, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2023-01-15 21:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ramsay Jones
Jeff King <peff@peff.net> writes:
> The IOCTLFUNCTION option has been deprecated, and generates a compiler
> warning in recent versions of curl. We can switch to using SEEKFUNCTION
> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> indicates we require at least curl 7.19.4.
> ...
> + if (offset < 0 || offset >= buffer->buf.len) {
> + error("curl seek would be outside of buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
> +
> + buffer->posn = offset;
> + return CURL_SEEKFUNC_OK;
> }
Now we depend on having at least cURL 7.19.5 because
CURL_SEEKFUNC_{FAIL,OK} are not available before that version.
cf. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions#L127
Which is fine, as 7.19.5 is from May 2009 that is old enough. We
just need to update the place where you got the 7.19.4 above from.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
2023-01-15 21:45 ` Junio C Hamano
@ 2023-01-15 23:17 ` Jeff King
0 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-15 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
On Sun, Jan 15, 2023 at 01:45:39PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The IOCTLFUNCTION option has been deprecated, and generates a compiler
> > warning in recent versions of curl. We can switch to using SEEKFUNCTION
> > instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> > indicates we require at least curl 7.19.4.
> > ...
> > + if (offset < 0 || offset >= buffer->buf.len) {
> > + error("curl seek would be outside of buffer");
> > + return CURL_SEEKFUNC_FAIL;
> > }
> > +
> > + buffer->posn = offset;
> > + return CURL_SEEKFUNC_OK;
> > }
>
> Now we depend on having at least cURL 7.19.5 because
> CURL_SEEKFUNC_{FAIL,OK} are not available before that version.
>
> cf. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions#L127
Ugh. I did not even check that, because why would anyone introduce the
return values in a separate version! ;)
> Which is fine, as 7.19.5 is from May 2009 that is old enough. We
> just need to update the place where you got the 7.19.4 above from.
OK, if we are all right with bumping the version, I can squash that in.
The other option is to fall back to the obvious 0/1 for OK/FAIL, which
is what curl does. But that feels awfully hacky, and it would only be
used if you were at _exactly_ 7.19.4. So I prefer the bump if it's
acceptable.
diff --git a/INSTALL b/INSTALL
index 3344788397..d5694f8c47 100644
--- a/INSTALL
+++ b/INSTALL
@@ -139,7 +139,7 @@ Issues of note:
not need that functionality, use NO_CURL to build without
it.
- Git requires version "7.19.4" or later of "libcurl" to build
+ Git requires version "7.19.5" or later of "libcurl" to build
without NO_CURL. This version requirement may be bumped in
the future.
-Peff
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-15 20:09 ` Jeff King
2023-01-15 20:10 ` [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT Jeff King
2023-01-15 20:10 ` [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION Jeff King
@ 2023-01-15 20:12 ` Jeff King
2023-01-15 21:37 ` Ramsay Jones
2023-01-16 13:06 ` Ævar Arnfjörð Bjarmason
2023-01-17 3:03 ` [PATCH v2] avoiding deprecated curl options Jeff King
3 siblings, 2 replies; 42+ messages in thread
From: Jeff King @ 2023-01-15 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was
deprecated in curl 7.85.0, and using it generate compiler warnings as of
curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we
can't just do so unilaterally, as it was only introduced less than a
year ago in 7.85.0.
Until that version becomes ubiquitous, we have to either disable the
deprecation warning or conditionally use the "STR" variant on newer
versions of libcurl. This patch switches to the new variant, which is
nice for two reasons:
- we don't have to worry that silencing curl's deprecation warnings
might cause us to miss other more useful ones
- we'd eventually want to move to the new variant anyway, so this gets
us set up (albeit with some extra ugly boilerplate for the
conditional)
There are a lot of ways to split up the two cases. One way would be to
abstract the storage type (strbuf versus a long), how to append
(strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,
and so on. But the resulting code looks pretty magical:
GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;
if (...http is allowed...)
GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP);
and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than
actual code.
On the other end of the spectrum, we could just implement two separate
functions, one that handles a string list and one that handles bits. But
then we end up repeating our list of protocols (http, https, ftp, ftp).
This patch takes the middle ground. The run-time code is always there to
handle both types, and we just choose which one to feed to curl.
Signed-off-by: Jeff King <peff@peff.net>
---
git-curl-compat.h | 8 ++++++++
http.c | 41 ++++++++++++++++++++++++++++++++++-------
2 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/git-curl-compat.h b/git-curl-compat.h
index 56a83b6bbd..fd96b3cdff 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,4 +126,12 @@
#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
#endif
+/**
+ * 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
+
#endif
diff --git a/http.c b/http.c
index ca0fe80ddb..e529ebc993 100644
--- a/http.c
+++ b/http.c
@@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
-static long get_curl_allowed_protocols(int from_user)
+static void proto_list_append(struct strbuf *list_str, const char *proto_str,
+ long *list_bits, long proto_bits)
+{
+ *list_bits |= proto_bits;
+ if (list_str) {
+ if (list_str->len)
+ strbuf_addch(list_str, ',');
+ strbuf_addstr(list_str, proto_str);
+ }
+}
+
+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
{
long allowed_protocols = 0;
if (is_transport_allowed("http", from_user))
- allowed_protocols |= CURLPROTO_HTTP;
+ proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP);
if (is_transport_allowed("https", from_user))
- allowed_protocols |= CURLPROTO_HTTPS;
+ proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS);
if (is_transport_allowed("ftp", from_user))
- allowed_protocols |= CURLPROTO_FTP;
+ proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP);
if (is_transport_allowed("ftps", from_user))
- allowed_protocols |= CURLPROTO_FTPS;
+ proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS);
return allowed_protocols;
}
@@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
+
+#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);
+ }
+#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);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-15 20:12 ` [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR Jeff King
@ 2023-01-15 21:37 ` Ramsay Jones
2023-01-15 23:22 ` Jeff King
2023-01-16 13:06 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 42+ messages in thread
From: Ramsay Jones @ 2023-01-15 21:37 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
On 15/01/2023 20:12, Jeff King wrote:
> The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was
> deprecated in curl 7.85.0, and using it generate compiler warnings as of
> curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we
> can't just do so unilaterally, as it was only introduced less than a
> year ago in 7.85.0.
Yep, I hadn't quite finished my version of this patch yet, but you
would probably not be shocked to learn that I had two separate sets
of functions #ifdef-ed by curl version number! What you have here
looks *much* better!
>
> Until that version becomes ubiquitous, we have to either disable the
> deprecation warning or conditionally use the "STR" variant on newer
> versions of libcurl. This patch switches to the new variant, which is
> nice for two reasons:
>
> - we don't have to worry that silencing curl's deprecation warnings
> might cause us to miss other more useful ones
>
> - we'd eventually want to move to the new variant anyway, so this gets
> us set up (albeit with some extra ugly boilerplate for the
> conditional)
>
> There are a lot of ways to split up the two cases. One way would be to
> abstract the storage type (strbuf versus a long), how to append
> (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,
> and so on. But the resulting code looks pretty magical:
>
> GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;
> if (...http is allowed...)
> GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP);
>
> and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than
> actual code.
>
> On the other end of the spectrum, we could just implement two separate
> functions, one that handles a string list and one that handles bits. But
> then we end up repeating our list of protocols (http, https, ftp, ftp).
>
> This patch takes the middle ground. The run-time code is always there to
> handle both types, and we just choose which one to feed to curl.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> git-curl-compat.h | 8 ++++++++
> http.c | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/git-curl-compat.h b/git-curl-compat.h
> index 56a83b6bbd..fd96b3cdff 100644
> --- a/git-curl-compat.h
> +++ b/git-curl-compat.h
> @@ -126,4 +126,12 @@
> #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
> #endif
>
> +/**
> + * 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. ;)
> +
> #endif
> diff --git a/http.c b/http.c
> index ca0fe80ddb..e529ebc993 100644
> --- a/http.c
> +++ b/http.c
> @@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)
> curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
> }
>
> -static long get_curl_allowed_protocols(int from_user)
> +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> + long *list_bits, long proto_bits)
> +{
> + *list_bits |= proto_bits;
> + if (list_str) {
> + if (list_str->len)
> + strbuf_addch(list_str, ',');
> + strbuf_addstr(list_str, proto_str);
> + }
> +}
> +
> +static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
> {
> long allowed_protocols = 0;
>
> if (is_transport_allowed("http", from_user))
> - allowed_protocols |= CURLPROTO_HTTP;
> + proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP);
> if (is_transport_allowed("https", from_user))
> - allowed_protocols |= CURLPROTO_HTTPS;
> + proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS);
> if (is_transport_allowed("ftp", from_user))
> - allowed_protocols |= CURLPROTO_FTP;
> + proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP);
> if (is_transport_allowed("ftps", from_user))
> - allowed_protocols |= CURLPROTO_FTPS;
> + proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS);
>
> return allowed_protocols;
> }
> @@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)
>
> curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
> curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
> +
> +#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!
> + }
> +#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).
For what it's worth, this LGTM.
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-15 21:37 ` Ramsay Jones
@ 2023-01-15 23:22 ` Jeff King
0 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-15 23:22 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-15 20:12 ` [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR Jeff King
2023-01-15 21:37 ` Ramsay Jones
@ 2023-01-16 13:06 ` Ævar Arnfjörð Bjarmason
2023-01-16 16:05 ` Junio C Hamano
2023-01-16 17:27 ` Jeff King
1 sibling, 2 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-16 13:06 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Ramsay Jones
On Sun, Jan 15 2023, Jeff King wrote:
> The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was
> deprecated in curl 7.85.0, and using it generate compiler warnings as of
> curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we
> can't just do so unilaterally, as it was only introduced less than a
> year ago in 7.85.0.
>
> Until that version becomes ubiquitous, we have to either disable the
> deprecation warning or conditionally use the "STR" variant on newer
> versions of libcurl. This patch switches to the new variant, which is
> nice for two reasons:
>
> - we don't have to worry that silencing curl's deprecation warnings
> might cause us to miss other more useful ones
>
> - we'd eventually want to move to the new variant anyway, so this gets
> us set up (albeit with some extra ugly boilerplate for the
> conditional)
>
> There are a lot of ways to split up the two cases. One way would be to
> abstract the storage type (strbuf versus a long), how to append
> (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,
> and so on. But the resulting code looks pretty magical:
>
> GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;
> if (...http is allowed...)
> GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP);
>
> and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than
> actual code.
>
> On the other end of the spectrum, we could just implement two separate
> functions, one that handles a string list and one that handles bits. But
> then we end up repeating our list of protocols (http, https, ftp, ftp).
>
> This patch takes the middle ground. The run-time code is always there to
> handle both types, and we just choose which one to feed to curl.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> git-curl-compat.h | 8 ++++++++
> http.c | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/git-curl-compat.h b/git-curl-compat.h
> index 56a83b6bbd..fd96b3cdff 100644
> --- a/git-curl-compat.h
> +++ b/git-curl-compat.h
> @@ -126,4 +126,12 @@
> #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
> #endif
>
> +/**
> + * 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
> +
> #endif
Thanks, I hadn't had time to comment on this yet, but was wondering what
this had to do with "CI" or "DEVEDEVELOPER_CFLAGS", the "CI" just seems
to be "where we happened to spot this", and as has been noted this is
also an issue without DEVELOPER_CFLAGS, we just happen to have -Werror
there.
But this is the right direction, and the reason we have git-curl-compat.h.
> diff --git a/http.c b/http.c
> index ca0fe80ddb..e529ebc993 100644
> --- a/http.c
> +++ b/http.c
> @@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)
> curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
> }
>
> -static long get_curl_allowed_protocols(int from_user)
> +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> + long *list_bits, long proto_bits)
> +{
> + *list_bits |= proto_bits;
> + if (list_str) {
> + if (list_str->len)
> + strbuf_addch(list_str, ',');
> + strbuf_addstr(list_str, proto_str);
> + }
> +}
Nit: It would be nice (especially in this even smaller function) to
carry forward the name the parent get_curl_allowed_protocols() uses,
i.e. just "list", not "list_str", ditto "proto" rather than "proto_str".
> +#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);
> + }
> +#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);
I wonder if it isn't better to avoid conditionally compiled code here if
we can avoid it, i.e. just define GIT_* dummy fallbacks, and only use
these if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR is true. I.e. something
like the below squashed in.
diff --git a/git-curl-compat.h b/git-curl-compat.h
index fd96b3cdffd..3bc6e151ca7 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -130,8 +130,13 @@
* 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
+#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)
+#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
+#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR
+#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
+#else
+#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
+#define GIT_CURLOPT_PROTOCOLS_STR 0
#endif
#endif
diff --git a/http.c b/http.c
index e529ebc993d..3224e897f02 100644
--- a/http.c
+++ b/http.c
@@ -933,24 +933,22 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
-#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
- {
+ if (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);
+ curl_easy_setopt(result, GIT_CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);
strbuf_reset(&buf);
get_curl_allowed_protocols(-1, &buf);
- curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);
+ curl_easy_setopt(result, GIT_CURLOPT_PROTOCOLS_STR, buf.buf);
strbuf_release(&buf);
+ } else {
+ curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
+ get_curl_allowed_protocols(0, NULL));
+ curl_easy_setopt(result, CURLOPT_PROTOCOLS,
+ get_curl_allowed_protocols(-1, NULL));
}
-#else
- curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
- get_curl_allowed_protocols(0, NULL));
- curl_easy_setopt(result, CURLOPT_PROTOCOLS,
- get_curl_allowed_protocols(-1, NULL));
-#endif
if (getenv("GIT_CURL_VERBOSE"))
http_trace_curl_no_data();
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-16 13:06 ` Ævar Arnfjörð Bjarmason
@ 2023-01-16 16:05 ` Junio C Hamano
2023-01-16 16:26 ` Junio C Hamano
2023-01-16 17:23 ` Jeff King
2023-01-16 17:27 ` Jeff King
1 sibling, 2 replies; 42+ messages in thread
From: Junio C Hamano @ 2023-01-16 16:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git, Ramsay Jones
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)
> +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR
> +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
> +#else
> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
> +#define GIT_CURLOPT_PROTOCOLS_STR 0
> #endif
I find it a bit ugly that CURLOPT_* being used are all non-zero, but
it may be true in practice.
> diff --git a/http.c b/http.c
> index e529ebc993d..3224e897f02 100644
> --- a/http.c
> +++ b/http.c
> @@ -933,24 +933,22 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
> curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
>
> -#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> - {
> + if (GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR) {
> struct strbuf buf = STRBUF_INIT;
> ...
> + } else {
> + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> + get_curl_allowed_protocols(0, NULL));
> + curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> + get_curl_allowed_protocols(-1, NULL));
> }
> -#else
> - curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> - get_curl_allowed_protocols(0, NULL));
> - curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> - get_curl_allowed_protocols(-1, NULL));
> -#endif
I somehow find the above over-engineered solution looking for a
problem. Conditional compilation might be ugly, but what is uglier
is a conditional compilation hidden behind what pretends to be a
runtime conditional but gets optimized away at compile time. Also,
when the non _STR variant changes status from deprecated to removed,
the code will cease to build, so I am not sure if the above is the
whole story. You'd also need dummy definitions for them when the
version of cURL is advanced enough.
It is true that with the above we will always pass both sides to the
compiler, which may be an upside, but at the same time doesn't it
make it harder to notice and remove the support of the older side
once the time comes?
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-16 16:05 ` Junio C Hamano
@ 2023-01-16 16:26 ` Junio C Hamano
2023-01-16 17:23 ` Jeff King
1 sibling, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2023-01-16 16:26 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git, Ramsay Jones
Junio C Hamano <gitster@pobox.com> writes:
>> +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
>> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR
>> +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
>> +#else
>> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
>> +#define GIT_CURLOPT_PROTOCOLS_STR 0
>> #endif
>
> I find it a bit ugly that CURLOPT_* being used are all non-zero, but
> it may be true in practice.
Sorry for making the first half of the sentence unintelligible. I
meant to say that it is ugly that the correctness of the solution
depends on the real value for these macros being non-zero.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-16 16:05 ` Junio C Hamano
2023-01-16 16:26 ` Junio C Hamano
@ 2023-01-16 17:23 ` Jeff King
1 sibling, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-16 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Ramsay Jones
On Mon, Jan 16, 2023 at 08:05:56AM -0800, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)
> > +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> > +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR
> > +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
> > +#else
> > +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
> > +#define GIT_CURLOPT_PROTOCOLS_STR 0
> > #endif
>
> I find it a bit ugly that CURLOPT_* being used are all non-zero, but
> it may be true in practice.
I don't think it matters what the dummy values are, at least from a
run-time perspective. We'd only feed them to curl inside the "if (0)"
block. They just need to be non-empty so the compiler is happy.
But I do think there's the possibility of compile-time confusion. Curl
is doing macro magic for deprecation and type-checking here, and it's
not clear how it will handle the magic "0" values, even if we know
the code will never actually run.
> I somehow find the above over-engineered solution looking for a
> problem. Conditional compilation might be ugly, but what is uglier
> is a conditional compilation hidden behind what pretends to be a
> runtime conditional but gets optimized away at compile time. Also,
> when the non _STR variant changes status from deprecated to removed,
> the code will cease to build, so I am not sure if the above is the
> whole story. You'd also need dummy definitions for them when the
> version of cURL is advanced enough.
>
> It is true that with the above we will always pass both sides to the
> compiler, which may be an upside, but at the same time doesn't it
> make it harder to notice and remove the support of the older side
> once the time comes?
I likewise found that solution in an uncanny valley of over-engineering
for not much gain.
If you really want to over-engineer, then something like the patch below
at least makes the conditional part simpler, because it shares the
curl_easy_setopt() calls. You can actually take it even further and
abstract the declaration of the strbuf completely (and thus omit it when
it is not going to be used), but it makes the code even more obscure.
I dunno. My original patch tried to err on the side of
simple-and-stupid. If we don't mind repeating the list of
http/https/ftp/ftps, then it can be even simpler (and stupider). But the
eventual cleanup becomes really easy, then: just delete the #else half
of each #ifdef.
diff --git a/git-curl-compat.h b/git-curl-compat.h
index 56a83b6bbd..e545d26dfa 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,4 +126,20 @@
#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
#endif
+/**
+ * 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_PROTOCOL_TYPE const char *
+#define GIT_CURL_PROTOCOL_RETURN(list, bits) list->buf
+#define GIT_CURL_OPT_PROTOCOLS CURLOPT_PROTOCOLS_STR
+#define GIT_CURL_OPT_REDIR_PROTOCOLS CURLOPT_REDIR_PROTOCOLS_STR
+#else
+#define GIT_CURL_PROTOCOL_TYPE long
+#define GIT_CURL_PROTOCOL_RETURN(list, bits) bits
+#define GIT_CURL_OPT_PROTOCOLS CURLOPT_PROTOCOLS
+#define GIT_CURL_OPT_REDIR_PROTOCOLS CURLOPT_REDIR_PROTOCOLS
+#endif
+
#endif
diff --git a/http.c b/http.c
index ca0fe80ddb..e5ed3521db 100644
--- a/http.c
+++ b/http.c
@@ -764,20 +764,32 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
-static long get_curl_allowed_protocols(int from_user)
+static void proto_list_append(struct strbuf *list_str, const char *proto_str,
+ long *list_bits, long proto_bits)
{
- long allowed_protocols = 0;
+ *list_bits |= proto_bits;
+ if (list_str) {
+ if (list_str->len)
+ strbuf_addch(list_str, ',');
+ strbuf_addstr(list_str, proto_str);
+ }
+}
+
+static GIT_CURL_PROTOCOL_TYPE get_curl_allowed_protocols(struct strbuf *list,
+ int from_user)
+{
+ long bits = 0;
if (is_transport_allowed("http", from_user))
- allowed_protocols |= CURLPROTO_HTTP;
+ proto_list_append(list, "http", &bits, CURLPROTO_HTTP);
if (is_transport_allowed("https", from_user))
- allowed_protocols |= CURLPROTO_HTTPS;
+ proto_list_append(list, "https", &bits, CURLPROTO_HTTPS);
if (is_transport_allowed("ftp", from_user))
- allowed_protocols |= CURLPROTO_FTP;
+ proto_list_append(list, "ftp", &bits, CURLPROTO_FTP);
if (is_transport_allowed("ftps", from_user))
- allowed_protocols |= CURLPROTO_FTPS;
+ proto_list_append(list, "ftps", &bits, CURLPROTO_FTPS);
- return allowed_protocols;
+ return GIT_CURL_PROTOCOL_RETURN(list, bits);
}
#ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2
@@ -921,10 +933,17 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
- curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
- get_curl_allowed_protocols(0));
- curl_easy_setopt(result, CURLOPT_PROTOCOLS,
- get_curl_allowed_protocols(-1));
+
+ {
+ struct strbuf buf = STRBUF_INIT;
+
+ curl_easy_setopt(result, GIT_CURL_OPT_REDIR_PROTOCOLS,
+ get_curl_allowed_protocols(&buf, 0));
+ curl_easy_setopt(result, GIT_CURL_OPT_PROTOCOLS,
+ get_curl_allowed_protocols(&buf, -1));
+ strbuf_release(&buf);
+ }
+
if (getenv("GIT_CURL_VERBOSE"))
http_trace_curl_no_data();
setup_curl_trace(result);
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-16 13:06 ` Ævar Arnfjörð Bjarmason
2023-01-16 16:05 ` Junio C Hamano
@ 2023-01-16 17:27 ` Jeff King
1 sibling, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-16 17:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git, Ramsay Jones
On Mon, Jan 16, 2023 at 02:06:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > -static long get_curl_allowed_protocols(int from_user)
> > +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> > + long *list_bits, long proto_bits)
> > +{
> > + *list_bits |= proto_bits;
> > + if (list_str) {
> > + if (list_str->len)
> > + strbuf_addch(list_str, ',');
> > + strbuf_addstr(list_str, proto_str);
> > + }
> > +}
>
> Nit: It would be nice (especially in this even smaller function) to
> carry forward the name the parent get_curl_allowed_protocols() uses,
> i.e. just "list", not "list_str", ditto "proto" rather than "proto_str".
I think it gets confusing in this function, then, because you have both
types. If anything, the sin is in the caller which uses "list" and
"allowed_protocols". I had originally written that as "list" and "bits",
but I left "bits" as "allowed_protocols" to reduce the size of the diff.
Maybe that was a bad choice.
Likewise, the caller could just do the bitwise-OR inline, like:
if (is_transported_allowed("http", from_user)) {
bits |= CURLPROTO_HTTP;
proto_list_append(list, "http");
}
but that makes the diff bigger (the whole function body is replaced,
because the "if" lines, which now have a "{", are no longer unchanged
context). But again, maybe optimizing for a small diff is a bad idea if
the resulting code is harder to follow (I didn't think it was, but then
I also wrote it).
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2] avoiding deprecated curl options
2023-01-15 20:09 ` Jeff King
` (2 preceding siblings ...)
2023-01-15 20:12 ` [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR Jeff King
@ 2023-01-17 3:03 ` Jeff King
2023-01-17 3:04 ` [PATCH v2 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT Jeff King
` (3 more replies)
3 siblings, 4 replies; 42+ messages in thread
From: Jeff King @ 2023-01-17 3:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Ramsay Jones
On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote:
> So I took a look at just dropping the deprecated bits, and it wasn't
> _too_ bad. Here's that series. The first two I hope are obviously good.
> The third one is _ugly_, but at least it punts on the whole "how should
> we silence this" argument, and it takes us in the direction we'd
> ultimately want to go.
>
> [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
> [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
> [3/3]: http: support CURLOPT_PROTOCOLS_STR
In the interests of wrapping this up, here's a v2 that:
- bumps the required curl version to 7.19.5 in patch 2
- aims for slightly better readability in the final code of patch 3,
versus minimizing the diff
As discussed, there are a lot of different ways one could do patch 3,
but I really don't think it's worth spending that much brain power on.
What's here is correct (most important), and I think should be easy to
clean up when we can eventually drop the old style.
[1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
[2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
[3/3]: http: support CURLOPT_PROTOCOLS_STR
INSTALL | 2 +-
git-curl-compat.h | 8 +++++
http-push.c | 6 ++--
http.c | 79 +++++++++++++++++++++++++++++++++--------------
http.h | 2 +-
remote-curl.c | 28 ++++++++---------
6 files changed, 81 insertions(+), 44 deletions(-)
1: 2229c0468f = 1: 5ae6831af5 http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
2: 00120fa40e ! 2: 5be76d74de http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
@@ Commit message
instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
indicates we require at least curl 7.19.4.
- We have to rewrite the ioctl functions into seek functions. In some ways
- they are simpler (seeking is the only operation), but in some ways more
- complex (the ioctl allowed only a full rewind, but now we can seek to
- arbitrary offsets).
+ But there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL},
+ and those didn't arrive until 7.19.5. One workaround would be to use a
+ bare 0/1 here (or define our own macros). But let's just bump the
+ minimum required version to 7.19.5. That version is only a minor version
+ bump from our existing requirement, and is only a 2 month time bump for
+ versions that are almost 13 years old. So it's not likely that anybody
+ cares about the distinction.
+
+ Switching means we have to rewrite the ioctl functions into seek
+ functions. In some ways they are simpler (seeking is the only
+ operation), but in some ways more complex (the ioctl allowed only a full
+ rewind, but now we can seek to arbitrary offsets).
Curl will only ever use SEEK_SET (per their documentation), so I didn't
bother implementing anything else, since it would naturally be
@@ Commit message
Signed-off-by: Jeff King <peff@peff.net>
+ ## INSTALL ##
+@@ INSTALL: Issues of note:
+ not need that functionality, use NO_CURL to build without
+ it.
+
+- Git requires version "7.19.4" or later of "libcurl" to build
++ Git requires version "7.19.5" or later of "libcurl" to build
+ without NO_CURL. This version requirement may be bumped in
+ the future.
+
+
## http-push.c ##
@@ http-push.c: static void curl_setup_http(CURL *curl, const char *url,
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
3: 22eb2fd0fe ! 3: d2c28e22e1 http: support CURLOPT_PROTOCOLS_STR
@@ http.c: void setup_curl_trace(CURL *handle)
}
-static long get_curl_allowed_protocols(int from_user)
-+static void proto_list_append(struct strbuf *list_str, const char *proto_str,
-+ long *list_bits, long proto_bits)
-+{
-+ *list_bits |= proto_bits;
-+ if (list_str) {
-+ if (list_str->len)
-+ strbuf_addch(list_str, ',');
-+ strbuf_addstr(list_str, proto_str);
-+ }
-+}
-+
-+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
++static void proto_list_append(struct strbuf *list, const char *proto)
{
- long allowed_protocols = 0;
+- long allowed_protocols = 0;
++ if (!list)
++ return;
++ if (list->len)
++ strbuf_addch(list, ',');
++ strbuf_addstr(list, proto);
++}
- if (is_transport_allowed("http", from_user))
+- if (is_transport_allowed("http", from_user))
- allowed_protocols |= CURLPROTO_HTTP;
-+ proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP);
- if (is_transport_allowed("https", from_user))
+- if (is_transport_allowed("https", from_user))
- allowed_protocols |= CURLPROTO_HTTPS;
-+ proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS);
- if (is_transport_allowed("ftp", from_user))
+- if (is_transport_allowed("ftp", from_user))
- allowed_protocols |= CURLPROTO_FTP;
-+ proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP);
- if (is_transport_allowed("ftps", from_user))
+- if (is_transport_allowed("ftps", from_user))
- allowed_protocols |= CURLPROTO_FTPS;
-+ proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS);
++static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
++{
++ long bits = 0;
- return allowed_protocols;
+- return allowed_protocols;
++ if (is_transport_allowed("http", from_user)) {
++ bits |= CURLPROTO_HTTP;
++ proto_list_append(list, "http");
++ }
++ if (is_transport_allowed("https", from_user)) {
++ bits |= CURLPROTO_HTTPS;
++ proto_list_append(list, "https");
++ }
++ if (is_transport_allowed("ftp", from_user)) {
++ bits |= CURLPROTO_FTP;
++ proto_list_append(list, "ftp");
++ }
++ if (is_transport_allowed("ftps", from_user)) {
++ bits |= CURLPROTO_FTPS;
++ proto_list_append(list, "ftps");
++ }
++
++ return bits;
}
+
+ #ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2
@@ http.c: static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v2 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
2023-01-17 3:03 ` [PATCH v2] avoiding deprecated curl options Jeff King
@ 2023-01-17 3:04 ` Jeff King
2023-01-17 3:04 ` [PATCH v2 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-17 3:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Ramsay Jones
The two options do exactly the same thing, but the latter has been
deprecated and in recent versions of curl may produce a compiler
warning. Since the UPLOAD form is available everywhere (it was
introduced in the year 2000 by curl 7.1), we can just switch to it.
Signed-off-by: Jeff King <peff@peff.net>
---
http-push.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http-push.c b/http-push.c
index 5f4340a36e..1b18e775d0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,
const char *custom_req, struct buffer *buffer,
curl_write_callback write_fn)
{
- curl_easy_setopt(curl, CURLOPT_PUT, 1);
+ curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
curl_easy_setopt(curl, CURLOPT_URL, url);
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
2023-01-17 3:03 ` [PATCH v2] avoiding deprecated curl options Jeff King
2023-01-17 3:04 ` [PATCH v2 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT Jeff King
@ 2023-01-17 3:04 ` Jeff King
2023-01-17 3:04 ` [PATCH v2 3/3] http: support CURLOPT_PROTOCOLS_STR Jeff King
2023-01-18 1:03 ` [PATCH v2] avoiding deprecated curl options Ramsay Jones
3 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-17 3:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Ramsay Jones
The IOCTLFUNCTION option has been deprecated, and generates a compiler
warning in recent versions of curl. We can switch to using SEEKFUNCTION
instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
indicates we require at least curl 7.19.4.
But there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL},
and those didn't arrive until 7.19.5. One workaround would be to use a
bare 0/1 here (or define our own macros). But let's just bump the
minimum required version to 7.19.5. That version is only a minor version
bump from our existing requirement, and is only a 2 month time bump for
versions that are almost 13 years old. So it's not likely that anybody
cares about the distinction.
Switching means we have to rewrite the ioctl functions into seek
functions. In some ways they are simpler (seeking is the only
operation), but in some ways more complex (the ioctl allowed only a full
rewind, but now we can seek to arbitrary offsets).
Curl will only ever use SEEK_SET (per their documentation), so I didn't
bother implementing anything else, since it would naturally be
completely untested. This seems unlikely to change, but I added an
assertion just in case.
Likewise, I doubt curl will ever try to seek outside of the buffer sizes
we've told it, but I erred on the defensive side here, rather than do an
out-of-bounds read.
Signed-off-by: Jeff King <peff@peff.net>
---
INSTALL | 2 +-
http-push.c | 4 ++--
http.c | 20 +++++++++-----------
http.h | 2 +-
remote-curl.c | 28 +++++++++++++---------------
5 files changed, 26 insertions(+), 30 deletions(-)
diff --git a/INSTALL b/INSTALL
index 3344788397..d5694f8c47 100644
--- a/INSTALL
+++ b/INSTALL
@@ -139,7 +139,7 @@ Issues of note:
not need that functionality, use NO_CURL to build without
it.
- Git requires version "7.19.4" or later of "libcurl" to build
+ Git requires version "7.19.5" or later of "libcurl" to build
without NO_CURL. This version requirement may be bumped in
the future.
diff --git a/http-push.c b/http-push.c
index 1b18e775d0..7f71316456 100644
--- a/http-push.c
+++ b/http-push.c
@@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
- curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
- curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
+ curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
+ curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
diff --git a/http.c b/http.c
index 8a5ba3f477..ca0fe80ddb 100644
--- a/http.c
+++ b/http.c
@@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
return size / eltsize;
}
-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
+int seek_buffer(void *clientp, curl_off_t offset, int origin)
{
struct buffer *buffer = clientp;
- switch (cmd) {
- case CURLIOCMD_NOP:
- return CURLIOE_OK;
-
- case CURLIOCMD_RESTARTREAD:
- buffer->posn = 0;
- return CURLIOE_OK;
-
- default:
- return CURLIOE_UNKNOWNCMD;
+ if (origin != SEEK_SET)
+ BUG("seek_buffer only handles SEEK_SET");
+ if (offset < 0 || offset >= buffer->buf.len) {
+ error("curl seek would be outside of buffer");
+ return CURL_SEEKFUNC_FAIL;
}
+
+ buffer->posn = offset;
+ return CURL_SEEKFUNC_OK;
}
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
diff --git a/http.h b/http.h
index 3c94c47910..77c042706c 100644
--- a/http.h
+++ b/http.h
@@ -40,7 +40,7 @@ struct buffer {
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
+int seek_buffer(void *clientp, curl_off_t offset, int origin);
/* Slot lifecycle functions */
struct active_request_slot *get_active_slot(void);
diff --git a/remote-curl.c b/remote-curl.c
index 72dfb8fb86..a76b6405eb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,
return avail;
}
-static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
+static int rpc_seek(void *clientp, curl_off_t offset, int origin)
{
struct rpc_state *rpc = clientp;
- switch (cmd) {
- case CURLIOCMD_NOP:
- return CURLIOE_OK;
+ if (origin != SEEK_SET)
+ BUG("rpc_seek only handles SEEK_SET, not %d", origin);
- case CURLIOCMD_RESTARTREAD:
- if (rpc->initial_buffer) {
- rpc->pos = 0;
- return CURLIOE_OK;
+ if (rpc->initial_buffer) {
+ if (offset < 0 || offset > rpc->len) {
+ error("curl seek would be outside of rpc buffer");
+ return CURL_SEEKFUNC_FAIL;
}
- error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
- return CURLIOE_FAILRESTART;
-
- default:
- return CURLIOE_UNKNOWNCMD;
+ rpc->pos = offset;
+ return CURL_SEEKFUNC_OK;
}
+ error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
+ return CURL_SEEKFUNC_FAIL;
}
struct check_pktline_state {
@@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
rpc->initial_buffer = 1;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
- curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
- curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
+ curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
+ curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);
if (options.verbosity > 1) {
fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
fflush(stderr);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 3/3] http: support CURLOPT_PROTOCOLS_STR
2023-01-17 3:03 ` [PATCH v2] avoiding deprecated curl options Jeff King
2023-01-17 3:04 ` [PATCH v2 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT Jeff King
2023-01-17 3:04 ` [PATCH v2 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION Jeff King
@ 2023-01-17 3:04 ` Jeff King
2023-01-18 1:03 ` [PATCH v2] avoiding deprecated curl options Ramsay Jones
3 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2023-01-17 3:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Ramsay Jones
The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was
deprecated in curl 7.85.0, and using it generate compiler warnings as of
curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we
can't just do so unilaterally, as it was only introduced less than a
year ago in 7.85.0.
Until that version becomes ubiquitous, we have to either disable the
deprecation warning or conditionally use the "STR" variant on newer
versions of libcurl. This patch switches to the new variant, which is
nice for two reasons:
- we don't have to worry that silencing curl's deprecation warnings
might cause us to miss other more useful ones
- we'd eventually want to move to the new variant anyway, so this gets
us set up (albeit with some extra ugly boilerplate for the
conditional)
There are a lot of ways to split up the two cases. One way would be to
abstract the storage type (strbuf versus a long), how to append
(strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,
and so on. But the resulting code looks pretty magical:
GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;
if (...http is allowed...)
GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP);
and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than
actual code.
On the other end of the spectrum, we could just implement two separate
functions, one that handles a string list and one that handles bits. But
then we end up repeating our list of protocols (http, https, ftp, ftp).
This patch takes the middle ground. The run-time code is always there to
handle both types, and we just choose which one to feed to curl.
Signed-off-by: Jeff King <peff@peff.net>
---
git-curl-compat.h | 8 +++++++
http.c | 59 ++++++++++++++++++++++++++++++++++++-----------
2 files changed, 54 insertions(+), 13 deletions(-)
diff --git a/git-curl-compat.h b/git-curl-compat.h
index 56a83b6bbd..fd96b3cdff 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,4 +126,12 @@
#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
#endif
+/**
+ * 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
+
#endif
diff --git a/http.c b/http.c
index ca0fe80ddb..c4b6ddef28 100644
--- a/http.c
+++ b/http.c
@@ -764,20 +764,37 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
-static long get_curl_allowed_protocols(int from_user)
+static void proto_list_append(struct strbuf *list, const char *proto)
{
- long allowed_protocols = 0;
+ if (!list)
+ return;
+ if (list->len)
+ strbuf_addch(list, ',');
+ strbuf_addstr(list, proto);
+}
- if (is_transport_allowed("http", from_user))
- allowed_protocols |= CURLPROTO_HTTP;
- if (is_transport_allowed("https", from_user))
- allowed_protocols |= CURLPROTO_HTTPS;
- if (is_transport_allowed("ftp", from_user))
- allowed_protocols |= CURLPROTO_FTP;
- if (is_transport_allowed("ftps", from_user))
- allowed_protocols |= CURLPROTO_FTPS;
+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
+{
+ long bits = 0;
- return allowed_protocols;
+ if (is_transport_allowed("http", from_user)) {
+ bits |= CURLPROTO_HTTP;
+ proto_list_append(list, "http");
+ }
+ if (is_transport_allowed("https", from_user)) {
+ bits |= CURLPROTO_HTTPS;
+ proto_list_append(list, "https");
+ }
+ if (is_transport_allowed("ftp", from_user)) {
+ bits |= CURLPROTO_FTP;
+ proto_list_append(list, "ftp");
+ }
+ if (is_transport_allowed("ftps", from_user)) {
+ bits |= CURLPROTO_FTPS;
+ proto_list_append(list, "ftps");
+ }
+
+ return bits;
}
#ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2
@@ -921,10 +938,26 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
+
+#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);
+ }
+#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);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2] avoiding deprecated curl options
2023-01-17 3:03 ` [PATCH v2] avoiding deprecated curl options Jeff King
` (2 preceding siblings ...)
2023-01-17 3:04 ` [PATCH v2 3/3] http: support CURLOPT_PROTOCOLS_STR Jeff King
@ 2023-01-18 1:03 ` Ramsay Jones
3 siblings, 0 replies; 42+ messages in thread
From: Ramsay Jones @ 2023-01-18 1:03 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git
On 17/01/2023 03:03, Jeff King wrote:
> On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote:
>
>> So I took a look at just dropping the deprecated bits, and it wasn't
>> _too_ bad. Here's that series. The first two I hope are obviously good.
>> The third one is _ugly_, but at least it punts on the whole "how should
>> we silence this" argument, and it takes us in the direction we'd
>> ultimately want to go.
>>
>> [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
>> [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
>> [3/3]: http: support CURLOPT_PROTOCOLS_STR
>
> In the interests of wrapping this up, here's a v2 that:
>
> - bumps the required curl version to 7.19.5 in patch 2
>
> - aims for slightly better readability in the final code of patch 3,
> versus minimizing the diff
>
I have a _slight_ preference for your v1 patches, but I don't hate
this version either! :)
Tonight, I have compile tested both v1 and v2 patches (both in 'seen')
on cygwin and linux. I would like to say I have run the tests as well,
but it seems I have disabled all the tests on cygwin (expected) *and*
on linux (most unexpected).
ie. I don't have apache installed. (I used to have apache, svn and cvs
installed to run the tests, but they just took too long to run and
caused *many* test runs to hang and leave server processes all over
the place. On cygwin, even with all of those tests skipped, it still
takes approx 5.5 hours to run the tests).
I thought I was still running the '*http*' tests on linux, but I seem
to have dropped the installation of apache at some point - oops!
I guess I should install apache tomorrow ...
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 42+ messages in thread