[PATCH] ci: do not die on deprecated-declarations warning

[PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Like a recent GitHub CI run on linux-musl [1] shows, we seem to be
getting a bunch of errors of the form:

  Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:
  since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR
  [-Werror=deprecated-declarations]

For some of them, it may be reasonable to follow the deprecation
notice and update the code, but some symbols like the above is not.

According to the release table [2], 7.85.0 that deprecates
CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year
ago, and according to the symbols-in-versions table [3],
CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will
make us incompatible with anything older than a year if we rewrote
the call as the message suggests.

Make sure that we won't break the build when -Wdeprecated-declarations
triggers.

[1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050
[2] https://curl.se/docs/releases.html
[3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.dev | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/config.mak.dev b/config.mak.dev
index 981304727c..afcffa6a04 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
 endif
 endif
 
+# Libraries deprecate symbols while retaining them for a long time to
+# keep software working with both older and newer versions of them.
+# Getting warnings does help the developers' awareness, but we cannot
+# afford to update too aggressively.  E.g. CURLOPT_REDIR_PROTOCOLS_STR
+# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS
+# but we cannot rewrite the uses of the latter with the former until
+# 7.85.0, which was released in August 2022, becomes ubiquitous.
+DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations
+
 # Old versions of clang complain about initializaing a
 # struct-within-a-struct using just "{0}" rather than "{{0}}".  This
 # error is considered a false-positive and not worth fixing, because
-- 
2.39.0-198-ga38d39a4c5

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Ramsay Jones]

Hi Junio,

On 14/01/2023 03:47, Junio C Hamano wrote:
> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be
> getting a bunch of errors of the form:
> 
>   Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:
>   since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR
>   [-Werror=deprecated-declarations]

Yes, on 30-Dec-2022 I updated my cygwin installation, which updated
the curl package(s) from v7.86.0 to v7.87.0, and caused my build
to fail. I had a quick look at the new 'curl.h' header file and
disabled the deprecation warnings to fix my build. I used vim to
add the following to my config.mak:

    CFLAGS += -DCURL_DISABLE_DEPRECATION

.. and then promptly forgot about it for a couple of weeks! :)

As it happens, just last night I started to look at fixing the code
to avoid the deprecated 'options'. However, if you are not interested
in doing that, I can stop looking into that. ;)

[I was probably doing it wrong anyway - I was just #ifdef-ing code
based on the curl version number, rather than setting something up
in the git-curl-compat.h header file; I haven't grokked that yet.]

> 
> For some of them, it may be reasonable to follow the deprecation
> notice and update the code, but some symbols like the above is not.
> 
> According to the release table [2], 7.85.0 that deprecates
> CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year
> ago, and according to the symbols-in-versions table [3],
> CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will
> make us incompatible with anything older than a year if we rewrote
> the call as the message suggests.
> 
> Make sure that we won't break the build when -Wdeprecated-declarations
> triggers.
> 
> [1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050
> [2] https://curl.se/docs/releases.html
> [3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  config.mak.dev | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/config.mak.dev b/config.mak.dev
> index 981304727c..afcffa6a04 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
>  endif
>  endif
>  
> +# Libraries deprecate symbols while retaining them for a long time to
> +# keep software working with both older and newer versions of them.
> +# Getting warnings does help the developers' awareness, but we cannot
> +# afford to update too aggressively.  E.g. CURLOPT_REDIR_PROTOCOLS_STR
> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS
> +# but we cannot rewrite the uses of the latter with the former until
> +# 7.85.0, which was released in August 2022, becomes ubiquitous.
> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations
> +
>  # Old versions of clang complain about initializaing a
>  # struct-within-a-struct using just "{0}" rather than "{{0}}".  This
>  # error is considered a false-positive and not worth fixing, because

Rather than suppressing 'deprecated-declarations' globally, we could
just add '-DCURL_DISABLE_DEPRECATION' to the compilation of http.c,
http-push.c and remote-curl.c; similar to how we use target specific
rules to pass '-DCURL_DISABLE_TYPECHECK' to sparse (only) to a similar
set of files.

I haven't tried that yet (so famous last words...).

Sorry for not looking into this (and notifying the list) sooner.

ATB,
Ramsay Jones

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote:

> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be
> getting a bunch of errors of the form:
> 
>   Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:
>   since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR
>   [-Werror=deprecated-declarations]
> 
> For some of them, it may be reasonable to follow the deprecation
> notice and update the code, but some symbols like the above is not.

Heh, I was just poking at this myself.

> According to the release table [2], 7.85.0 that deprecates
> CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year
> ago, and according to the symbols-in-versions table [3],
> CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will
> make us incompatible with anything older than a year if we rewrote
> the call as the message suggests.

Yeah, we'd definitely have to keep code covering both versions for a
while. Which is a good reason to punt until the deprecated option is
actually removed, since enough time may have passed by then that we can
avoid having to maintain both versions.

The other thing that makes me care less about this deprecation is that
the deprecation warning in the manpage says:

  We strongly recommend using CURLOPT_REDIR_PROTOCOLS_STR(3) instead
  because this option cannot control all available protocols!

But we don't care about that. We are setting a very small and vanilla
subset of the possible protocols, and the non-STR version is fine for
that.

> +# Libraries deprecate symbols while retaining them for a long time to
> +# keep software working with both older and newer versions of them.
> +# Getting warnings does help the developers' awareness, but we cannot
> +# afford to update too aggressively.  E.g. CURLOPT_REDIR_PROTOCOLS_STR
> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS
> +# but we cannot rewrite the uses of the latter with the former until
> +# 7.85.0, which was released in August 2022, becomes ubiquitous.
> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations

That's a pretty broad hammer. And I think it may stomp on the hack to
rely on deprecated() in the UNUSED macro.

As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to
limit this just to the problematic case. An even more focused option is
to use curl's helper here:

diff --git a/http.c b/http.c
index 17d954dd95..21891493d9 100644
--- a/http.c
+++ b/http.c
@@ -1,3 +1,4 @@
+#define CURL_DISABLE_TYPECHECK 1
 #include "git-compat-util.h"
 #include "git-curl-compat.h"
 #include "http.h"
@@ -979,10 +980,12 @@ 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));
+	CURL_IGNORE_DEPRECATION(
+		curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
+				 get_curl_allowed_protocols(0));
+		curl_easy_setopt(result, CURLOPT_PROTOCOLS,
+				 get_curl_allowed_protocols(-1));
+	)
 	if (getenv("GIT_CURL_VERBOSE"))
 		http_trace_curl_no_data();
 	setup_curl_trace(result);

though I think that was introduced only in 7.87.0 along with the
deprecation warnings themselves, so we'd need to have a fallback like:

  #ifndef CURL_IGNORE_DEPRECATION(x)
  #define CURL_IGNORE_DEPRECATION(x) x
  #endif

-Peff

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote:

> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be
> getting a bunch of errors of the form:
> 
>   Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:
>   since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR
>   [-Werror=deprecated-declarations]

By the way, it seemed odd to me that this failed in just the linux-musl
job, and not elsewhere (nor on my development machine, which has curl
7.87.0). It looks like there's a bad interaction within curl between the
typecheck and deprecation macros. Here's a minimal reproduction:

-- >8 --
cat >foo.c <<-\EOF
#include <curl/curl.h>
void foo(CURL *c)
{
	curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);
}
EOF

# this will complain about deprecated CURLOPT_PROTOCOLS
gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c

# this will not
gcc -Wdeprecated-declarations -c foo.c
-- 8< --

I didn't look into why the musl build behaves differently, but
presumably it has an older compiler or something that causes curl to
decide not to trigger the typecheck macros.

Adding relevant curl folks to the cc (the curl list itself is
subscriber-only).

-Peff

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Sat, Jan 14, 2023 at 09:47:38AM -0500, Jeff King wrote:

> As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to
> limit this just to the problematic case. An even more focused option is
> to use curl's helper here:
> 
> diff --git a/http.c b/http.c
> index 17d954dd95..21891493d9 100644
> --- a/http.c
> +++ b/http.c
> @@ -1,3 +1,4 @@
> +#define CURL_DISABLE_TYPECHECK 1

Oops, this line snuck in from my testing. See my other message for why
it's relevant. ;)

-Peff

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Ramsay Jones]

On 14/01/2023 14:29, Ramsay Jones wrote:
> Hi Junio,
[snip]

> Sorry for not looking into this (and notifying the list) sooner.

Ah, that reminds me...

When I updated to Linux Mint v21.0 (based on latest Ubuntu LTS) last
year, sparse started failing. This was due to a change to the 'regex.h'
header file (the libc6-dev package version was updated from 2.31 to
2.35), where (among other things) the diff looked like:

..

524a525,548
> #ifndef _REGEX_NELTS
> # if (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \
>       && !defined __STDC_NO_VLA__)
> #  define _REGEX_NELTS(n) n
> # else
> #  define _REGEX_NELTS(n)
> # endif
> #endif
> 

..

645c681,682
<                   regmatch_t __pmatch[_Restrict_arr_],
---
>                   regmatch_t __pmatch[_Restrict_arr_
>                                       _REGEX_NELTS (__nmatch)],

..

The last hunk is the declaration of regexec(), thus:

extern int regexec (const regex_t *_Restrict_ __preg,
                    const char *_Restrict_ __String, size_t __nmatch,
                    regmatch_t __pmatch[_Restrict_arr_
                                        _REGEX_NELTS (__nmatch)],
                    int __eflags);

So, sparse falls over on the '__nmatch' as part of the __pmatch
argument declaration. [Actually, it doesn't bomb out on the
declaration, but at each regexec() call site].

To fix my build, I added the following to my config.mak file on linux:

    SPARSE_FLAGS += -D__STDC_NO_VLA__

.. and forgot about it! :)

I need to fix this sometime.

ATB,
Ramsay Jones

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Yes, on 30-Dec-2022 I updated my cygwin installation, which updated
> the curl package(s) from v7.86.0 to v7.87.0, and caused my build
> to fail. I had a quick look at the new 'curl.h' header file and
> disabled the deprecation warnings to fix my build. I used vim to
> add the following to my config.mak:
>
>     CFLAGS += -DCURL_DISABLE_DEPRECATION
>
> .. and then promptly forgot about it for a couple of weeks! :)

Ahh, this is why I love to be working with this community, where I
can float a problem with a possible solution and fellow developers
respond with better solutions very quickly.

>> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations
>> +
>
> Rather than suppressing 'deprecated-declarations' globally, we could
> just add '-DCURL_DISABLE_DEPRECATION' to the compilation of http.c,
> http-push.c and remote-curl.c; similar to how we use target specific
> rules to pass '-DCURL_DISABLE_TYPECHECK' to sparse (only) to a similar
> set of files.

OK, or, assuming that non-curl including code will not be affected
with the macro, we can globally add -DCURL_DISABLE_DEPRECATION to
CFLAGS or DEVELOPER_CFLAGS.

I guess I can sit back and wait until a better version materializes
;-)?

Thanks.

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations
>
> That's a pretty broad hammer. And I think it may stomp on the hack to
> rely on deprecated() in the UNUSED macro.
>
> As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to
> limit this just to the problematic case.

Yeah, I like that approach.

But not this one ...

> +	CURL_IGNORE_DEPRECATION(
> +		curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> +				 get_curl_allowed_protocols(0));
> +		curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> +				 get_curl_allowed_protocols(-1));
> +	)
>
> though I think that was introduced only in 7.87.0 along with the
> deprecation warnings themselves, so we'd need to have a fallback like:
>
>   #ifndef CURL_IGNORE_DEPRECATION(x)
>   #define CURL_IGNORE_DEPRECATION(x) x
>   #endif

... as much.

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +# Libraries deprecate symbols while retaining them for a long time to
>> +# keep software working with both older and newer versions of them.
>> +# Getting warnings does help the developers' awareness, but we cannot
>> +# afford to update too aggressively.  E.g. CURLOPT_REDIR_PROTOCOLS_STR
>> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS
>> +# but we cannot rewrite the uses of the latter with the former until
>> +# 7.85.0, which was released in August 2022, becomes ubiquitous.
>> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations
>
> That's a pretty broad hammer. And I think it may stomp on the hack to
> rely on deprecated() in the UNUSED macro.

True.

> As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to
> limit this just to the problematic case. An even more focused option is
> to use curl's helper here:

One possible downside of the use of CURL_DISABLE_DEPRECATION is that
we may still want to see deprecation warning to learn about upcoming
change.  We just do not want the -Werror to make us die when it
happens.

But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it
goes.

Thanks.

[PATCH v2] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Like a recent GitHub CI run on linux-musl [1] shows, we seem to be
getting a bunch of errors of the form:

  Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:
  since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR
  [-Werror=deprecated-declarations]

For some of them, it may be reasonable to follow the deprecation
notice and update the code, but some symbols like the above is not.

According to the release table [2], 7.85.0 that deprecates
CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year
ago, and according to the symbols-in-versions table [3],
CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will
make us incompatible with anything older than a year if we rewrote
the call as the message suggests.

For now, let's disable the deprecation warnings from libcURL
altogether.  Ideally we may still want to see them to learn about
urgency of future need to rewrite our code (we only want to avoid
-Werror to stop our build).

[1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050
[2] https://curl.se/docs/releases.html
[3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.dev | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/config.mak.dev b/config.mak.dev
index 981304727c..03a0bac8c9 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
 endif
 endif
 
+# Libraries deprecate symbols while retaining them for a long time to
+# keep software working with both older and newer versions of them.
+# Getting warnings does help the developers' awareness, but we cannot
+# afford to update too aggressively.  E.g. CURLOPT_REDIR_PROTOCOLS_STR
+# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS
+# but we cannot rewrite the uses of the latter with the former until
+# 7.85.0, which was released in August 2022, becomes ubiquitous.
+DEVELOPER_CFLAGS += -DCURL_DISABLE_DEPRECATION
+
 # Old versions of clang complain about initializaing a
 # struct-within-a-struct using just "{0}" rather than "{{0}}".  This
 # error is considered a false-positive and not worth fixing, because
-- 
2.39.0-198-ga38d39a4c5

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Sat, Jan 14, 2023 at 08:15:21AM -0800, Junio C Hamano wrote:

> Yeah, I like that approach.
> 
> But not this one ...
> 
> > +	CURL_IGNORE_DEPRECATION(
> > +		curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> > +				 get_curl_allowed_protocols(0));
> > +		curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> > +				 get_curl_allowed_protocols(-1));
> > +	)
> >
> > though I think that was introduced only in 7.87.0 along with the
> > deprecation warnings themselves, so we'd need to have a fallback like:
> >
> >   #ifndef CURL_IGNORE_DEPRECATION(x)
> >   #define CURL_IGNORE_DEPRECATION(x) x
> >   #endif
> 
> ... as much.

I agree it's pretty ugly. The one advantage it has is that we'd be
informed of _other_ curl deprecations that might be more interesting to
us.

On the other hand, I don't know how useful those deprecations are going
to be, as it depends on the timeframe. If the deprecation is added for
the same version of libcurl that implements the alternative (which is
roughly the case here), then we'd always be stuck supporting the old and
new forms (old for backwards compatibility, and new to silence the
deprecation warning). We care a lot more about the deprecation once the
alternative has been around for a while, and/or the old way of doing
things is about to be removed. And if we just wait until that removal,
then we do not have to rely on deprecation warnings. The build will
break just fine on its own. :)

-Peff

Re: [PATCH v2] ci: do not die on deprecated-declarations warning

[Jeff King]

On Sat, Jan 14, 2023 at 09:13:14AM -0800, Junio C Hamano wrote:

> +# Libraries deprecate symbols while retaining them for a long time to
> +# keep software working with both older and newer versions of them.
> +# Getting warnings does help the developers' awareness, but we cannot
> +# afford to update too aggressively.  E.g. CURLOPT_REDIR_PROTOCOLS_STR
> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS
> +# but we cannot rewrite the uses of the latter with the former until
> +# 7.85.0, which was released in August 2022, becomes ubiquitous.
> +DEVELOPER_CFLAGS += -DCURL_DISABLE_DEPRECATION

This seems like a reasonable middle ground to me.

Though we could perhaps even just add it to the main Makefile, and
always pass it. People who aren't using DEVELOPER=1 might be annoyed by
the warnings (and might even be using -Werror themselves!).

-Peff

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On the other hand, I don't know how useful those deprecations are going
> to be, as it depends on the timeframe. If the deprecation is added for
> the same version of libcurl that implements the alternative (which is
> roughly the case here), then we'd always be stuck supporting the old and
> new forms (old for backwards compatibility, and new to silence the
> deprecation warning).

... or just keep the warning without promoting, with "-Wno-error=...".

> We care a lot more about the deprecation once the
> alternative has been around for a while, and/or the old way of doing
> things is about to be removed. And if we just wait until that removal,
> then we do not have to rely on deprecation warnings. The build will
> break just fine on its own. :)

Yes and no.  It is not always like "this symbol is now known under
this different name", which is trivial to adjust.  I briefly tried
to see how IOCTL -> SEEK change should look like until I realized
that the new way was invented too recently and stopped looking, but
it would involve changes to the function logic in the callback
functions, as the function signature---both parameters and its
return values---of the callback changes.  I do not want to see us
scrambling to make such adjustments to the code at the last minute,
so some sort of advance warning is a good thing to have.

Thanks.

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it
> goes.

The "DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations" version
was merged to 'next', only because I wanted to see the commit
cleanly pass the tests (and it does), but I do think in the longer
term (like, before the topic hits 'master'), it probably is better
to do this for everybody, not just for those who use DEVELOPER=Yes.

So, further patches on top are very much welcomed.

Thanks.

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Sat, Jan 14, 2023 at 10:59:18PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On the other hand, I don't know how useful those deprecations are going
> > to be, as it depends on the timeframe. If the deprecation is added for
> > the same version of libcurl that implements the alternative (which is
> > roughly the case here), then we'd always be stuck supporting the old and
> > new forms (old for backwards compatibility, and new to silence the
> > deprecation warning).
> 
> ... or just keep the warning without promoting, with "-Wno-error=...".

I'm pretty strongly against that solution for anything long-term. Having
to see those warnings is not only ugly, but it's confusing and trains
people to ignore them (and many times I've noticed ugly warnings
floating by and realized that oops, I had temporarily disabled -Werror
because I had been working with some older code).

> > We care a lot more about the deprecation once the
> > alternative has been around for a while, and/or the old way of doing
> > things is about to be removed. And if we just wait until that removal,
> > then we do not have to rely on deprecation warnings. The build will
> > break just fine on its own. :)
> 
> Yes and no.  It is not always like "this symbol is now known under
> this different name", which is trivial to adjust.  I briefly tried
> to see how IOCTL -> SEEK change should look like until I realized
> that the new way was invented too recently and stopped looking, but
> it would involve changes to the function logic in the callback
> functions, as the function signature---both parameters and its
> return values---of the callback changes.  I do not want to see us
> scrambling to make such adjustments to the code at the last minute,
> so some sort of advance warning is a good thing to have.

True.

I do think the IOCTL/SEEK one is old enough that we can do, though. The
deprecation is newer, but the SEEK interface was added in an old enough
version.

-Peff

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Sat, Jan 14, 2023 at 11:02:32PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it
> > goes.
> 
> The "DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations" version
> was merged to 'next', only because I wanted to see the commit
> cleanly pass the tests (and it does), but I do think in the longer
> term (like, before the topic hits 'master'), it probably is better
> to do this for everybody, not just for those who use DEVELOPER=Yes.
> 
> So, further patches on top are very much welcomed.

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

 git-curl-compat.h |  8 +++++++
 http-push.c       |  6 ++---
 http.c            | 61 +++++++++++++++++++++++++++++++++--------------
 http.h            |  2 +-
 remote-curl.c     | 28 ++++++++++------------
 5 files changed, 68 insertions(+), 37 deletions(-)

[PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Jeff King]

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

[PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION

[Jeff King]

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

[PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR

[Jeff King]

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

Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Ramsay Jones]

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);

Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION

[Ramsay Jones]

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

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

[Ramsay Jones]

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

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do think the IOCTL/SEEK one is old enough that we can do, though. The
> deprecation is newer, but the SEEK interface was added in an old enough
> version.

Yes, that one is old enough (SEEKFUNCTION and frieds are from 7.18.0
and 7.19.5, IOCTL was deprecated at 7.18.0, released 15 years ago).

A problematic one is REDIR_PROTOCOLS that was deprecated in 7.85.0
(Aug 2022) whose replacement appeared in the same 7.85.0 version.

Thanks.

Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION

[Junio C Hamano]

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.

Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Jeff King]

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

Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION

[Jeff King]

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

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

[Jeff King]

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

Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Jeff King]

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

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Ramsay Jones]

On 14/01/2023 14:56, Jeff King wrote:
> On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote:
> 
>> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be
>> getting a bunch of errors of the form:
>>
>>   Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:
>>   since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR
>>   [-Werror=deprecated-declarations]
> 
> By the way, it seemed odd to me that this failed in just the linux-musl
> job, and not elsewhere (nor on my development machine, which has curl
> 7.87.0). It looks like there's a bad interaction within curl between the
> typecheck and deprecation macros. Here's a minimal reproduction:
> 
> -- >8 --
> cat >foo.c <<-\EOF
> #include <curl/curl.h>
> void foo(CURL *c)
> {
> 	curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);
> }
> EOF
> 
> # this will complain about deprecated CURLOPT_PROTOCOLS
> gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c
> 
> # this will not
> gcc -Wdeprecated-declarations -c foo.c
> -- 8< --

FYI, I just tried this on cygwin and both gcc invocations above
complain about deprecated CURLOPT_PROTOCOLS. (On Linux I have
curl 7.81.0, so I can't test there).

[cygwin uses newlib, of course].

ATB,
Ramsay Jones

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

[Ævar Arnfjörð Bjarmason]

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();

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

[Junio C Hamano]

Æ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.

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

[Junio C Hamano]

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.

Re: [PATCH] ci: do not die on deprecated-declarations warning

[Jeff King]

On Mon, Jan 16, 2023 at 12:39:39AM +0000, Ramsay Jones wrote:

> > By the way, it seemed odd to me that this failed in just the linux-musl
> > job, and not elsewhere (nor on my development machine, which has curl
> > 7.87.0). It looks like there's a bad interaction within curl between the
> > typecheck and deprecation macros. Here's a minimal reproduction:
> > 
> > -- >8 --
> > cat >foo.c <<-\EOF
> > #include <curl/curl.h>
> > void foo(CURL *c)
> > {
> > 	curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);
> > }
> > EOF
> > 
> > # this will complain about deprecated CURLOPT_PROTOCOLS
> > gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c
> > 
> > # this will not
> > gcc -Wdeprecated-declarations -c foo.c
> > -- 8< --
> 
> FYI, I just tried this on cygwin and both gcc invocations above
> complain about deprecated CURLOPT_PROTOCOLS. (On Linux I have
> curl 7.81.0, so I can't test there).

It did work on Linux for me, of course, using 7.87.0. But curiously this
morning it behaved differently!

In the meantime, Debian's libcurl packaging picked up this upstream
patch (and I upgraded):

  https://github.com/curl/curl/commit/e2aed004302e51cfa5b6ce8c8ab65ef92aa83196

and now the deprecation warning happens consistently. So I think on the
curl side there is nothing left to do.

-Peff

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

[Jeff King]

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);

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

[Jeff King]

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

[PATCH v2] avoiding deprecated curl options

[Jeff King]

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);

[PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Jeff King]

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

Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Jeff King]

On Mon, Jan 16, 2023 at 10:03:33PM -0500, 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>

Whoops, this was supposed to be part of v2, but I accidentally sent it
in reply to the wrong part of the thread. Please ignore, and I'll resend
in the right spot.

-Peff

[PATCH v2 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT

[Jeff King]

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

[PATCH v2 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION

[Jeff King]

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

[PATCH v2 3/3] http: support CURLOPT_PROTOCOLS_STR

[Jeff King]

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

Re: [PATCH v2] avoiding deprecated curl options

[Ramsay Jones]

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