[PATCH v4 0/5] http: add support for HTTP 429 rate limit retries

Changes since v3:

 * Clean up of all strbuf_attach() call sites

 * Add strbuf_attach() contract enforcement via BUG()

Changes since v2:

 * New preparatory patch: Introduced show_http_message_fatal() helper
   function to reduce code duplication in remote-curl.c (suggested by Taylor
   Blau)

 * Removed specific HTTP_RATE_LIMITED error handling from http-push.c and
   http-walker.c for the obsolete "dumb" protocol, allowing generic error
   handling to take over (suggested by Jeff King)

 * Added support for CURLINFO_RETRY_AFTER on curl >= 7.66.0, falling back to
   manual header parsing on older versions

 * Simplified retry/delay architecture: replaced complex non-blocking
   "delayed slot" mechanism with simple blocking sleep() call in the retry
   loop, removing ~66 lines of timing logic (suggested by Jeff King)

 * Fixed Retry-After: 0 handling to allow immediate retry as specified by
   RFC 9110

 * Changed http.retryAfter default from -1 to 0, so Git will retry
   immediately when encountering HTTP 429 without a Retry-After header,
   rather than failing with a configuration error

 * Improved error messages: shortened to be more concise

 * Fixed coding style issues: removed unnecessary curly braces, changed x ==
   0 to !x (per CodingGuidelines)

 * Improved test portability: replaced non-portable date(1) commands with
   test-tool date, added nanosecond-precision timing with getnanos, replaced
   cut(1) with POSIX shell parameter expansion

 * Split out strbuf.c bugfix into separate preparatory patch (the
   strbuf_reencode alloc size fix is unrelated to HTTP 429 support)

 * Squashed separate trace2 logging patch into main HTTP 429 retry support
   commit

 * Kept header_is_last_match assignment for Retry-After to prevent incorrect
   handling of HTTP header continuation lines

The implementation includes:

 1. A bug fix in strbuf_reencode() that corrects the allocation size passed
    to strbuf_attach(), passing len+1 instead of len so that the existing
    buffer is reused rather than immediately reallocated.

 2. A cleanup of all strbuf_attach() call sites that were passing alloc ==
    len, leaving no room for the NUL terminator. Sites with a
    known-NUL-terminated buffer now pass len+1; sites where the source
    buffer has no trailing NUL (ll_merge output) are converted to use
    strbuf_add() instead.

 3. A hardening of strbuf_attach() itself: the internal strbuf_grow() that
    silently papered over incorrect alloc values is replaced with an
    explicit BUG() check, enforcing the documented contract that alloc must
    be greater than len.

 4. A new show_http_message_fatal() helper in remote-curl.c that combines
    the repeated pattern of show_http_message() followed by die() into a
    single NORETURN function, reducing boilerplate at existing call sites
    and providing a clean hook for the retry logic.

 5. The main feature: HTTP 429 retry logic with support for the Retry-After
    header (both delay-seconds and HTTP-date formats), configurable via
    http.maxRetries, http.retryAfter, and http.maxRetryTime options. If any
    computed delay exceeds maxRetryTime the request fails immediately with a
    clear diagnostic rather than capping and retrying silently.

Vaidas Pilkauskas (5):
  strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
  strbuf_attach: fix all call sites to pass correct alloc
  strbuf: replace strbuf_grow() in strbuf_attach() with BUG() check
  remote-curl: introduce show_http_message_fatal() helper
  http: add support for HTTP 429 rate limit retries

 Documentation/config/http.adoc |  23 +++
 apply.c                        |   3 +-
 builtin/am.c                   |   2 +-
 builtin/fast-import.c          |   2 +-
 git-curl-compat.h              |   8 +
 http.c                         | 190 +++++++++++++++++++++--
 http.h                         |   2 +
 mailinfo.c                     |   2 +-
 refs/files-backend.c           |   2 +-
 remote-curl.c                  |  49 +++---
 rerere.c                       |   3 +-
 strbuf.c                       |   5 +-
 t/lib-httpd.sh                 |   1 +
 t/lib-httpd/apache.conf        |   8 +
 t/lib-httpd/http-429.sh        |  98 ++++++++++++
 t/meson.build                  |   1 +
 t/t5584-http-429-retry.sh      | 266 +++++++++++++++++++++++++++++++++
 trailer.c                      |   2 +-
 18 files changed, 629 insertions(+), 38 deletions(-)
 create mode 100644 t/lib-httpd/http-429.sh
 create mode 100755 t/t5584-http-429-retry.sh


base-commit: 73fd77805fc6406f31c36212846d9e2541d19321
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2008%2Fvaidas-shopify%2Fretry-after-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2008/vaidas-shopify/retry-after-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2008

Range-diff vs v3:

 1:  821043c664 ! 1:  a3386f5b56 strbuf: fix incorrect alloc size in strbuf_reencode()
     @@ Metadata
      Author: Vaidas Pilkauskas [off-list ref]
      
       ## Commit message ##
     -    strbuf: fix incorrect alloc size in strbuf_reencode()
     +    strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
      
     -    The strbuf_reencode() function incorrectly passes the string length
     -    as the allocation size to strbuf_attach(), when it should pass
     -    length + 1 to account for the null terminator.
     +    reencode_string_len() allocates len+1 bytes (including the NUL) and
     +    returns the string length in len. strbuf_reencode() was calling
     +    strbuf_attach(sb, out, len, len), so alloc was one byte too small.
      
     -    The reencode_string_len() function allocates len + 1 bytes (including
     -    the null terminator) and returns the string length (excluding the null
     -    terminator) via the len parameter. However, strbuf_reencode() then
     -    calls strbuf_attach() with this length value as both the len and alloc
     -    parameters:
     -
     -        strbuf_attach(sb, out, len, len);
     -
     -    This is incorrect because strbuf_attach()'s alloc parameter should
     -    reflect the actual allocated buffer size, which includes space for the
     -    null terminator. This could lead to incorrect memory management in code
     -    that relies on sb->alloc being accurate.
     -
     -    Fix by passing len + 1 as the alloc parameter:
     -
     -        strbuf_attach(sb, out, len, len + 1);
     +    strbuf_attach() then calls strbuf_grow(sb, 0). With alloc < len+1,
     +    ALLOC_GROW always reallocates, so we reallocated immediately after
     +    attach even when the strbuf was not extended further. Pass len+1 as
     +    the alloc argument so the existing buffer is reused and the
     +    reallocation is avoided.
      
          Signed-off-by: Vaidas Pilkauskas [off-list ref]
      
 -:  ---------- > 2:  f48b1f07c4 strbuf_attach: fix all call sites to pass correct alloc
 -:  ---------- > 3:  557fd77444 strbuf: replace strbuf_grow() in strbuf_attach() with BUG() check
 2:  3653067f0e = 4:  3a39dc9e39 remote-curl: introduce show_http_message_fatal() helper
 3:  3cece62a63 = 5:  5e0f4a56ef http: add support for HTTP 429 rate limit retries

-- 
gitgitgadget