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

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

>
> > +                             slot->results->retry_after = retry_after;
> > +                     } else {
> > +                             /* Try parsing as HTTP-date format */
> > +                             timestamp_t timestamp;
> > +                             int offset;
> > +                             if (!parse_date_basic(buf.buf, &timestamp, &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.

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

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