From: Taylor Blau <me@ttaylorr.com>
Date: Sat, 21 Mar 2026 03:30:33 GMT
Subject: Re: [PATCH v6 3/3] http: add support for HTTP 429 rate limit retries
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:
>  	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.

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

> 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