Re: [PATCH v6 3/3] http: add support for HTTP 429 rate limit retries
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 21, 2026, 03:30 UTC
- Message-ID
- <ab4Q2XMQIaOYDjPw@nand.local>
- In-Reply-To
- <3418f4553d246c797697c80f439c77fee293f7e0.1773752435.git.gitgitgadget@gmail.com>
On Tue, Mar 17, 2026 at 01:00:35PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:
Show 9 quoted lines
> return size && (*ptr == ' ' || *ptr == '\t');
> }
>
> -static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p UNUSED)
> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p MAYBE_UNUSED)
> {
> size_t size = eltsize * nmemb;
> struct strvec *values = &http_auth.wwwauth_headers;
> @@ -575,6 +582,21 @@ static int http_options(const char *var, const char *value,Good, this version drops the special case where we do not define GIT_CURL_HAVE_CURLINFO_RETRY_AFTER, which Peff suggested in his review of the earlier round.
I agree with his suggestion that we can document that handling Retry-After requires a libcurl newer than 7.66.0, and that is well documented in the user-facing documentation and code comments where appropriate.
Show 6 quoted lines
> @@ -2119,10 +2150,10 @@ static void http_opt_request_remainder(CURL *curl, off_t pos) > > static int http_request(const char *url, > void *result, int target, > - const struct http_get_options *options) > + struct http_get_options *options)
The previous round had this as a const pointer, with a separate out-parameter via 'long *retry_after_out'. Review on the previous round suggested making the retry_after part of the existing out-parameter. Of course, doing so requires that we make that parameter non-const, hence the change here, which looks good to me.
> {
> struct active_request_slot *slot;
> - struct slot_results results;
> + struct slot_results results = { .retry_after = -1 };This also moved from run_one_slot(); this location makes more sense to me.
Show 23 quoted lines
> diff --git a/http.h b/http.h
> index f9d4593404..f9ee888c3e 100644
> --- a/http.h
> +++ b/http.h
> @@ -20,6 +20,7 @@ struct slot_results {
> long http_code;
> long auth_avail;
> long http_connectcode;
> + long retry_after;
> };
>
> struct active_request_slot {
> @@ -157,6 +158,13 @@ struct http_get_options {
> * request has completed.
> */
> struct string_list *extra_headers;
> +
> + /*
> + * After a request completes, contains the Retry-After delay in seconds
> + * if the server returned HTTP 429 with a Retry-After header (requires
> + * libcurl 7.66.0 or later), or -1 if no such header was present.
> + */
> + long retry_after;I think making this a pure long instead of a pointer as is the case with other members of this struct makes sense for the reasons that Peff pointed out in the review of the previous round.
Thanks, Taylor