{"thread":{"id":"59086","subject":"[PATCH] ci: do not die on deprecated-declarations warning","startedAt":"2023-01-14T03:47:29Z","lastAt":"2023-01-18T01:09:11Z","messageCount":42,"participants":["Junio C Hamano","Ramsay Jones","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470320","messageId":"xmqqv8l9n5fj.fsf@gitster.g","threadId":"59086","inReplyTo":null,"subject":"[PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-14T03:47:12Z","receivedAt":"2023-01-14T03:47:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Like a recent GitHub CI run on linux-musl [1] shows, we seem to be\ngetting a bunch of errors of the form:\n\n Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:\n since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR\n [-Werror=deprecated-declarations]\n\nFor some of them, it may be reasonable to follow the deprecation\nnotice and update the code, but some symbols like the above is not.\n\nAccording to the release table [2], 7.85.0 that deprecates\nCURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year\nago, and according to the symbols-in-versions table [3],\nCURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will\nmake us incompatible with anything older than a year if we rewrote\nthe call as the message suggests.\n\nMake sure that we won't break the build when -Wdeprecated-declarations\ntriggers.\n\n[1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050\n[2] https://curl.se/docs/releases.html\n[3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions\n\nSigned-off-by: Junio C Hamano \n---\n config.mak.dev | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 981304727c..afcffa6a04 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces\n endif\n endif\n \n+# Libraries deprecate symbols while retaining them for a long time to\n+# keep software working with both older and newer versions of them.\n+# Getting warnings does help the developers' awareness, but we cannot\n+# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR\n+# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS\n+# but we cannot rewrite the uses of the latter with the former until\n+# 7.85.0, which was released in August 2022, becomes ubiquitous.\n+DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\n+\n # Old versions of clang complain about initializaing a\n # struct-within-a-struct using just \"{0}\" rather than \"{{0}}\". This\n # error is considered a false-positive and not worth fixing, because\n-- \n2.39.0-198-ga38d39a4c5\n\n"},{"id":"470339","messageId":"11dccdea-0e33-6669-9554-19f064df6010@ramsayjones.plus.com","threadId":"59086","inReplyTo":"xmqqv8l9n5fj.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-14T14:29:27Z","receivedAt":"2023-01-14T14:32:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Junio,\n\nOn 14/01/2023 03:47, Junio C Hamano wrote:\n> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be\n> getting a bunch of errors of the form:\n> \n> Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:\n> since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR\n> [-Werror=deprecated-declarations]\n\nYes, on 30-Dec-2022 I updated my cygwin installation, which updated\nthe curl package(s) from v7.86.0 to v7.87.0, and caused my build\nto fail. I had a quick look at the new 'curl.h' header file and\ndisabled the deprecation warnings to fix my build. I used vim to\nadd the following to my config.mak:\n\n CFLAGS += -DCURL_DISABLE_DEPRECATION\n\n.. and then promptly forgot about it for a couple of weeks! :)\n\nAs it happens, just last night I started to look at fixing the code\nto avoid the deprecated 'options'. However, if you are not interested\nin doing that, I can stop looking into that. ;)\n\n[I was probably doing it wrong anyway - I was just #ifdef-ing code\nbased on the curl version number, rather than setting something up\nin the git-curl-compat.h header file; I haven't grokked that yet.]\n\n> \n> For some of them, it may be reasonable to follow the deprecation\n> notice and update the code, but some symbols like the above is not.\n> \n> According to the release table [2], 7.85.0 that deprecates\n> CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year\n> ago, and according to the symbols-in-versions table [3],\n> CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will\n> make us incompatible with anything older than a year if we rewrote\n> the call as the message suggests.\n> \n> Make sure that we won't break the build when -Wdeprecated-declarations\n> triggers.\n> \n> [1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050\n> [2] https://curl.se/docs/releases.html\n> [3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> config.mak.dev | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n> \n> diff --git a/config.mak.dev b/config.mak.dev\n> index 981304727c..afcffa6a04 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces\n> endif\n> endif\n> \n> +# Libraries deprecate symbols while retaining them for a long time to\n> +# keep software working with both older and newer versions of them.\n> +# Getting warnings does help the developers' awareness, but we cannot\n> +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR\n> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS\n> +# but we cannot rewrite the uses of the latter with the former until\n> +# 7.85.0, which was released in August 2022, becomes ubiquitous.\n> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\n> +\n> # Old versions of clang complain about initializaing a\n> # struct-within-a-struct using just \"{0}\" rather than \"{{0}}\". This\n> # error is considered a false-positive and not worth fixing, because\n\nRather than suppressing 'deprecated-declarations' globally, we could\njust add '-DCURL_DISABLE_DEPRECATION' to the compilation of http.c,\nhttp-push.c and remote-curl.c; similar to how we use target specific\nrules to pass '-DCURL_DISABLE_TYPECHECK' to sparse (only) to a similar\nset of files.\n\nI haven't tried that yet (so famous last words...).\n\nSorry for not looking into this (and notifying the list) sooner.\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"470341","messageId":"Y8LAim4D3g6qnZdq@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqqv8l9n5fj.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-14T14:47:38Z","receivedAt":"2023-01-14T14:49:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote:\n\n> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be\n> getting a bunch of errors of the form:\n> \n> Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:\n> since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR\n> [-Werror=deprecated-declarations]\n> \n> For some of them, it may be reasonable to follow the deprecation\n> notice and update the code, but some symbols like the above is not.\n\nHeh, I was just poking at this myself.\n\n> According to the release table [2], 7.85.0 that deprecates\n> CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year\n> ago, and according to the symbols-in-versions table [3],\n> CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will\n> make us incompatible with anything older than a year if we rewrote\n> the call as the message suggests.\n\nYeah, we'd definitely have to keep code covering both versions for a\nwhile. Which is a good reason to punt until the deprecated option is\nactually removed, since enough time may have passed by then that we can\navoid having to maintain both versions.\n\nThe other thing that makes me care less about this deprecation is that\nthe deprecation warning in the manpage says:\n\n We strongly recommend using CURLOPT_REDIR_PROTOCOLS_STR(3) instead\n because this option cannot control all available protocols!\n\nBut we don't care about that. We are setting a very small and vanilla\nsubset of the possible protocols, and the non-STR version is fine for\nthat.\n\n> +# Libraries deprecate symbols while retaining them for a long time to\n> +# keep software working with both older and newer versions of them.\n> +# Getting warnings does help the developers' awareness, but we cannot\n> +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR\n> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS\n> +# but we cannot rewrite the uses of the latter with the former until\n> +# 7.85.0, which was released in August 2022, becomes ubiquitous.\n> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\n\nThat's a pretty broad hammer. And I think it may stomp on the hack to\nrely on deprecated() in the UNUSED macro.\n\nAs Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to\nlimit this just to the problematic case. An even more focused option is\nto use curl's helper here:\n\ndiff --git a/http.c b/http.c\nindex 17d954dd95..21891493d9 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,3 +1,4 @@\n+#define CURL_DISABLE_TYPECHECK 1\n #include \"git-compat-util.h\"\n #include \"git-curl-compat.h\"\n #include \"http.h\"\n@@ -979,10 +980,12 @@ static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n-\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(0));\n-\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(-1));\n+\tCURL_IGNORE_DEPRECATION(\n+\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n+\t\t\t\t get_curl_allowed_protocols(0));\n+\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n+\t\t\t\t get_curl_allowed_protocols(-1));\n+\t)\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(result);\n\nthough I think that was introduced only in 7.87.0 along with the\ndeprecation warnings themselves, so we'd need to have a fallback like:\n\n #ifndef CURL_IGNORE_DEPRECATION(x)\n #define CURL_IGNORE_DEPRECATION(x) x\n #endif\n\n-Peff\n"},{"id":"470343","messageId":"Y8LCsxz8gyTCxFUp@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqqv8l9n5fj.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-14T14:56:51Z","receivedAt":"2023-01-14T14:56:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote:\n\n> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be\n> getting a bunch of errors of the form:\n> \n> Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:\n> since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR\n> [-Werror=deprecated-declarations]\n\nBy the way, it seemed odd to me that this failed in just the linux-musl\njob, and not elsewhere (nor on my development machine, which has curl\n7.87.0). It looks like there's a bad interaction within curl between the\ntypecheck and deprecation macros. Here's a minimal reproduction:\n\n-- >8 --\ncat >foo.c <<-\\EOF\n#include \nvoid foo(CURL *c)\n{\n\tcurl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);\n}\nEOF\n\n# this will complain about deprecated CURLOPT_PROTOCOLS\ngcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c\n\n# this will not\ngcc -Wdeprecated-declarations -c foo.c\n-- 8< --\n\nI didn't look into why the musl build behaves differently, but\npresumably it has an older compiler or something that causes curl to\ndecide not to trigger the typecheck macros.\n\nAdding relevant curl folks to the cc (the curl list itself is\nsubscriber-only).\n\n-Peff\n"},{"id":"470344","messageId":"Y8LC44cxKt0wfB7q@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8LAim4D3g6qnZdq@coredump.intra.peff.net","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-14T14:57:39Z","receivedAt":"2023-01-14T14:57:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 14, 2023 at 09:47:38AM -0500, Jeff King wrote:\n\n> As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to\n> limit this just to the problematic case. An even more focused option is\n> to use curl's helper here:\n> \n> diff --git a/http.c b/http.c\n> index 17d954dd95..21891493d9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1,3 +1,4 @@\n> +#define CURL_DISABLE_TYPECHECK 1\n\nOops, this line snuck in from my testing. See my other message for why\nit's relevant. ;)\n\n-Peff\n"},{"id":"470346","messageId":"7498bff0-782f-8c81-5817-ed841f12dbba@ramsayjones.plus.com","threadId":"59086","inReplyTo":"11dccdea-0e33-6669-9554-19f064df6010@ramsayjones.plus.com","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-14T15:14:44Z","receivedAt":"2023-01-14T15:14:52Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 14/01/2023 14:29, Ramsay Jones wrote:\n> Hi Junio,\n[snip]\n\n> Sorry for not looking into this (and notifying the list) sooner.\n\nAh, that reminds me...\n\nWhen I updated to Linux Mint v21.0 (based on latest Ubuntu LTS) last\nyear, sparse started failing. This was due to a change to the 'regex.h'\nheader file (the libc6-dev package version was updated from 2.31 to\n2.35), where (among other things) the diff looked like:\n\n..\n\n524a525,548\n> #ifndef _REGEX_NELTS\n> # if (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \\\n> && !defined __STDC_NO_VLA__)\n> # define _REGEX_NELTS(n) n\n> # else\n> # define _REGEX_NELTS(n)\n> # endif\n> #endif\n> \n\n..\n\n645c681,682\n< regmatch_t __pmatch[_Restrict_arr_],\n---\n> regmatch_t __pmatch[_Restrict_arr_\n> _REGEX_NELTS (__nmatch)],\n\n..\n\nThe last hunk is the declaration of regexec(), thus:\n\nextern int regexec (const regex_t *_Restrict_ __preg,\n const char *_Restrict_ __String, size_t __nmatch,\n regmatch_t __pmatch[_Restrict_arr_\n _REGEX_NELTS (__nmatch)],\n int __eflags);\n\nSo, sparse falls over on the '__nmatch' as part of the __pmatch\nargument declaration. [Actually, it doesn't bomb out on the\ndeclaration, but at each regexec() call site].\n\nTo fix my build, I added the following to my config.mak file on linux:\n\n SPARSE_FLAGS += -D__STDC_NO_VLA__\n\n.. and forgot about it! :)\n\nI need to fix this sometime.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"470350","messageId":"xmqq1qnxm6vu.fsf@gitster.g","threadId":"59086","inReplyTo":"11dccdea-0e33-6669-9554-19f064df6010@ramsayjones.plus.com","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-14T16:13:25Z","receivedAt":"2023-01-14T16:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Yes, on 30-Dec-2022 I updated my cygwin installation, which updated\n> the curl package(s) from v7.86.0 to v7.87.0, and caused my build\n> to fail. I had a quick look at the new 'curl.h' header file and\n> disabled the deprecation warnings to fix my build. I used vim to\n> add the following to my config.mak:\n>\n> CFLAGS += -DCURL_DISABLE_DEPRECATION\n>\n> .. and then promptly forgot about it for a couple of weeks! :)\n\nAhh, this is why I love to be working with this community, where I\ncan float a problem with a possible solution and fellow developers\nrespond with better solutions very quickly.\n\n>> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\n>> +\n>\n> Rather than suppressing 'deprecated-declarations' globally, we could\n> just add '-DCURL_DISABLE_DEPRECATION' to the compilation of http.c,\n> http-push.c and remote-curl.c; similar to how we use target specific\n> rules to pass '-DCURL_DISABLE_TYPECHECK' to sparse (only) to a similar\n> set of files.\n\nOK, or, assuming that non-curl including code will not be affected\nwith the macro, we can globally add -DCURL_DISABLE_DEPRECATION to\nCFLAGS or DEVELOPER_CFLAGS.\n\nI guess I can sit back and wait until a better version materializes\n;-)?\n\nThanks.\n\n"},{"id":"470351","messageId":"xmqqv8l9ks86.fsf@gitster.g","threadId":"59086","inReplyTo":"Y8LAim4D3g6qnZdq@coredump.intra.peff.net","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-14T16:15:21Z","receivedAt":"2023-01-14T16:15:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\n>\n> That's a pretty broad hammer. And I think it may stomp on the hack to\n> rely on deprecated() in the UNUSED macro.\n>\n> As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to\n> limit this just to the problematic case.\n\nYeah, I like that approach.\n\nBut not this one ...\n\n> +\tCURL_IGNORE_DEPRECATION(\n> +\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> +\t\t\t\t get_curl_allowed_protocols(0));\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> +\t\t\t\t get_curl_allowed_protocols(-1));\n> +\t)\n>\n> though I think that was introduced only in 7.87.0 along with the\n> deprecation warnings themselves, so we'd need to have a fallback like:\n>\n> #ifndef CURL_IGNORE_DEPRECATION(x)\n> #define CURL_IGNORE_DEPRECATION(x) x\n> #endif\n\n... as much.\n"},{"id":"470354","messageId":"xmqqilh9kqdy.fsf@gitster.g","threadId":"59086","inReplyTo":"Y8LAim4D3g6qnZdq@coredump.intra.peff.net","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-14T16:55:05Z","receivedAt":"2023-01-14T16:55:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +# Libraries deprecate symbols while retaining them for a long time to\n>> +# keep software working with both older and newer versions of them.\n>> +# Getting warnings does help the developers' awareness, but we cannot\n>> +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR\n>> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS\n>> +# but we cannot rewrite the uses of the latter with the former until\n>> +# 7.85.0, which was released in August 2022, becomes ubiquitous.\n>> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\n>\n> That's a pretty broad hammer. And I think it may stomp on the hack to\n> rely on deprecated() in the UNUSED macro.\n\nTrue.\n\n> As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to\n> limit this just to the problematic case. An even more focused option is\n> to use curl's helper here:\n\nOne possible downside of the use of CURL_DISABLE_DEPRECATION is that\nwe may still want to see deprecation warning to learn about upcoming\nchange. We just do not want the -Werror to make us die when it\nhappens.\n\nBut anyway, let's use CURL_DISABLE_DEPRECATION first to see how it\ngoes.\n\nThanks.\n"},{"id":"470355","messageId":"xmqq7cxpkpjp.fsf@gitster.g","threadId":"59086","inReplyTo":"xmqqv8l9n5fj.fsf@gitster.g","subject":"[PATCH v2] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-14T17:13:14Z","receivedAt":"2023-01-14T17:13:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Like a recent GitHub CI run on linux-musl [1] shows, we seem to be\ngetting a bunch of errors of the form:\n\n Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:\n since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR\n [-Werror=deprecated-declarations]\n\nFor some of them, it may be reasonable to follow the deprecation\nnotice and update the code, but some symbols like the above is not.\n\nAccording to the release table [2], 7.85.0 that deprecates\nCURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year\nago, and according to the symbols-in-versions table [3],\nCURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will\nmake us incompatible with anything older than a year if we rewrote\nthe call as the message suggests.\n\nFor now, let's disable the deprecation warnings from libcURL\naltogether. Ideally we may still want to see them to learn about\nurgency of future need to rewrite our code (we only want to avoid\n-Werror to stop our build).\n\n[1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050\n[2] https://curl.se/docs/releases.html\n[3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions\n\nSigned-off-by: Junio C Hamano \n---\n config.mak.dev | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 981304727c..03a0bac8c9 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces\n endif\n endif\n \n+# Libraries deprecate symbols while retaining them for a long time to\n+# keep software working with both older and newer versions of them.\n+# Getting warnings does help the developers' awareness, but we cannot\n+# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR\n+# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS\n+# but we cannot rewrite the uses of the latter with the former until\n+# 7.85.0, which was released in August 2022, becomes ubiquitous.\n+DEVELOPER_CFLAGS += -DCURL_DISABLE_DEPRECATION\n+\n # Old versions of clang complain about initializaing a\n # struct-within-a-struct using just \"{0}\" rather than \"{{0}}\". This\n # error is considered a false-positive and not worth fixing, because\n-- \n2.39.0-198-ga38d39a4c5\n\n"},{"id":"470356","messageId":"Y8LjTYhTycp/tTBn@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqqv8l9ks86.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-14T17:15:57Z","receivedAt":"2023-01-14T17:16:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 14, 2023 at 08:15:21AM -0800, Junio C Hamano wrote:\n\n> Yeah, I like that approach.\n> \n> But not this one ...\n> \n> > +\tCURL_IGNORE_DEPRECATION(\n> > +\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> > +\t\t\t\t get_curl_allowed_protocols(0));\n> > +\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> > +\t\t\t\t get_curl_allowed_protocols(-1));\n> > +\t)\n> >\n> > though I think that was introduced only in 7.87.0 along with the\n> > deprecation warnings themselves, so we'd need to have a fallback like:\n> >\n> > #ifndef CURL_IGNORE_DEPRECATION(x)\n> > #define CURL_IGNORE_DEPRECATION(x) x\n> > #endif\n> \n> ... as much.\n\nI agree it's pretty ugly. The one advantage it has is that we'd be\ninformed of _other_ curl deprecations that might be more interesting to\nus.\n\nOn the other hand, I don't know how useful those deprecations are going\nto be, as it depends on the timeframe. If the deprecation is added for\nthe same version of libcurl that implements the alternative (which is\nroughly the case here), then we'd always be stuck supporting the old and\nnew forms (old for backwards compatibility, and new to silence the\ndeprecation warning). We care a lot more about the deprecation once the\nalternative has been around for a while, and/or the old way of doing\nthings is about to be removed. And if we just wait until that removal,\nthen we do not have to rely on deprecation warnings. The build will\nbreak just fine on its own. :)\n\n-Peff\n"},{"id":"470357","messageId":"Y8Ljp7yEWTUd92w8@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqq7cxpkpjp.fsf@gitster.g","subject":"Re: [PATCH v2] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-14T17:17:27Z","receivedAt":"2023-01-14T17:17:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 14, 2023 at 09:13:14AM -0800, Junio C Hamano wrote:\n\n> +# Libraries deprecate symbols while retaining them for a long time to\n> +# keep software working with both older and newer versions of them.\n> +# Getting warnings does help the developers' awareness, but we cannot\n> +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR\n> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS\n> +# but we cannot rewrite the uses of the latter with the former until\n> +# 7.85.0, which was released in August 2022, becomes ubiquitous.\n> +DEVELOPER_CFLAGS += -DCURL_DISABLE_DEPRECATION\n\nThis seems like a reasonable middle ground to me.\n\nThough we could perhaps even just add it to the main Makefile, and\nalways pass it. People who aren't using DEVELOPER=1 might be annoyed by\nthe warnings (and might even be using -Werror themselves!).\n\n-Peff\n"},{"id":"470378","messageId":"xmqq4jssi8qh.fsf@gitster.g","threadId":"59086","inReplyTo":"Y8LjTYhTycp/tTBn@coredump.intra.peff.net","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T06:59:18Z","receivedAt":"2023-01-15T06:59:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On the other hand, I don't know how useful those deprecations are going\n> to be, as it depends on the timeframe. If the deprecation is added for\n> the same version of libcurl that implements the alternative (which is\n> roughly the case here), then we'd always be stuck supporting the old and\n> new forms (old for backwards compatibility, and new to silence the\n> deprecation warning).\n\n... or just keep the warning without promoting, with \"-Wno-error=...\".\n\n> We care a lot more about the deprecation once the\n> alternative has been around for a while, and/or the old way of doing\n> things is about to be removed. And if we just wait until that removal,\n> then we do not have to rely on deprecation warnings. The build will\n> break just fine on its own. :)\n\nYes and no. It is not always like \"this symbol is now known under\nthis different name\", which is trivial to adjust. I briefly tried\nto see how IOCTL -> SEEK change should look like until I realized\nthat the new way was invented too recently and stopped looking, but\nit would involve changes to the function logic in the callback\nfunctions, as the function signature---both parameters and its\nreturn values---of the callback changes. I do not want to see us\nscrambling to make such adjustments to the code at the last minute,\nso some sort of advance warning is a good thing to have.\n\nThanks.\n\n"},{"id":"470379","messageId":"xmqqzgakgu0n.fsf@gitster.g","threadId":"59086","inReplyTo":"xmqqilh9kqdy.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T07:02:32Z","receivedAt":"2023-01-15T07:02:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it\n> goes.\n\nThe \"DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\" version\nwas merged to 'next', only because I wanted to see the commit\ncleanly pass the tests (and it does), but I do think in the longer\nterm (like, before the topic hits 'master'), it probably is better\nto do this for everybody, not just for those who use DEVELOPER=Yes.\n\nSo, further patches on top are very much welcomed.\n\nThanks.\n\n\n"},{"id":"470389","messageId":"Y8RdOt02JTvDnLiX@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqq4jssi8qh.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T20:08:26Z","receivedAt":"2023-01-15T20:08:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 14, 2023 at 10:59:18PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On the other hand, I don't know how useful those deprecations are going\n> > to be, as it depends on the timeframe. If the deprecation is added for\n> > the same version of libcurl that implements the alternative (which is\n> > roughly the case here), then we'd always be stuck supporting the old and\n> > new forms (old for backwards compatibility, and new to silence the\n> > deprecation warning).\n> \n> ... or just keep the warning without promoting, with \"-Wno-error=...\".\n\nI'm pretty strongly against that solution for anything long-term. Having\nto see those warnings is not only ugly, but it's confusing and trains\npeople to ignore them (and many times I've noticed ugly warnings\nfloating by and realized that oops, I had temporarily disabled -Werror\nbecause I had been working with some older code).\n\n> > We care a lot more about the deprecation once the\n> > alternative has been around for a while, and/or the old way of doing\n> > things is about to be removed. And if we just wait until that removal,\n> > then we do not have to rely on deprecation warnings. The build will\n> > break just fine on its own. :)\n> \n> Yes and no. It is not always like \"this symbol is now known under\n> this different name\", which is trivial to adjust. I briefly tried\n> to see how IOCTL -> SEEK change should look like until I realized\n> that the new way was invented too recently and stopped looking, but\n> it would involve changes to the function logic in the callback\n> functions, as the function signature---both parameters and its\n> return values---of the callback changes. I do not want to see us\n> scrambling to make such adjustments to the code at the last minute,\n> so some sort of advance warning is a good thing to have.\n\nTrue.\n\nI do think the IOCTL/SEEK one is old enough that we can do, though. The\ndeprecation is newer, but the SEEK interface was added in an old enough\nversion.\n\n-Peff\n"},{"id":"470390","messageId":"Y8RddcM9Vr71ljp4@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqqzgakgu0n.fsf@gitster.g","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T20:09:25Z","receivedAt":"2023-01-15T20:09:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 14, 2023 at 11:02:32PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it\n> > goes.\n> \n> The \"DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations\" version\n> was merged to 'next', only because I wanted to see the commit\n> cleanly pass the tests (and it does), but I do think in the longer\n> term (like, before the topic hits 'master'), it probably is better\n> to do this for everybody, not just for those who use DEVELOPER=Yes.\n> \n> So, further patches on top are very much welcomed.\n\nSo I took a look at just dropping the deprecated bits, and it wasn't\n_too_ bad. Here's that series. The first two I hope are obviously good.\nThe third one is _ugly_, but at least it punts on the whole \"how should\nwe silence this\" argument, and it takes us in the direction we'd\nultimately want to go.\n\n [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT\n [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION\n [3/3]: http: support CURLOPT_PROTOCOLS_STR\n\n git-curl-compat.h | 8 +++++++\n http-push.c | 6 ++---\n http.c | 61 +++++++++++++++++++++++++++++++++--------------\n http.h | 2 +-\n remote-curl.c | 28 ++++++++++------------\n 5 files changed, 68 insertions(+), 37 deletions(-)\n\n"},{"id":"470391","messageId":"Y8RduNqadAfaOgs1@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8RddcM9Vr71ljp4@coredump.intra.peff.net","subject":"[PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T20:10:32Z","receivedAt":"2023-01-15T20:10:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The two options do exactly the same thing, but the latter has been\ndeprecated and in recent versions of curl may produce a compiler\nwarning. Since the UPLOAD form is available everywhere (it was\nintroduced in the year 2000 by curl 7.1), we can just switch to it.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 5f4340a36e..1b18e775d0 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,\n \t\tconst char *custom_req, struct buffer *buffer,\n \t\tcurl_write_callback write_fn)\n {\n-\tcurl_easy_setopt(curl, CURLOPT_PUT, 1);\n+\tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1);\n \tcurl_easy_setopt(curl, CURLOPT_URL, url);\n \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n-- \n2.39.0.513.g00e40dbe01\n\n"},{"id":"470392","messageId":"Y8Rd0KXYcHKykvjq@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8RddcM9Vr71ljp4@coredump.intra.peff.net","subject":"[PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T20:10:56Z","receivedAt":"2023-01-15T20:11:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The IOCTLFUNCTION option has been deprecated, and generates a compiler\nwarning in recent versions of curl. We can switch to using SEEKFUNCTION\ninstead. It was added in 2008 via curl 7.18.0; our INSTALL file already\nindicates we require at least curl 7.19.4.\n\nWe have to rewrite the ioctl functions into seek functions. In some ways\nthey are simpler (seeking is the only operation), but in some ways more\ncomplex (the ioctl allowed only a full rewind, but now we can seek to\narbitrary offsets).\n\nCurl will only ever use SEEK_SET (per their documentation), so I didn't\nbother implementing anything else, since it would naturally be\ncompletely untested. This seems unlikely to change, but I added an\nassertion just in case.\n\nLikewise, I doubt curl will ever try to seek outside of the buffer sizes\nwe've told it, but I erred on the defensive side here, rather than do an\nout-of-bounds read.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 4 ++--\n http.c | 20 +++++++++-----------\n http.h | 2 +-\n remote-curl.c | 28 +++++++++++++---------------\n 4 files changed, 25 insertions(+), 29 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 1b18e775d0..7f71316456 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,\n \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n \tcurl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);\n-\tcurl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);\n-\tcurl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);\n+\tcurl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);\n+\tcurl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);\n \tcurl_easy_setopt(curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);\ndiff --git a/http.c b/http.c\nindex 8a5ba3f477..ca0fe80ddb 100644\n--- a/http.c\n+++ b/http.c\n@@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \treturn size / eltsize;\n }\n \n-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)\n+int seek_buffer(void *clientp, curl_off_t offset, int origin)\n {\n \tstruct buffer *buffer = clientp;\n \n-\tswitch (cmd) {\n-\tcase CURLIOCMD_NOP:\n-\t\treturn CURLIOE_OK;\n-\n-\tcase CURLIOCMD_RESTARTREAD:\n-\t\tbuffer->posn = 0;\n-\t\treturn CURLIOE_OK;\n-\n-\tdefault:\n-\t\treturn CURLIOE_UNKNOWNCMD;\n+\tif (origin != SEEK_SET)\n+\t\tBUG(\"seek_buffer only handles SEEK_SET\");\n+\tif (offset < 0 || offset >= buffer->buf.len) {\n+\t\terror(\"curl seek would be outside of buffer\");\n+\t\treturn CURL_SEEKFUNC_FAIL;\n \t}\n+\n+\tbuffer->posn = offset;\n+\treturn CURL_SEEKFUNC_OK;\n }\n \n size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\ndiff --git a/http.h b/http.h\nindex 3c94c47910..77c042706c 100644\n--- a/http.h\n+++ b/http.h\n@@ -40,7 +40,7 @@ struct buffer {\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);\n+int seek_buffer(void *clientp, curl_off_t offset, int origin);\n \n /* Slot lifecycle functions */\n struct active_request_slot *get_active_slot(void);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 72dfb8fb86..a76b6405eb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \treturn avail;\n }\n \n-static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n+static int rpc_seek(void *clientp, curl_off_t offset, int origin)\n {\n \tstruct rpc_state *rpc = clientp;\n \n-\tswitch (cmd) {\n-\tcase CURLIOCMD_NOP:\n-\t\treturn CURLIOE_OK;\n+\tif (origin != SEEK_SET)\n+\t\tBUG(\"rpc_seek only handles SEEK_SET, not %d\", origin);\n \n-\tcase CURLIOCMD_RESTARTREAD:\n-\t\tif (rpc->initial_buffer) {\n-\t\t\trpc->pos = 0;\n-\t\t\treturn CURLIOE_OK;\n+\tif (rpc->initial_buffer) {\n+\t\tif (offset < 0 || offset > rpc->len) {\n+\t\t\terror(\"curl seek would be outside of rpc buffer\");\n+\t\t\treturn CURL_SEEKFUNC_FAIL;\n \t\t}\n-\t\terror(_(\"unable to rewind rpc post data - try increasing http.postBuffer\"));\n-\t\treturn CURLIOE_FAILRESTART;\n-\n-\tdefault:\n-\t\treturn CURLIOE_UNKNOWNCMD;\n+\t\trpc->pos = offset;\n+\t\treturn CURL_SEEKFUNC_OK;\n \t}\n+\terror(_(\"unable to rewind rpc post data - try increasing http.postBuffer\"));\n+\treturn CURL_SEEKFUNC_FAIL;\n }\n \n struct check_pktline_state {\n@@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \t\trpc->initial_buffer = 1;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);\n \t\tif (options.verbosity > 1) {\n \t\t\tfprintf(stderr, \"POST %s (chunked)\\n\", rpc->service_name);\n \t\t\tfflush(stderr);\n-- \n2.39.0.513.g00e40dbe01\n\n"},{"id":"470393","messageId":"Y8ReHbGWetJHQcI1@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8RddcM9Vr71ljp4@coredump.intra.peff.net","subject":"[PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T20:12:13Z","receivedAt":"2023-01-15T20:12:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was\ndeprecated in curl 7.85.0, and using it generate compiler warnings as of\ncurl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we\ncan't just do so unilaterally, as it was only introduced less than a\nyear ago in 7.85.0.\n\nUntil that version becomes ubiquitous, we have to either disable the\ndeprecation warning or conditionally use the \"STR\" variant on newer\nversions of libcurl. This patch switches to the new variant, which is\nnice for two reasons:\n\n - we don't have to worry that silencing curl's deprecation warnings\n might cause us to miss other more useful ones\n\n - we'd eventually want to move to the new variant anyway, so this gets\n us set up (albeit with some extra ugly boilerplate for the\n conditional)\n\nThere are a lot of ways to split up the two cases. One way would be to\nabstract the storage type (strbuf versus a long), how to append\n(strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,\nand so on. But the resulting code looks pretty magical:\n\n GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;\n if (...http is allowed...)\n\tGIT_CURL_PROTOCOL_APPEND(&allowed, \"http\", CURLOPT_HTTP);\n\nand you end up with more \"#define GIT_CURL_PROTOCOL_TYPE\" macros than\nactual code.\n\nOn the other end of the spectrum, we could just implement two separate\nfunctions, one that handles a string list and one that handles bits. But\nthen we end up repeating our list of protocols (http, https, ftp, ftp).\n\nThis patch takes the middle ground. The run-time code is always there to\nhandle both types, and we just choose which one to feed to curl.\n\nSigned-off-by: Jeff King \n---\n git-curl-compat.h | 8 ++++++++\n http.c | 41 ++++++++++++++++++++++++++++++++++-------\n 2 files changed, 42 insertions(+), 7 deletions(-)\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex 56a83b6bbd..fd96b3cdff 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -126,4 +126,12 @@\n #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS\n #endif\n \n+/**\n+ * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n+ * released in August 2022.\n+ */\n+#if LIBCURL_VERSION_NUM >= 0x075500\n+#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n+#endif\n+\n #endif\ndiff --git a/http.c b/http.c\nindex ca0fe80ddb..e529ebc993 100644\n--- a/http.c\n+++ b/http.c\n@@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)\n \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n }\n \n-static long get_curl_allowed_protocols(int from_user)\n+static void proto_list_append(struct strbuf *list_str, const char *proto_str,\n+\t\t\t long *list_bits, long proto_bits)\n+{\n+\t*list_bits |= proto_bits;\n+\tif (list_str) {\n+\t\tif (list_str->len)\n+\t\t\tstrbuf_addch(list_str, ',');\n+\t\tstrbuf_addstr(list_str, proto_str);\n+\t}\n+}\n+\n+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)\n {\n \tlong allowed_protocols = 0;\n \n \tif (is_transport_allowed(\"http\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_HTTP;\n+\t\tproto_list_append(list, \"http\", &allowed_protocols, CURLPROTO_HTTP);\n \tif (is_transport_allowed(\"https\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_HTTPS;\n+\t\tproto_list_append(list, \"https\", &allowed_protocols, CURLPROTO_HTTPS);\n \tif (is_transport_allowed(\"ftp\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_FTP;\n+\t\tproto_list_append(list, \"ftp\", &allowed_protocols, CURLPROTO_FTP);\n \tif (is_transport_allowed(\"ftps\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_FTPS;\n+\t\tproto_list_append(list, \"ftps\", &allowed_protocols, CURLPROTO_FTPS);\n \n \treturn allowed_protocols;\n }\n@@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n+\n+#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n+\t{\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tget_curl_allowed_protocols(0, &buf);\n+\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n+\t\tstrbuf_reset(&buf);\n+\n+\t\tget_curl_allowed_protocols(-1, &buf);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n+#else\n \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(0));\n+\t\t\t get_curl_allowed_protocols(0, NULL));\n \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(-1));\n+\t\t\t get_curl_allowed_protocols(-1, NULL));\n+#endif\n+\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(result);\n-- \n2.39.0.513.g00e40dbe01\n"},{"id":"470394","messageId":"c77892ba-e5cd-1192-ceba-d49edcb95da8@ramsayjones.plus.com","threadId":"59086","inReplyTo":"Y8RduNqadAfaOgs1@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-15T20:54:59Z","receivedAt":"2023-01-15T20:58:08Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/01/2023 20:10, Jeff King wrote:\n> The two options do exactly the same thing, but the latter has been\n> deprecated and in recent versions of curl may produce a compiler\n> warning. Since the UPLOAD form is available everywhere (it was\n> introduced in the year 2000 by curl 7.1), we can just switch to it.\n> \n> Signed-off-by: Jeff King \n> ---\n> http-push.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/http-push.c b/http-push.c\n> index 5f4340a36e..1b18e775d0 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,\n> \t\tconst char *custom_req, struct buffer *buffer,\n> \t\tcurl_write_callback write_fn)\n> {\n> -\tcurl_easy_setopt(curl, CURLOPT_PUT, 1);\n> +\tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1);\n\nMy version of this patch had '1L' rather than just '1' - but it\ndoesn't really matter (and was probably because all the curl\nexamples did so!).\n\nLGTM\n\nATB,\nRamsay Jones\n\n> \tcurl_easy_setopt(curl, CURLOPT_URL, url);\n> \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n> \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n"},{"id":"470397","messageId":"78679697-ceb3-e061-fc6e-d425fdd78577@ramsayjones.plus.com","threadId":"59086","inReplyTo":"Y8Rd0KXYcHKykvjq@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-15T21:11:06Z","receivedAt":"2023-01-15T21:11:19Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/01/2023 20:10, Jeff King wrote:\n> The IOCTLFUNCTION option has been deprecated, and generates a compiler\n> warning in recent versions of curl. We can switch to using SEEKFUNCTION\n> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already\n> indicates we require at least curl 7.19.4.\n\nAh, I didn't even think about this! I knew they were implemented in 7.18.0\nand (as I mentioned previously) used the curl version number to switch\nbetween the two sets of 'options'/implementations. Duh! :(\n\nThis is much better!\n\n> We have to rewrite the ioctl functions into seek functions. In some ways\n> they are simpler (seeking is the only operation), but in some ways more\n> complex (the ioctl allowed only a full rewind, but now we can seek to\n> arbitrary offsets).\n> \n> Curl will only ever use SEEK_SET (per their documentation), so I didn't\n> bother implementing anything else, since it would naturally be\n> completely untested. This seems unlikely to change, but I added an\n> assertion just in case.\n> \n> Likewise, I doubt curl will ever try to seek outside of the buffer sizes\n> we've told it, but I erred on the defensive side here, rather than do an\n> out-of-bounds read.\n> \n> Signed-off-by: Jeff King \n> ---\n> http-push.c | 4 ++--\n> http.c | 20 +++++++++-----------\n> http.h | 2 +-\n> remote-curl.c | 28 +++++++++++++---------------\n> 4 files changed, 25 insertions(+), 29 deletions(-)\n> \n> diff --git a/http-push.c b/http-push.c\n> index 1b18e775d0..7f71316456 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,\n> \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n> \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n> \tcurl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);\n> -\tcurl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);\n> -\tcurl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);\n> +\tcurl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);\n> +\tcurl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);\n> \tcurl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);\n> \tcurl_easy_setopt(curl, CURLOPT_NOBODY, 0);\n> \tcurl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);\n> diff --git a/http.c b/http.c\n> index 8a5ba3f477..ca0fe80ddb 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n> \treturn size / eltsize;\n> }\n> \n> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)\n> +int seek_buffer(void *clientp, curl_off_t offset, int origin)\n> {\n> \tstruct buffer *buffer = clientp;\n> \n> -\tswitch (cmd) {\n> -\tcase CURLIOCMD_NOP:\n> -\t\treturn CURLIOE_OK;\n> -\n> -\tcase CURLIOCMD_RESTARTREAD:\n> -\t\tbuffer->posn = 0;\n> -\t\treturn CURLIOE_OK;\n> -\n> -\tdefault:\n> -\t\treturn CURLIOE_UNKNOWNCMD;\n> +\tif (origin != SEEK_SET)\n> +\t\tBUG(\"seek_buffer only handles SEEK_SET\");\n\nI didn't even think to check this; as you say, the documentation\nclaims only to send SEEK_SET, so ... (but this is obviously a\ngood idea).\n\n> +\tif (offset < 0 || offset >= buffer->buf.len) {\n> +\t\terror(\"curl seek would be outside of buffer\");\n> +\t\treturn CURL_SEEKFUNC_FAIL;\n> \t}\n\nI did at least do this! :)\n\n> +\n> +\tbuffer->posn = offset;\n> +\treturn CURL_SEEKFUNC_OK;\n> }\n> \n> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n> diff --git a/http.h b/http.h\n> index 3c94c47910..77c042706c 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -40,7 +40,7 @@ struct buffer {\n> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n> size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);\n> +int seek_buffer(void *clientp, curl_off_t offset, int origin);\n> \n> /* Slot lifecycle functions */\n> struct active_request_slot *get_active_slot(void);\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 72dfb8fb86..a76b6405eb 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n> \treturn avail;\n> }\n> \n> -static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n> +static int rpc_seek(void *clientp, curl_off_t offset, int origin)\n> {\n> \tstruct rpc_state *rpc = clientp;\n> \n> -\tswitch (cmd) {\n> -\tcase CURLIOCMD_NOP:\n> -\t\treturn CURLIOE_OK;\n> +\tif (origin != SEEK_SET)\n> +\t\tBUG(\"rpc_seek only handles SEEK_SET, not %d\", origin);\n> \n> -\tcase CURLIOCMD_RESTARTREAD:\n> -\t\tif (rpc->initial_buffer) {\n> -\t\t\trpc->pos = 0;\n> -\t\t\treturn CURLIOE_OK;\n> +\tif (rpc->initial_buffer) {\n> +\t\tif (offset < 0 || offset > rpc->len) {\n> +\t\t\terror(\"curl seek would be outside of rpc buffer\");\n> +\t\t\treturn CURL_SEEKFUNC_FAIL;\n> \t\t}\n> -\t\terror(_(\"unable to rewind rpc post data - try increasing http.postBuffer\"));\n> -\t\treturn CURLIOE_FAILRESTART;\n> -\n> -\tdefault:\n> -\t\treturn CURLIOE_UNKNOWNCMD;\n> +\t\trpc->pos = offset;\n> +\t\treturn CURL_SEEKFUNC_OK;\n> \t}\n> +\terror(_(\"unable to rewind rpc post data - try increasing http.postBuffer\"));\n> +\treturn CURL_SEEKFUNC_FAIL;\n> }\n> \n> struct check_pktline_state {\n> @@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n> \t\trpc->initial_buffer = 1;\n> \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n> \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n> -\t\tcurl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);\n> -\t\tcurl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);\n> +\t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);\n> +\t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);\n> \t\tif (options.verbosity > 1) {\n> \t\t\tfprintf(stderr, \"POST %s (chunked)\\n\", rpc->service_name);\n> \t\t\tfflush(stderr);\n\nIt looks so much better without #ifdef's (or having to worry about\nthe git-curl-compat.h header file)!\n\nLGTM\n\nATB,\nRamsay Jones\n\n"},{"id":"470399","messageId":"8f175d26-3d84-3019-031d-e358390f2de4@ramsayjones.plus.com","threadId":"59086","inReplyTo":"Y8ReHbGWetJHQcI1@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-15T21:37:07Z","receivedAt":"2023-01-15T21:37:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/01/2023 20:12, Jeff King wrote:\n> The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was\n> deprecated in curl 7.85.0, and using it generate compiler warnings as of\n> curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we\n> can't just do so unilaterally, as it was only introduced less than a\n> year ago in 7.85.0.\n\nYep, I hadn't quite finished my version of this patch yet, but you\nwould probably not be shocked to learn that I had two separate sets\nof functions #ifdef-ed by curl version number! What you have here\nlooks *much* better!\n\n> \n> Until that version becomes ubiquitous, we have to either disable the\n> deprecation warning or conditionally use the \"STR\" variant on newer\n> versions of libcurl. This patch switches to the new variant, which is\n> nice for two reasons:\n> \n> - we don't have to worry that silencing curl's deprecation warnings\n> might cause us to miss other more useful ones\n> \n> - we'd eventually want to move to the new variant anyway, so this gets\n> us set up (albeit with some extra ugly boilerplate for the\n> conditional)\n> \n> There are a lot of ways to split up the two cases. One way would be to\n> abstract the storage type (strbuf versus a long), how to append\n> (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,\n> and so on. But the resulting code looks pretty magical:\n> \n> GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;\n> if (...http is allowed...)\n> \tGIT_CURL_PROTOCOL_APPEND(&allowed, \"http\", CURLOPT_HTTP);\n> \n> and you end up with more \"#define GIT_CURL_PROTOCOL_TYPE\" macros than\n> actual code.\n> \n> On the other end of the spectrum, we could just implement two separate\n> functions, one that handles a string list and one that handles bits. But\n> then we end up repeating our list of protocols (http, https, ftp, ftp).\n> \n> This patch takes the middle ground. The run-time code is always there to\n> handle both types, and we just choose which one to feed to curl.\n> \n> Signed-off-by: Jeff King \n> ---\n> git-curl-compat.h | 8 ++++++++\n> http.c | 41 ++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 42 insertions(+), 7 deletions(-)\n> \n> diff --git a/git-curl-compat.h b/git-curl-compat.h\n> index 56a83b6bbd..fd96b3cdff 100644\n> --- a/git-curl-compat.h\n> +++ b/git-curl-compat.h\n> @@ -126,4 +126,12 @@\n> #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS\n> #endif\n> \n> +/**\n> + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n> + * released in August 2022.\n> + */\n> +#if LIBCURL_VERSION_NUM >= 0x075500\n> +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n> +#endif\n\nAh, I haven't really grokked what this file is about ... but this\nlooks simple enough. ;)\n\n> +\n> #endif\n> diff --git a/http.c b/http.c\n> index ca0fe80ddb..e529ebc993 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)\n> \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n> }\n> \n> -static long get_curl_allowed_protocols(int from_user)\n> +static void proto_list_append(struct strbuf *list_str, const char *proto_str,\n> +\t\t\t long *list_bits, long proto_bits)\n> +{\n> +\t*list_bits |= proto_bits;\n> +\tif (list_str) {\n> +\t\tif (list_str->len)\n> +\t\t\tstrbuf_addch(list_str, ',');\n> +\t\tstrbuf_addstr(list_str, proto_str);\n> +\t}\n> +}\n> +\n> +static long get_curl_allowed_protocols(int from_user, struct strbuf *list)\n> {\n> \tlong allowed_protocols = 0;\n> \n> \tif (is_transport_allowed(\"http\", from_user))\n> -\t\tallowed_protocols |= CURLPROTO_HTTP;\n> +\t\tproto_list_append(list, \"http\", &allowed_protocols, CURLPROTO_HTTP);\n> \tif (is_transport_allowed(\"https\", from_user))\n> -\t\tallowed_protocols |= CURLPROTO_HTTPS;\n> +\t\tproto_list_append(list, \"https\", &allowed_protocols, CURLPROTO_HTTPS);\n> \tif (is_transport_allowed(\"ftp\", from_user))\n> -\t\tallowed_protocols |= CURLPROTO_FTP;\n> +\t\tproto_list_append(list, \"ftp\", &allowed_protocols, CURLPROTO_FTP);\n> \tif (is_transport_allowed(\"ftps\", from_user))\n> -\t\tallowed_protocols |= CURLPROTO_FTPS;\n> +\t\tproto_list_append(list, \"ftps\", &allowed_protocols, CURLPROTO_FTPS);\n> \n> \treturn allowed_protocols;\n> }\n> @@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)\n> \n> \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n> \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n> +\n> +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n> +\t{\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tget_curl_allowed_protocols(0, &buf);\n> +\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n> +\t\tstrbuf_reset(&buf);\n> +\n> +\t\tget_curl_allowed_protocols(-1, &buf);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);\n> +\t\tstrbuf_release(&buf);\n\nI used two static char arrays to accumulate the strings before\npassing them to curl. I was unsure of the lifetime/ownership\nsemantics - I still haven't got around to looking them up!\n\n> +\t}\n> +#else\n> \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> -\t\t\t get_curl_allowed_protocols(0));\n> +\t\t\t get_curl_allowed_protocols(0, NULL));\n> \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> -\t\t\t get_curl_allowed_protocols(-1));\n> +\t\t\t get_curl_allowed_protocols(-1, NULL));\n> +#endif\n> +\n> \tif (getenv(\"GIT_CURL_VERBOSE\"))\n> \t\thttp_trace_curl_no_data();\n> \tsetup_curl_trace(result);\n\n(another reason for not completing these patches - I don't\nknow what the test coverage is like for these changes; are\nmore tests required? dunno).\n\nFor what it's worth, this LGTM.\n\nThanks!\n\nATB,\nRamsay Jones\n\n"},{"id":"470400","messageId":"xmqqpmbffpgf.fsf@gitster.g","threadId":"59086","inReplyTo":"Y8RdOt02JTvDnLiX@coredump.intra.peff.net","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T21:38:40Z","receivedAt":"2023-01-15T21:38:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do think the IOCTL/SEEK one is old enough that we can do, though. The\n> deprecation is newer, but the SEEK interface was added in an old enough\n> version.\n\nYes, that one is old enough (SEEKFUNCTION and frieds are from 7.18.0\nand 7.19.5, IOCTL was deprecated at 7.18.0, released 15 years ago).\n\nA problematic one is REDIR_PROTOCOLS that was deprecated in 7.85.0\n(Aug 2022) whose replacement appeared in the same 7.85.0 version.\n\nThanks.\n"},{"id":"470401","messageId":"xmqqilh7fp4s.fsf@gitster.g","threadId":"59086","inReplyTo":"Y8Rd0KXYcHKykvjq@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T21:45:39Z","receivedAt":"2023-01-15T21:45:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The IOCTLFUNCTION option has been deprecated, and generates a compiler\n> warning in recent versions of curl. We can switch to using SEEKFUNCTION\n> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already\n> indicates we require at least curl 7.19.4.\n> ...\n> +\tif (offset < 0 || offset >= buffer->buf.len) {\n> +\t\terror(\"curl seek would be outside of buffer\");\n> +\t\treturn CURL_SEEKFUNC_FAIL;\n> \t}\n> +\n> +\tbuffer->posn = offset;\n> +\treturn CURL_SEEKFUNC_OK;\n> }\n\nNow we depend on having at least cURL 7.19.5 because\nCURL_SEEKFUNC_{FAIL,OK} are not available before that version.\n\ncf. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions#L127\n\nWhich is fine, as 7.19.5 is from May 2009 that is old enough. We\njust need to update the place where you got the 7.19.4 above from.\n\nThanks.\n"},{"id":"470406","messageId":"Y8SIf8AyGrNflBA7@coredump.intra.peff.net","threadId":"59086","inReplyTo":"c77892ba-e5cd-1192-ceba-d49edcb95da8@ramsayjones.plus.com","subject":"Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T23:13:03Z","receivedAt":"2023-01-15T23:14:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 15, 2023 at 08:54:59PM +0000, Ramsay Jones wrote:\n\n> > -\tcurl_easy_setopt(curl, CURLOPT_PUT, 1);\n> > +\tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1);\n> \n> My version of this patch had '1L' rather than just '1' - but it\n> doesn't really matter (and was probably because all the curl\n> examples did so!).\n\nYeah, I think it probably ought to be 1L because it's going to a\nvariadic function (and it's been a while, but I think the regular\ninteger promotions make small things into int, but not into long).\n\nSo I don't mind fixing it, but I think we'd want it in a separate patch,\nsince it's orthogonal to what's happening here.\n\n-Peff\n"},{"id":"470407","messageId":"Y8SJoulYtKnfRgrO@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqqilh7fp4s.fsf@gitster.g","subject":"Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T23:17:54Z","receivedAt":"2023-01-15T23:17:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 15, 2023 at 01:45:39PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The IOCTLFUNCTION option has been deprecated, and generates a compiler\n> > warning in recent versions of curl. We can switch to using SEEKFUNCTION\n> > instead. It was added in 2008 via curl 7.18.0; our INSTALL file already\n> > indicates we require at least curl 7.19.4.\n> > ...\n> > +\tif (offset < 0 || offset >= buffer->buf.len) {\n> > +\t\terror(\"curl seek would be outside of buffer\");\n> > +\t\treturn CURL_SEEKFUNC_FAIL;\n> > \t}\n> > +\n> > +\tbuffer->posn = offset;\n> > +\treturn CURL_SEEKFUNC_OK;\n> > }\n> \n> Now we depend on having at least cURL 7.19.5 because\n> CURL_SEEKFUNC_{FAIL,OK} are not available before that version.\n> \n> cf. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions#L127\n\nUgh. I did not even check that, because why would anyone introduce the\nreturn values in a separate version! ;)\n\n> Which is fine, as 7.19.5 is from May 2009 that is old enough. We\n> just need to update the place where you got the 7.19.4 above from.\n\nOK, if we are all right with bumping the version, I can squash that in.\nThe other option is to fall back to the obvious 0/1 for OK/FAIL, which\nis what curl does. But that feels awfully hacky, and it would only be\nused if you were at _exactly_ 7.19.4. So I prefer the bump if it's\nacceptable.\n\ndiff --git a/INSTALL b/INSTALL\nindex 3344788397..d5694f8c47 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -139,7 +139,7 @@ Issues of note:\n \t not need that functionality, use NO_CURL to build without\n \t it.\n \n-\t Git requires version \"7.19.4\" or later of \"libcurl\" to build\n+\t Git requires version \"7.19.5\" or later of \"libcurl\" to build\n \t without NO_CURL. This version requirement may be bumped in\n \t the future.\n \n\n-Peff\n"},{"id":"470408","messageId":"Y8SKxZyh6xn2npbh@coredump.intra.peff.net","threadId":"59086","inReplyTo":"8f175d26-3d84-3019-031d-e358390f2de4@ramsayjones.plus.com","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T23:22:45Z","receivedAt":"2023-01-15T23:22:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 15, 2023 at 09:37:07PM +0000, Ramsay Jones wrote:\n\n> > +/**\n> > + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n> > + * released in August 2022.\n> > + */\n> > +#if LIBCURL_VERSION_NUM >= 0x075500\n> > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n> > +#endif\n> \n> Ah, I haven't really grokked what this file is about ... but this\n> looks simple enough. ;)\n\nIt's newish from the cleanups in e4ff3b67c2 (http: centralize the\naccounting of libcurl dependencies, 2021-09-13). I mostly just\ncargo-culted this part. ;)\n\n> > +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n> > +\t{\n> > +\t\tstruct strbuf buf = STRBUF_INIT;\n> > +\n> > +\t\tget_curl_allowed_protocols(0, &buf);\n> > +\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n> > +\t\tstrbuf_reset(&buf);\n> > +\n> > +\t\tget_curl_allowed_protocols(-1, &buf);\n> > +\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);\n> > +\t\tstrbuf_release(&buf);\n> \n> I used two static char arrays to accumulate the strings before\n> passing them to curl. I was unsure of the lifetime/ownership\n> semantics - I still haven't got around to looking them up!\n\nReally old versions of curl had lifetime issues, but for a long now\n(since before the oldest version we'd support), the rule is generally\nthat curl will copy any opt strings as necessary.\n\nThe allocations do feel heavyweight for setting an option. And I think\nthis get_curl_handle() is really called once per request, so we _could_\nprobably just generate them once and cache the result. But in general\nI've been trying to avoid hidden static variables, etc, as they make\nlater libification efforts harder. And an extra malloc() on top of an\nHTTP request is probably not noticeable.\n\n> > +#else\n> > \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> > -\t\t\t get_curl_allowed_protocols(0));\n> > +\t\t\t get_curl_allowed_protocols(0, NULL));\n> > \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> > -\t\t\t get_curl_allowed_protocols(-1));\n> > +\t\t\t get_curl_allowed_protocols(-1, NULL));\n> > +#endif\n> > +\n> > \tif (getenv(\"GIT_CURL_VERBOSE\"))\n> > \t\thttp_trace_curl_no_data();\n> > \tsetup_curl_trace(result);\n> \n> (another reason for not completing these patches - I don't\n> know what the test coverage is like for these changes; are\n> more tests required? dunno).\n\nI had wondered that, too. ;) It's covered by t5812 (my quick and dirty\ncheck was to just drop these lines and see what broke in the test\nsuite).\n\n-Peff\n"},{"id":"470410","messageId":"Y8SQ9ua7w2MbeU9C@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8SIf8AyGrNflBA7@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-15T23:49:10Z","receivedAt":"2023-01-15T23:49:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 15, 2023 at 06:13:04PM -0500, Jeff King wrote:\n\n> On Sun, Jan 15, 2023 at 08:54:59PM +0000, Ramsay Jones wrote:\n> \n> > > -\tcurl_easy_setopt(curl, CURLOPT_PUT, 1);\n> > > +\tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1);\n> > \n> > My version of this patch had '1L' rather than just '1' - but it\n> > doesn't really matter (and was probably because all the curl\n> > examples did so!).\n> \n> Yeah, I think it probably ought to be 1L because it's going to a\n> variadic function (and it's been a while, but I think the regular\n> integer promotions make small things into int, but not into long).\n> \n> So I don't mind fixing it, but I think we'd want it in a separate patch,\n> since it's orthogonal to what's happening here.\n\nI dug a little bit more and as far as I can tell, yes, \"1\" is\ntechnically violating the standard but will often work depending on the\nABI and endianness of the platform.\n\nThat said, there are other cases besides this one, too, so if we want to\ntackle it, let's make it a separate topic (but I'm happy to leave it if\nnobody's platform is broken).\n\n-Peff\n"},{"id":"470417","messageId":"6d826cd9-e447-8df9-83b9-8deb32d6063d@ramsayjones.plus.com","threadId":"59086","inReplyTo":"Y8LCsxz8gyTCxFUp@coredump.intra.peff.net","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-16T00:39:39Z","receivedAt":"2023-01-16T00:39:50Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 14/01/2023 14:56, Jeff King wrote:\n> On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote:\n> \n>> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be\n>> getting a bunch of errors of the form:\n>>\n>> Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated:\n>> since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR\n>> [-Werror=deprecated-declarations]\n> \n> By the way, it seemed odd to me that this failed in just the linux-musl\n> job, and not elsewhere (nor on my development machine, which has curl\n> 7.87.0). It looks like there's a bad interaction within curl between the\n> typecheck and deprecation macros. Here's a minimal reproduction:\n> \n> -- >8 --\n> cat >foo.c <<-\\EOF\n> #include \n> void foo(CURL *c)\n> {\n> \tcurl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);\n> }\n> EOF\n> \n> # this will complain about deprecated CURLOPT_PROTOCOLS\n> gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c\n> \n> # this will not\n> gcc -Wdeprecated-declarations -c foo.c\n> -- 8< --\n\nFYI, I just tried this on cygwin and both gcc invocations above\ncomplain about deprecated CURLOPT_PROTOCOLS. (On Linux I have\ncurl 7.81.0, so I can't test there).\n\n[cygwin uses newlib, of course].\n\nATB,\nRamsay Jones\n\n"},{"id":"470431","messageId":"230116.86edruzk5m.gmgdl@evledraar.gmail.com","threadId":"59086","inReplyTo":"Y8ReHbGWetJHQcI1@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-16T13:06:50Z","receivedAt":"2023-01-16T13:25:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jan 15 2023, Jeff King wrote:\n\n> The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was\n> deprecated in curl 7.85.0, and using it generate compiler warnings as of\n> curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we\n> can't just do so unilaterally, as it was only introduced less than a\n> year ago in 7.85.0.\n>\n> Until that version becomes ubiquitous, we have to either disable the\n> deprecation warning or conditionally use the \"STR\" variant on newer\n> versions of libcurl. This patch switches to the new variant, which is\n> nice for two reasons:\n>\n> - we don't have to worry that silencing curl's deprecation warnings\n> might cause us to miss other more useful ones\n>\n> - we'd eventually want to move to the new variant anyway, so this gets\n> us set up (albeit with some extra ugly boilerplate for the\n> conditional)\n>\n> There are a lot of ways to split up the two cases. One way would be to\n> abstract the storage type (strbuf versus a long), how to append\n> (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,\n> and so on. But the resulting code looks pretty magical:\n>\n> GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;\n> if (...http is allowed...)\n> \tGIT_CURL_PROTOCOL_APPEND(&allowed, \"http\", CURLOPT_HTTP);\n>\n> and you end up with more \"#define GIT_CURL_PROTOCOL_TYPE\" macros than\n> actual code.\n>\n> On the other end of the spectrum, we could just implement two separate\n> functions, one that handles a string list and one that handles bits. But\n> then we end up repeating our list of protocols (http, https, ftp, ftp).\n>\n> This patch takes the middle ground. The run-time code is always there to\n> handle both types, and we just choose which one to feed to curl.\n>\n> Signed-off-by: Jeff King \n> ---\n> git-curl-compat.h | 8 ++++++++\n> http.c | 41 ++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 42 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-curl-compat.h b/git-curl-compat.h\n> index 56a83b6bbd..fd96b3cdff 100644\n> --- a/git-curl-compat.h\n> +++ b/git-curl-compat.h\n> @@ -126,4 +126,12 @@\n> #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS\n> #endif\n> \n> +/**\n> + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n> + * released in August 2022.\n> + */\n> +#if LIBCURL_VERSION_NUM >= 0x075500\n> +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n> +#endif\n> +\n> #endif\n\nThanks, I hadn't had time to comment on this yet, but was wondering what\nthis had to do with \"CI\" or \"DEVEDEVELOPER_CFLAGS\", the \"CI\" just seems\nto be \"where we happened to spot this\", and as has been noted this is\nalso an issue without DEVELOPER_CFLAGS, we just happen to have -Werror\nthere.\n\nBut this is the right direction, and the reason we have git-curl-compat.h.\n\n> diff --git a/http.c b/http.c\n> index ca0fe80ddb..e529ebc993 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)\n> \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n> }\n> \n> -static long get_curl_allowed_protocols(int from_user)\n> +static void proto_list_append(struct strbuf *list_str, const char *proto_str,\n> +\t\t\t long *list_bits, long proto_bits)\n> +{\n> +\t*list_bits |= proto_bits;\n> +\tif (list_str) {\n> +\t\tif (list_str->len)\n> +\t\t\tstrbuf_addch(list_str, ',');\n> +\t\tstrbuf_addstr(list_str, proto_str);\n> +\t}\n> +}\n\nNit: It would be nice (especially in this even smaller function) to\ncarry forward the name the parent get_curl_allowed_protocols() uses,\ni.e. just \"list\", not \"list_str\", ditto \"proto\" rather than \"proto_str\".\n\n> +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n> +\t{\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tget_curl_allowed_protocols(0, &buf);\n> +\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n> +\t\tstrbuf_reset(&buf);\n> +\n> +\t\tget_curl_allowed_protocols(-1, &buf);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);\n> +\t\tstrbuf_release(&buf);\n> +\t}\n> +#else\n> \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> -\t\t\t get_curl_allowed_protocols(0));\n> +\t\t\t get_curl_allowed_protocols(0, NULL));\n> \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> -\t\t\t get_curl_allowed_protocols(-1));\n> +\t\t\t get_curl_allowed_protocols(-1, NULL));\n> +#endif\n> +\n> \tif (getenv(\"GIT_CURL_VERBOSE\"))\n> \t\thttp_trace_curl_no_data();\n> \tsetup_curl_trace(result);\n\nI wonder if it isn't better to avoid conditionally compiled code here if\nwe can avoid it, i.e. just define GIT_* dummy fallbacks, and only use\nthese if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR is true. I.e. something\nlike the below squashed in.\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex fd96b3cdffd..3bc6e151ca7 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -130,8 +130,13 @@\n * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n * released in August 2022.\n */\n-#if LIBCURL_VERSION_NUM >= 0x075500\n-#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n+#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)\n+#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n+#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR \n+#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR\n+#else\n+#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0\n+#define GIT_CURLOPT_PROTOCOLS_STR 0\n #endif\n \n #endif\ndiff --git a/http.c b/http.c\nindex e529ebc993d..3224e897f02 100644\n--- a/http.c\n+++ b/http.c\n@@ -933,24 +933,22 @@ static CURL *get_curl_handle(void)\n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n \n-#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n-\t{\n+\tif (GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\tget_curl_allowed_protocols(0, &buf);\n-\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n+\t\tcurl_easy_setopt(result, GIT_CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n \t\tstrbuf_reset(&buf);\n \n \t\tget_curl_allowed_protocols(-1, &buf);\n-\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);\n+\t\tcurl_easy_setopt(result, GIT_CURLOPT_PROTOCOLS_STR, buf.buf);\n \t\tstrbuf_release(&buf);\n+\t} else {\n+\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n+\t\t\t\t get_curl_allowed_protocols(0, NULL));\n+\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n+\t\t\t\t get_curl_allowed_protocols(-1, NULL));\n \t}\n-#else\n-\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(0, NULL));\n-\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(-1, NULL));\n-#endif\n \n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\thttp_trace_curl_no_data();\n"},{"id":"470437","messageId":"xmqqzgaicvmj.fsf@gitster.g","threadId":"59086","inReplyTo":"230116.86edruzk5m.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-16T16:05:56Z","receivedAt":"2023-01-16T16:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)\n> +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR \n> +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR\n> +#else\n> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0\n> +#define GIT_CURLOPT_PROTOCOLS_STR 0\n> #endif\n\nI find it a bit ugly that CURLOPT_* being used are all non-zero, but\nit may be true in practice.\n\n> diff --git a/http.c b/http.c\n> index e529ebc993d..3224e897f02 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -933,24 +933,22 @@ static CURL *get_curl_handle(void)\n> \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n> \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n> \n> -#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n> -\t{\n> +\tif (GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR) {\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> ...\n> +\t} else {\n> +\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> +\t\t\t\t get_curl_allowed_protocols(0, NULL));\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> +\t\t\t\t get_curl_allowed_protocols(-1, NULL));\n> \t}\n> -#else\n> -\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> -\t\t\t get_curl_allowed_protocols(0, NULL));\n> -\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> -\t\t\t get_curl_allowed_protocols(-1, NULL));\n> -#endif\n\nI somehow find the above over-engineered solution looking for a\nproblem. Conditional compilation might be ugly, but what is uglier\nis a conditional compilation hidden behind what pretends to be a\nruntime conditional but gets optimized away at compile time. Also,\nwhen the non _STR variant changes status from deprecated to removed,\nthe code will cease to build, so I am not sure if the above is the\nwhole story. You'd also need dummy definitions for them when the\nversion of cURL is advanced enough.\n\nIt is true that with the above we will always pass both sides to the\ncompiler, which may be an upside, but at the same time doesn't it\nmake it harder to notice and remove the support of the older side\nonce the time comes?\n\nThanks.\n"},{"id":"470438","messageId":"xmqqv8l6cuoi.fsf@gitster.g","threadId":"59086","inReplyTo":"xmqqzgaicvmj.fsf@gitster.g","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-16T16:26:21Z","receivedAt":"2023-01-16T16:38:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n>> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR \n>> +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR\n>> +#else\n>> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0\n>> +#define GIT_CURLOPT_PROTOCOLS_STR 0\n>> #endif\n>\n> I find it a bit ugly that CURLOPT_* being used are all non-zero, but\n> it may be true in practice.\n\nSorry for making the first half of the sentence unintelligible. I\nmeant to say that it is ugly that the correctness of the solution\ndepends on the real value for these macros being non-zero.\n\n"},{"id":"470441","messageId":"Y8WFqjvMRLqfFT5G@coredump.intra.peff.net","threadId":"59086","inReplyTo":"6d826cd9-e447-8df9-83b9-8deb32d6063d@ramsayjones.plus.com","subject":"Re: [PATCH] ci: do not die on deprecated-declarations warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-16T17:13:14Z","receivedAt":"2023-01-16T17:37:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 16, 2023 at 12:39:39AM +0000, Ramsay Jones wrote:\n\n> > By the way, it seemed odd to me that this failed in just the linux-musl\n> > job, and not elsewhere (nor on my development machine, which has curl\n> > 7.87.0). It looks like there's a bad interaction within curl between the\n> > typecheck and deprecation macros. Here's a minimal reproduction:\n> > \n> > -- >8 --\n> > cat >foo.c <<-\\EOF\n> > #include \n> > void foo(CURL *c)\n> > {\n> > \tcurl_easy_setopt(c, CURLOPT_PROTOCOLS, 0);\n> > }\n> > EOF\n> > \n> > # this will complain about deprecated CURLOPT_PROTOCOLS\n> > gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c\n> > \n> > # this will not\n> > gcc -Wdeprecated-declarations -c foo.c\n> > -- 8< --\n> \n> FYI, I just tried this on cygwin and both gcc invocations above\n> complain about deprecated CURLOPT_PROTOCOLS. (On Linux I have\n> curl 7.81.0, so I can't test there).\n\nIt did work on Linux for me, of course, using 7.87.0. But curiously this\nmorning it behaved differently!\n\nIn the meantime, Debian's libcurl packaging picked up this upstream\npatch (and I upgraded):\n\n https://github.com/curl/curl/commit/e2aed004302e51cfa5b6ce8c8ab65ef92aa83196\n\nand now the deprecation warning happens consistently. So I think on the\ncurl side there is nothing left to do.\n\n-Peff\n"},{"id":"470443","messageId":"Y8WIEStWY8r8LKAx@coredump.intra.peff.net","threadId":"59086","inReplyTo":"xmqqzgaicvmj.fsf@gitster.g","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-16T17:23:29Z","receivedAt":"2023-01-16T17:45:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 16, 2023 at 08:05:56AM -0800, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)\n> > +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n> > +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR \n> > +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR\n> > +#else\n> > +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0\n> > +#define GIT_CURLOPT_PROTOCOLS_STR 0\n> > #endif\n> \n> I find it a bit ugly that CURLOPT_* being used are all non-zero, but\n> it may be true in practice.\n\nI don't think it matters what the dummy values are, at least from a\nrun-time perspective. We'd only feed them to curl inside the \"if (0)\"\nblock. They just need to be non-empty so the compiler is happy.\n\nBut I do think there's the possibility of compile-time confusion. Curl\nis doing macro magic for deprecation and type-checking here, and it's\nnot clear how it will handle the magic \"0\" values, even if we know\nthe code will never actually run.\n\n> I somehow find the above over-engineered solution looking for a\n> problem. Conditional compilation might be ugly, but what is uglier\n> is a conditional compilation hidden behind what pretends to be a\n> runtime conditional but gets optimized away at compile time. Also,\n> when the non _STR variant changes status from deprecated to removed,\n> the code will cease to build, so I am not sure if the above is the\n> whole story. You'd also need dummy definitions for them when the\n> version of cURL is advanced enough.\n> \n> It is true that with the above we will always pass both sides to the\n> compiler, which may be an upside, but at the same time doesn't it\n> make it harder to notice and remove the support of the older side\n> once the time comes?\n\nI likewise found that solution in an uncanny valley of over-engineering\nfor not much gain.\n\nIf you really want to over-engineer, then something like the patch below\nat least makes the conditional part simpler, because it shares the\ncurl_easy_setopt() calls. You can actually take it even further and\nabstract the declaration of the strbuf completely (and thus omit it when\nit is not going to be used), but it makes the code even more obscure.\n\nI dunno. My original patch tried to err on the side of\nsimple-and-stupid. If we don't mind repeating the list of\nhttp/https/ftp/ftps, then it can be even simpler (and stupider). But the\neventual cleanup becomes really easy, then: just delete the #else half\nof each #ifdef.\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex 56a83b6bbd..e545d26dfa 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -126,4 +126,20 @@\n #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS\n #endif\n \n+/**\n+ * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n+ * released in August 2022.\n+ */\n+#if LIBCURL_VERSION_NUM >= 0x075500\n+#define GIT_CURL_PROTOCOL_TYPE const char *\n+#define GIT_CURL_PROTOCOL_RETURN(list, bits) list->buf\n+#define GIT_CURL_OPT_PROTOCOLS CURLOPT_PROTOCOLS_STR\n+#define GIT_CURL_OPT_REDIR_PROTOCOLS CURLOPT_REDIR_PROTOCOLS_STR\n+#else\n+#define GIT_CURL_PROTOCOL_TYPE long\n+#define GIT_CURL_PROTOCOL_RETURN(list, bits) bits\n+#define GIT_CURL_OPT_PROTOCOLS CURLOPT_PROTOCOLS\n+#define GIT_CURL_OPT_REDIR_PROTOCOLS CURLOPT_REDIR_PROTOCOLS\n+#endif\n+\n #endif\ndiff --git a/http.c b/http.c\nindex ca0fe80ddb..e5ed3521db 100644\n--- a/http.c\n+++ b/http.c\n@@ -764,20 +764,32 @@ void setup_curl_trace(CURL *handle)\n \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n }\n \n-static long get_curl_allowed_protocols(int from_user)\n+static void proto_list_append(struct strbuf *list_str, const char *proto_str,\n+\t\t\t long *list_bits, long proto_bits)\n {\n-\tlong allowed_protocols = 0;\n+\t*list_bits |= proto_bits;\n+\tif (list_str) {\n+\t\tif (list_str->len)\n+\t\t\tstrbuf_addch(list_str, ',');\n+\t\tstrbuf_addstr(list_str, proto_str);\n+\t}\n+}\n+\n+static GIT_CURL_PROTOCOL_TYPE get_curl_allowed_protocols(struct strbuf *list,\n+\t\t\t\t\t\t\t int from_user)\n+{\n+\tlong bits = 0;\n \n \tif (is_transport_allowed(\"http\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_HTTP;\n+\t\tproto_list_append(list, \"http\", &bits, CURLPROTO_HTTP);\n \tif (is_transport_allowed(\"https\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_HTTPS;\n+\t\tproto_list_append(list, \"https\", &bits, CURLPROTO_HTTPS);\n \tif (is_transport_allowed(\"ftp\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_FTP;\n+\t\tproto_list_append(list, \"ftp\", &bits, CURLPROTO_FTP);\n \tif (is_transport_allowed(\"ftps\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_FTPS;\n+\t\tproto_list_append(list, \"ftps\", &bits, CURLPROTO_FTPS);\n \n-\treturn allowed_protocols;\n+\treturn GIT_CURL_PROTOCOL_RETURN(list, bits);\n }\n \n #ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2\n@@ -921,10 +933,17 @@ static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n-\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(0));\n-\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(-1));\n+\n+\t{\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tcurl_easy_setopt(result, GIT_CURL_OPT_REDIR_PROTOCOLS,\n+\t\t\t\t get_curl_allowed_protocols(&buf, 0));\n+\t\tcurl_easy_setopt(result, GIT_CURL_OPT_PROTOCOLS,\n+\t\t\t\t get_curl_allowed_protocols(&buf, -1));\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(result);\n"},{"id":"470444","messageId":"Y8WI+/A7a3BvUIWJ@coredump.intra.peff.net","threadId":"59086","inReplyTo":"230116.86edruzk5m.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-16T17:27:23Z","receivedAt":"2023-01-16T17:48:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 16, 2023 at 02:06:50PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > -static long get_curl_allowed_protocols(int from_user)\n> > +static void proto_list_append(struct strbuf *list_str, const char *proto_str,\n> > +\t\t\t long *list_bits, long proto_bits)\n> > +{\n> > +\t*list_bits |= proto_bits;\n> > +\tif (list_str) {\n> > +\t\tif (list_str->len)\n> > +\t\t\tstrbuf_addch(list_str, ',');\n> > +\t\tstrbuf_addstr(list_str, proto_str);\n> > +\t}\n> > +}\n> \n> Nit: It would be nice (especially in this even smaller function) to\n> carry forward the name the parent get_curl_allowed_protocols() uses,\n> i.e. just \"list\", not \"list_str\", ditto \"proto\" rather than \"proto_str\".\n\nI think it gets confusing in this function, then, because you have both\ntypes. If anything, the sin is in the caller which uses \"list\" and\n\"allowed_protocols\". I had originally written that as \"list\" and \"bits\",\nbut I left \"bits\" as \"allowed_protocols\" to reduce the size of the diff.\nMaybe that was a bad choice.\n\nLikewise, the caller could just do the bitwise-OR inline, like:\n\n if (is_transported_allowed(\"http\", from_user)) {\n\tbits |= CURLPROTO_HTTP;\n\tproto_list_append(list, \"http\");\n }\n\nbut that makes the diff bigger (the whole function body is replaced,\nbecause the \"if\" lines, which now have a \"{\", are no longer unchanged\ncontext). But again, maybe optimizing for a small diff is a bad idea if\nthe resulting code is harder to follow (I didn't think it was, but then\nI also wrote it).\n\n-Peff\n"},{"id":"470469","messageId":"Y8YP+R/hyNr6sEFA@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8RddcM9Vr71ljp4@coredump.intra.peff.net","subject":"[PATCH v2] avoiding deprecated curl options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-17T03:03:21Z","receivedAt":"2023-01-17T03:03:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote:\n\n> So I took a look at just dropping the deprecated bits, and it wasn't\n> _too_ bad. Here's that series. The first two I hope are obviously good.\n> The third one is _ugly_, but at least it punts on the whole \"how should\n> we silence this\" argument, and it takes us in the direction we'd\n> ultimately want to go.\n> \n> [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT\n> [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION\n> [3/3]: http: support CURLOPT_PROTOCOLS_STR\n\nIn the interests of wrapping this up, here's a v2 that:\n\n - bumps the required curl version to 7.19.5 in patch 2\n\n - aims for slightly better readability in the final code of patch 3,\n versus minimizing the diff\n\nAs discussed, there are a lot of different ways one could do patch 3,\nbut I really don't think it's worth spending that much brain power on.\nWhat's here is correct (most important), and I think should be easy to\nclean up when we can eventually drop the old style.\n\n [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT\n [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION\n [3/3]: http: support CURLOPT_PROTOCOLS_STR\n\n INSTALL | 2 +-\n git-curl-compat.h | 8 +++++\n http-push.c | 6 ++--\n http.c | 79 +++++++++++++++++++++++++++++++++--------------\n http.h | 2 +-\n remote-curl.c | 28 ++++++++---------\n 6 files changed, 81 insertions(+), 44 deletions(-)\n\n1: 2229c0468f = 1: 5ae6831af5 http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT\n2: 00120fa40e ! 2: 5be76d74de http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION\n @@ Commit message\n instead. It was added in 2008 via curl 7.18.0; our INSTALL file already\n indicates we require at least curl 7.19.4.\n \n - We have to rewrite the ioctl functions into seek functions. In some ways\n - they are simpler (seeking is the only operation), but in some ways more\n - complex (the ioctl allowed only a full rewind, but now we can seek to\n - arbitrary offsets).\n + But there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL},\n + and those didn't arrive until 7.19.5. One workaround would be to use a\n + bare 0/1 here (or define our own macros). But let's just bump the\n + minimum required version to 7.19.5. That version is only a minor version\n + bump from our existing requirement, and is only a 2 month time bump for\n + versions that are almost 13 years old. So it's not likely that anybody\n + cares about the distinction.\n +\n + Switching means we have to rewrite the ioctl functions into seek\n + functions. In some ways they are simpler (seeking is the only\n + operation), but in some ways more complex (the ioctl allowed only a full\n + rewind, but now we can seek to arbitrary offsets).\n \n Curl will only ever use SEEK_SET (per their documentation), so I didn't\n bother implementing anything else, since it would naturally be\n @@ Commit message\n \n Signed-off-by: Jeff King \n \n + ## INSTALL ##\n +@@ INSTALL: Issues of note:\n + \t not need that functionality, use NO_CURL to build without\n + \t it.\n + \n +-\t Git requires version \"7.19.4\" or later of \"libcurl\" to build\n ++\t Git requires version \"7.19.5\" or later of \"libcurl\" to build\n + \t without NO_CURL. This version requirement may be bumped in\n + \t the future.\n + \n +\n ## http-push.c ##\n @@ http-push.c: static void curl_setup_http(CURL *curl, const char *url,\n \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n3: 22eb2fd0fe ! 3: d2c28e22e1 http: support CURLOPT_PROTOCOLS_STR\n @@ http.c: void setup_curl_trace(CURL *handle)\n }\n \n -static long get_curl_allowed_protocols(int from_user)\n -+static void proto_list_append(struct strbuf *list_str, const char *proto_str,\n -+\t\t\t long *list_bits, long proto_bits)\n -+{\n -+\t*list_bits |= proto_bits;\n -+\tif (list_str) {\n -+\t\tif (list_str->len)\n -+\t\t\tstrbuf_addch(list_str, ',');\n -+\t\tstrbuf_addstr(list_str, proto_str);\n -+\t}\n -+}\n -+\n -+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)\n ++static void proto_list_append(struct strbuf *list, const char *proto)\n {\n - \tlong allowed_protocols = 0;\n +-\tlong allowed_protocols = 0;\n ++\tif (!list)\n ++\t\treturn;\n ++\tif (list->len)\n ++\t\tstrbuf_addch(list, ',');\n ++\tstrbuf_addstr(list, proto);\n ++}\n \n - \tif (is_transport_allowed(\"http\", from_user))\n +-\tif (is_transport_allowed(\"http\", from_user))\n -\t\tallowed_protocols |= CURLPROTO_HTTP;\n -+\t\tproto_list_append(list, \"http\", &allowed_protocols, CURLPROTO_HTTP);\n - \tif (is_transport_allowed(\"https\", from_user))\n +-\tif (is_transport_allowed(\"https\", from_user))\n -\t\tallowed_protocols |= CURLPROTO_HTTPS;\n -+\t\tproto_list_append(list, \"https\", &allowed_protocols, CURLPROTO_HTTPS);\n - \tif (is_transport_allowed(\"ftp\", from_user))\n +-\tif (is_transport_allowed(\"ftp\", from_user))\n -\t\tallowed_protocols |= CURLPROTO_FTP;\n -+\t\tproto_list_append(list, \"ftp\", &allowed_protocols, CURLPROTO_FTP);\n - \tif (is_transport_allowed(\"ftps\", from_user))\n +-\tif (is_transport_allowed(\"ftps\", from_user))\n -\t\tallowed_protocols |= CURLPROTO_FTPS;\n -+\t\tproto_list_append(list, \"ftps\", &allowed_protocols, CURLPROTO_FTPS);\n ++static long get_curl_allowed_protocols(int from_user, struct strbuf *list)\n ++{\n ++\tlong bits = 0;\n \n - \treturn allowed_protocols;\n +-\treturn allowed_protocols;\n ++\tif (is_transport_allowed(\"http\", from_user)) {\n ++\t\tbits |= CURLPROTO_HTTP;\n ++\t\tproto_list_append(list, \"http\");\n ++\t}\n ++\tif (is_transport_allowed(\"https\", from_user)) {\n ++\t\tbits |= CURLPROTO_HTTPS;\n ++\t\tproto_list_append(list, \"https\");\n ++\t}\n ++\tif (is_transport_allowed(\"ftp\", from_user)) {\n ++\t\tbits |= CURLPROTO_FTP;\n ++\t\tproto_list_append(list, \"ftp\");\n ++\t}\n ++\tif (is_transport_allowed(\"ftps\", from_user)) {\n ++\t\tbits |= CURLPROTO_FTPS;\n ++\t\tproto_list_append(list, \"ftps\");\n ++\t}\n ++\n ++\treturn bits;\n }\n + \n + #ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2\n @@ http.c: static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n"},{"id":"470470","messageId":"Y8YQBbjjf+i8BWL6@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8Ljp7yEWTUd92w8@coredump.intra.peff.net","subject":"[PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-17T03:03:33Z","receivedAt":"2023-01-17T03:03:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The two options do exactly the same thing, but the latter has been\ndeprecated and in recent versions of curl may produce a compiler\nwarning. Since the UPLOAD form is available everywhere (it was\nintroduced in the year 2000 by curl 7.1), we can just switch to it.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 5f4340a36e..1b18e775d0 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,\n \t\tconst char *custom_req, struct buffer *buffer,\n \t\tcurl_write_callback write_fn)\n {\n-\tcurl_easy_setopt(curl, CURLOPT_PUT, 1);\n+\tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1);\n \tcurl_easy_setopt(curl, CURLOPT_URL, url);\n \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n-- \n2.39.0.513.g00e40dbe01\n\n"},{"id":"470471","messageId":"Y8YQOADmbWbdKDvX@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8YQBbjjf+i8BWL6@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-17T03:04:24Z","receivedAt":"2023-01-17T03:04:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 16, 2023 at 10:03:33PM -0500, Jeff King wrote:\n\n> The two options do exactly the same thing, but the latter has been\n> deprecated and in recent versions of curl may produce a compiler\n> warning. Since the UPLOAD form is available everywhere (it was\n> introduced in the year 2000 by curl 7.1), we can just switch to it.\n> \n> Signed-off-by: Jeff King \n\nWhoops, this was supposed to be part of v2, but I accidentally sent it\nin reply to the wrong part of the thread. Please ignore, and I'll resend\nin the right spot.\n\n-Peff\n"},{"id":"470472","messageId":"Y8YQRst89JywLbL4@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8YP+R/hyNr6sEFA@coredump.intra.peff.net","subject":"[PATCH v2 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-17T03:04:38Z","receivedAt":"2023-01-17T03:04:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The two options do exactly the same thing, but the latter has been\ndeprecated and in recent versions of curl may produce a compiler\nwarning. Since the UPLOAD form is available everywhere (it was\nintroduced in the year 2000 by curl 7.1), we can just switch to it.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 5f4340a36e..1b18e775d0 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,\n \t\tconst char *custom_req, struct buffer *buffer,\n \t\tcurl_write_callback write_fn)\n {\n-\tcurl_easy_setopt(curl, CURLOPT_PUT, 1);\n+\tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1);\n \tcurl_easy_setopt(curl, CURLOPT_URL, url);\n \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n-- \n2.39.0.513.g00e40dbe01\n\n"},{"id":"470473","messageId":"Y8YQTC1SPM+flPmC@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8YP+R/hyNr6sEFA@coredump.intra.peff.net","subject":"[PATCH v2 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-17T03:04:44Z","receivedAt":"2023-01-17T03:04:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The IOCTLFUNCTION option has been deprecated, and generates a compiler\nwarning in recent versions of curl. We can switch to using SEEKFUNCTION\ninstead. It was added in 2008 via curl 7.18.0; our INSTALL file already\nindicates we require at least curl 7.19.4.\n\nBut there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL},\nand those didn't arrive until 7.19.5. One workaround would be to use a\nbare 0/1 here (or define our own macros). But let's just bump the\nminimum required version to 7.19.5. That version is only a minor version\nbump from our existing requirement, and is only a 2 month time bump for\nversions that are almost 13 years old. So it's not likely that anybody\ncares about the distinction.\n\nSwitching means we have to rewrite the ioctl functions into seek\nfunctions. In some ways they are simpler (seeking is the only\noperation), but in some ways more complex (the ioctl allowed only a full\nrewind, but now we can seek to arbitrary offsets).\n\nCurl will only ever use SEEK_SET (per their documentation), so I didn't\nbother implementing anything else, since it would naturally be\ncompletely untested. This seems unlikely to change, but I added an\nassertion just in case.\n\nLikewise, I doubt curl will ever try to seek outside of the buffer sizes\nwe've told it, but I erred on the defensive side here, rather than do an\nout-of-bounds read.\n\nSigned-off-by: Jeff King \n---\n INSTALL | 2 +-\n http-push.c | 4 ++--\n http.c | 20 +++++++++-----------\n http.h | 2 +-\n remote-curl.c | 28 +++++++++++++---------------\n 5 files changed, 26 insertions(+), 30 deletions(-)\n\ndiff --git a/INSTALL b/INSTALL\nindex 3344788397..d5694f8c47 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -139,7 +139,7 @@ Issues of note:\n \t not need that functionality, use NO_CURL to build without\n \t it.\n \n-\t Git requires version \"7.19.4\" or later of \"libcurl\" to build\n+\t Git requires version \"7.19.5\" or later of \"libcurl\" to build\n \t without NO_CURL. This version requirement may be bumped in\n \t the future.\n \ndiff --git a/http-push.c b/http-push.c\nindex 1b18e775d0..7f71316456 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,\n \tcurl_easy_setopt(curl, CURLOPT_INFILE, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);\n \tcurl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);\n-\tcurl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);\n-\tcurl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);\n+\tcurl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);\n+\tcurl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);\n \tcurl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);\n \tcurl_easy_setopt(curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);\ndiff --git a/http.c b/http.c\nindex 8a5ba3f477..ca0fe80ddb 100644\n--- a/http.c\n+++ b/http.c\n@@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \treturn size / eltsize;\n }\n \n-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)\n+int seek_buffer(void *clientp, curl_off_t offset, int origin)\n {\n \tstruct buffer *buffer = clientp;\n \n-\tswitch (cmd) {\n-\tcase CURLIOCMD_NOP:\n-\t\treturn CURLIOE_OK;\n-\n-\tcase CURLIOCMD_RESTARTREAD:\n-\t\tbuffer->posn = 0;\n-\t\treturn CURLIOE_OK;\n-\n-\tdefault:\n-\t\treturn CURLIOE_UNKNOWNCMD;\n+\tif (origin != SEEK_SET)\n+\t\tBUG(\"seek_buffer only handles SEEK_SET\");\n+\tif (offset < 0 || offset >= buffer->buf.len) {\n+\t\terror(\"curl seek would be outside of buffer\");\n+\t\treturn CURL_SEEKFUNC_FAIL;\n \t}\n+\n+\tbuffer->posn = offset;\n+\treturn CURL_SEEKFUNC_OK;\n }\n \n size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\ndiff --git a/http.h b/http.h\nindex 3c94c47910..77c042706c 100644\n--- a/http.h\n+++ b/http.h\n@@ -40,7 +40,7 @@ struct buffer {\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);\n-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);\n+int seek_buffer(void *clientp, curl_off_t offset, int origin);\n \n /* Slot lifecycle functions */\n struct active_request_slot *get_active_slot(void);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 72dfb8fb86..a76b6405eb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \treturn avail;\n }\n \n-static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n+static int rpc_seek(void *clientp, curl_off_t offset, int origin)\n {\n \tstruct rpc_state *rpc = clientp;\n \n-\tswitch (cmd) {\n-\tcase CURLIOCMD_NOP:\n-\t\treturn CURLIOE_OK;\n+\tif (origin != SEEK_SET)\n+\t\tBUG(\"rpc_seek only handles SEEK_SET, not %d\", origin);\n \n-\tcase CURLIOCMD_RESTARTREAD:\n-\t\tif (rpc->initial_buffer) {\n-\t\t\trpc->pos = 0;\n-\t\t\treturn CURLIOE_OK;\n+\tif (rpc->initial_buffer) {\n+\t\tif (offset < 0 || offset > rpc->len) {\n+\t\t\terror(\"curl seek would be outside of rpc buffer\");\n+\t\t\treturn CURL_SEEKFUNC_FAIL;\n \t\t}\n-\t\terror(_(\"unable to rewind rpc post data - try increasing http.postBuffer\"));\n-\t\treturn CURLIOE_FAILRESTART;\n-\n-\tdefault:\n-\t\treturn CURLIOE_UNKNOWNCMD;\n+\t\trpc->pos = offset;\n+\t\treturn CURL_SEEKFUNC_OK;\n \t}\n+\terror(_(\"unable to rewind rpc post data - try increasing http.postBuffer\"));\n+\treturn CURL_SEEKFUNC_FAIL;\n }\n \n struct check_pktline_state {\n@@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece\n \t\trpc->initial_buffer = 1;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);\n \t\tif (options.verbosity > 1) {\n \t\t\tfprintf(stderr, \"POST %s (chunked)\\n\", rpc->service_name);\n \t\t\tfflush(stderr);\n-- \n2.39.0.513.g00e40dbe01\n\n"},{"id":"470474","messageId":"Y8YQUD8bMHc4Lmph@coredump.intra.peff.net","threadId":"59086","inReplyTo":"Y8YP+R/hyNr6sEFA@coredump.intra.peff.net","subject":"[PATCH v2 3/3] http: support CURLOPT_PROTOCOLS_STR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-17T03:04:48Z","receivedAt":"2023-01-17T03:04:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was\ndeprecated in curl 7.85.0, and using it generate compiler warnings as of\ncurl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we\ncan't just do so unilaterally, as it was only introduced less than a\nyear ago in 7.85.0.\n\nUntil that version becomes ubiquitous, we have to either disable the\ndeprecation warning or conditionally use the \"STR\" variant on newer\nversions of libcurl. This patch switches to the new variant, which is\nnice for two reasons:\n\n - we don't have to worry that silencing curl's deprecation warnings\n might cause us to miss other more useful ones\n\n - we'd eventually want to move to the new variant anyway, so this gets\n us set up (albeit with some extra ugly boilerplate for the\n conditional)\n\nThere are a lot of ways to split up the two cases. One way would be to\nabstract the storage type (strbuf versus a long), how to append\n(strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,\nand so on. But the resulting code looks pretty magical:\n\n GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;\n if (...http is allowed...)\n\tGIT_CURL_PROTOCOL_APPEND(&allowed, \"http\", CURLOPT_HTTP);\n\nand you end up with more \"#define GIT_CURL_PROTOCOL_TYPE\" macros than\nactual code.\n\nOn the other end of the spectrum, we could just implement two separate\nfunctions, one that handles a string list and one that handles bits. But\nthen we end up repeating our list of protocols (http, https, ftp, ftp).\n\nThis patch takes the middle ground. The run-time code is always there to\nhandle both types, and we just choose which one to feed to curl.\n\nSigned-off-by: Jeff King \n---\n git-curl-compat.h | 8 +++++++\n http.c | 59 ++++++++++++++++++++++++++++++++++++-----------\n 2 files changed, 54 insertions(+), 13 deletions(-)\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex 56a83b6bbd..fd96b3cdff 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -126,4 +126,12 @@\n #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS\n #endif\n \n+/**\n+ * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,\n+ * released in August 2022.\n+ */\n+#if LIBCURL_VERSION_NUM >= 0x075500\n+#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n+#endif\n+\n #endif\ndiff --git a/http.c b/http.c\nindex ca0fe80ddb..c4b6ddef28 100644\n--- a/http.c\n+++ b/http.c\n@@ -764,20 +764,37 @@ void setup_curl_trace(CURL *handle)\n \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n }\n \n-static long get_curl_allowed_protocols(int from_user)\n+static void proto_list_append(struct strbuf *list, const char *proto)\n {\n-\tlong allowed_protocols = 0;\n+\tif (!list)\n+\t\treturn;\n+\tif (list->len)\n+\t\tstrbuf_addch(list, ',');\n+\tstrbuf_addstr(list, proto);\n+}\n \n-\tif (is_transport_allowed(\"http\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_HTTP;\n-\tif (is_transport_allowed(\"https\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_HTTPS;\n-\tif (is_transport_allowed(\"ftp\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_FTP;\n-\tif (is_transport_allowed(\"ftps\", from_user))\n-\t\tallowed_protocols |= CURLPROTO_FTPS;\n+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)\n+{\n+\tlong bits = 0;\n \n-\treturn allowed_protocols;\n+\tif (is_transport_allowed(\"http\", from_user)) {\n+\t\tbits |= CURLPROTO_HTTP;\n+\t\tproto_list_append(list, \"http\");\n+\t}\n+\tif (is_transport_allowed(\"https\", from_user)) {\n+\t\tbits |= CURLPROTO_HTTPS;\n+\t\tproto_list_append(list, \"https\");\n+\t}\n+\tif (is_transport_allowed(\"ftp\", from_user)) {\n+\t\tbits |= CURLPROTO_FTP;\n+\t\tproto_list_append(list, \"ftp\");\n+\t}\n+\tif (is_transport_allowed(\"ftps\", from_user)) {\n+\t\tbits |= CURLPROTO_FTPS;\n+\t\tproto_list_append(list, \"ftps\");\n+\t}\n+\n+\treturn bits;\n }\n \n #ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2\n@@ -921,10 +938,26 @@ static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n+\n+#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR\n+\t{\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tget_curl_allowed_protocols(0, &buf);\n+\t\tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);\n+\t\tstrbuf_reset(&buf);\n+\n+\t\tget_curl_allowed_protocols(-1, &buf);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n+#else\n \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(0));\n+\t\t\t get_curl_allowed_protocols(0, NULL));\n \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n-\t\t\t get_curl_allowed_protocols(-1));\n+\t\t\t get_curl_allowed_protocols(-1, NULL));\n+#endif\n+\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n \t\thttp_trace_curl_no_data();\n \tsetup_curl_trace(result);\n-- \n2.39.0.513.g00e40dbe01\n"},{"id":"470566","messageId":"bdd4eacb-4883-be16-e0b6-296a27dcd98f@ramsayjones.plus.com","threadId":"59086","inReplyTo":"Y8YP+R/hyNr6sEFA@coredump.intra.peff.net","subject":"Re: [PATCH v2] avoiding deprecated curl options","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-18T01:03:17Z","receivedAt":"2023-01-18T01:09:11Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/01/2023 03:03, Jeff King wrote:\n> On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote:\n> \n>> So I took a look at just dropping the deprecated bits, and it wasn't\n>> _too_ bad. Here's that series. The first two I hope are obviously good.\n>> The third one is _ugly_, but at least it punts on the whole \"how should\n>> we silence this\" argument, and it takes us in the direction we'd\n>> ultimately want to go.\n>>\n>> [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT\n>> [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION\n>> [3/3]: http: support CURLOPT_PROTOCOLS_STR\n> \n> In the interests of wrapping this up, here's a v2 that:\n> \n> - bumps the required curl version to 7.19.5 in patch 2\n> \n> - aims for slightly better readability in the final code of patch 3,\n> versus minimizing the diff\n> \n\nI have a _slight_ preference for your v1 patches, but I don't hate\nthis version either! :)\n\nTonight, I have compile tested both v1 and v2 patches (both in 'seen')\non cygwin and linux. I would like to say I have run the tests as well,\nbut it seems I have disabled all the tests on cygwin (expected) *and*\non linux (most unexpected).\n\nie. I don't have apache installed. (I used to have apache, svn and cvs\ninstalled to run the tests, but they just took too long to run and\ncaused *many* test runs to hang and leave server processes all over\nthe place. On cygwin, even with all of those tests skipped, it still\ntakes approx 5.5 hours to run the tests).\n\nI thought I was still running the '*http*' tests on linux, but I seem\nto have dropped the installation of apache at some point - oops!\n\nI guess I should install apache tomorrow ...\n\nATB,\nRamsay Jones\n\n"}]}