From: Vaidas Pilkauskas Date: Fri, 13 Feb 2026 13:30:18 GMT Subject: Re: [PATCH v2 1/2] http: add support for HTTP 429 rate limit retries Message-ID: In-Reply-To: On Wed, Feb 11, 2026 at 3:05 AM Taylor Blau wrote: > > +http.retryAfter:: > > + Default wait time in seconds before retrying when a server returns > > + HTTP 429 (Too Many Requests) without a Retry-After header. If set > > + to -1 (the default), Git will fail immediately when encountering > > While reviewing, I originally wrote: > > Setting the default as "-1" makes sense to me. The current behavior is > to give up when we receive a HTTP 429 response with or without a > Retry-After header, so retaining that behavior makes sense and seems > like a sensible path. > > , but I'm not sure that I am sold on that line of thinking. This is > controlling how long we'll wait after a 429 response before retrying, > not how many times we'll retry (which is `http.maxRetries` below). > > Should the default here be zero? We would "retry" immediately, but that > retry would fail since the maximum retries is set to "zero" by default. I think the only reason I was using "-1" is to have an opportunity to advise on existing configuration for retries, but I guess we can live without advising as I expect folks who are willing to configure retry handling will be advanced users who are aware of the options. I'll switch to "0". > > diff --git a/http-push.c b/http-push.c > > index 60a9b75620..ddb9948352 100644 > > --- a/http-push.c > > +++ b/http-push.c > > @@ -716,6 +716,10 @@ static int fetch_indices(void) > > + case HTTP_RATE_LIMITED: > > + error(_("rate limited by '%s', please try again later"), url); > > + ret = -1; > > + break; > > I wonder if there is an opportunity to DRY this up a bit? I think the > case in fetch_indices() is very similar to remote_Exists(), and ditto > for fetch_indices() in the http-walker.c code. I'll leave this code unchanged as per Peff's suggestion. > > > + slot->results->retry_after = retry_after; > > + } else { > > + /* Try parsing as HTTP-date format */ > > + timestamp_t timestamp; > > + int offset; > > + if (!parse_date_basic(buf.buf, ×tamp, &offset)) { > > + /* Successfully parsed as date, calculate delay from now */ > > + timestamp_t now = time(NULL); > > + if (timestamp > now) { > > + slot->results->retry_after = (long)(timestamp - now); > > + } else { > > + /* Past date means retry immediately */ > > + slot->results->retry_after = 0; > > + } > > + } else { > > + /* Failed to parse as either delay-seconds or HTTP-date */ > > + warning(_("unable to parse Retry-After header value: '%s'"), buf.buf); > > + } > > + } > > + } > > + > > + http_auth.header_is_last_match = 1; > > Could you help me understand why we're setting header_is_last_match > here? I think since we immediately "goto exit" this line isn't strictly > necessary. Yes, this should not be needed - I'll remove the statement. > As a separate but related note, I don't know if this function properly > handles header continuations for Retry-After headers, but in practice I > suspect it doesn't matter, as servers should not be continuing > Retry-After headers across multiple lines. Yes, I assume it's not applicable to Retry-After, so I'm not handling continuations. > > @@ -1660,44 +1729,98 @@ void run_active_slot(struct active_request_slot *slot) > I wonder if run_active_slot() is the right place for these changes or if > it should be handled separately. I think it may be somewhat surprising > for run_active_slot() to return without actually running the slot, even > if the slot is marked as "active" but just waiting for a delay. > > OTOH, like I mentioned earlier, I am far from an expert in this part of > the code, so perhaps this is totally OK. shortlog says that Peff (CC'd) > is among the most active contributors to this file in the past year, so > I'll be curious what he thinks as well. I'll follow Peff's review for this part. > > diff --git a/strbuf.c b/strbuf.c > > index 6c3851a7f8..1d3860869e 100644 > > --- a/strbuf.c > > +++ b/strbuf.c > > @@ -168,7 +168,7 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to) > > if (!out) > > return -1; > > > > - strbuf_attach(sb, out, len, len); > > + strbuf_attach(sb, out, len, len + 1); Sorry, I totally forgot about this change. I still got leak reported from CI, so I narrowed it down to this line. I'll make a separate commit to discuss it. > Not sure that I'm following this change. > Thanks, > Taylor Thanks, Taylor, for the review!