Re: [PATCH 3/4] http: add http.retryStatusCodes config option

[Junio C Hamano <gitster@pobox.com>]

"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  http.retryAfter::
>  	Initial wait time in seconds before retrying when a server returns
> -	HTTP 429 (Too Many Requests) without a Retry-After header.
> +	a retryable HTTP status code (HTTP 429 by default) without a
> +	Retry-After header.

This, and ...

>  http.maxRetries::
> -	Maximum number of times to retry after receiving HTTP 429 (Too Many
> -	Requests) responses. Set to 0 (the default) to disable retries.
> +	Maximum number of times to retry after receiving a retryable HTTP
> +	status code (HTTP 429 by default). Set to 0 (the default) to disable
> +	retries.

this, and ...

>  	Can be overridden by the `GIT_HTTP_MAX_RETRIES` environment variable.
> -	See also `http.retryAfter` and `http.maxRetryTime`.
> +	See also `http.retryAfter`, `http.maxRetryTime`, and
> +	`http.retryStatusCodes`.
>  
>  http.maxRetryTime::
>  	Maximum time in seconds to wait for a single retry attempt when
> -	handling HTTP 429 (Too Many Requests) responses. If the server
> -	requests a delay (via Retry-After header) or if `http.retryAfter`
> +	handling a retryable HTTP status code (HTTP 429 by default). If the
> +	server requests a delay (via Retry-After header) or if `http.retryAfter`

this all share two issues.

 - "Too Many Requests", which is a helpful comment to highlight what
   429 is, is lost, and

 - What we want to say is that there is a customizable set of HTTP
   response codes that we treat as "the other side invites us to
   retry if we want to", and that HTTP 429 is the ONLY member of
   that set by default.  But "HTTP 429 by default" alone in the
   above description does not quite say so.

It is unlikely that the reader realizes that we are talking about
the default membership in a set until ...

> +http.retryStatusCodes::
> +	Comma- or whitespace-separated list of HTTP status codes (in the
> +	400..599 range) that should be retried up to `http.maxRetries`
> +	times. Defaults to `429`. For example, setting this to
> +	`429,502,503,504` also retries transient `5xx` gateway and
> +	service-unavailable errors. Can be overridden by the
> +	`GIT_HTTP_RETRY_STATUS_CODES` environment variable.
> +	See also `http.maxRetries`, `http.retryAfter`, and
> +	`http.maxRetryTime`.

... they read this, which is very clearly written.

> diff --git a/http.c b/http.c
> index 0dec6377dd..950e2bbffa 100644
> --- a/http.c
> +++ b/http.c
> @@ -156,6 +156,7 @@ static int http_schannel_check_revoke = 1;
>  static long http_retry_after = 0;
>  static long http_max_retries = 0;
>  static long http_max_retry_time = 300;
> +static char *http_retry_status_codes;
>  
>  /*
>   * With the backend being set to `schannel`, setting sslCAinfo would override
> @@ -607,6 +608,11 @@ static int http_options(const char *var, const char *value,
>  		return 0;
>  	}
>  
> +	if (!strcmp("http.retrystatuscodes", var)) {
> +		FREE_AND_NULL(http_retry_status_codes);
> +		return git_config_string(&http_retry_status_codes, var, value);
> +	}

OK, the input is a simple string.

> +static int is_retryable_http_code(long code)
> +{
> +	const char *p = http_retry_status_codes ? http_retry_status_codes : "429";
> +	char *end;
> +
> +	for (; code >= 400 && code < 600 && *p; p = end) {
> +		while (*p == ',' || isspace(*p))
> +			p++;
> +		if (strtol(p, &end, 10) == code && end > p &&
> +		    (!*end || *end == ',' || isspace(*end)))
> +			return 1;
> +		while (*end && *end != ',' && !isspace(*end))
> +			end++;
> +	}
> +	return 0;
> +}

Ahh, we parse the string every time?

I guess it is not performance-critical (after all, we are talking
about waiting and then retrying here), but I somehow imagined we
would allocate an array of ints and pre-parse the string upon
reading the configuration variable.

Also 400 <= code < 600 is a constant throughout the function and it
is a tad strange to see it in the loop control part of a for()
statement.

In any case, the strtol() based parsing looks cleanly done.

Thanks.