Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP

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

Sean McAllister <smcallis@google.com> writes:

> +# generate a random 12 digit string
> +gen_nonce() {
> +    test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9
> +}

What is the randomness requirement of this application?  IOW, what
breaks if we just change this to "echo 0123456789ab"?

Or "date | git hash-object --stdin" for that matter?

We'd want to make our tests more predictiable, not less.

> diff --git a/t/lib-httpd/error-ntime.sh b/t/lib-httpd/error-ntime.sh
> new file mode 100755
> index 0000000000..e4f91ab816
> --- /dev/null
> +++ b/t/lib-httpd/error-ntime.sh
> @@ -0,0 +1,79 @@
> +#!/bin/sh
> +
> +# Script to simulate a transient error code with Retry-After header set.
> +#
> +# PATH_INFO must be of the form /<nonce>/<times>/<retcode>/<retry-after>/<path>
> +#   (eg: /dc724af1/3/429/10/some/url)
> +#
> +# The <nonce> value uniquely identifies the URL, since we're simulating
> +# a stateful operation using a stateless protocol, we need a way to "namespace"
> +# URLs so that they don't step on each other.
> +#
> +# The first <times> times this endpoint is called, it will return the given
> +# <retcode>, and if the <retry-after> is non-negative, it will set the
> +# Retry-After head to that value.
> +#
> +# Subsequent calls will return a 302 redirect to <path>.
> +#
> +# Supported error codes are 429,502,503, and 504
> +
> +print_status() {
> +      if [ "$1" -eq "302" ]; then printf "Status: 302 Found\n"
> +    elif [ "$1" -eq "429" ]; then printf "Status: 429 Too Many Requests\n"
> +    elif [ "$1" -eq "502" ]; then printf "Status: 502 Bad Gateway\n"
> +    elif [ "$1" -eq "503" ]; then printf "Status: 503 Service Unavailable\n"
> +    elif [ "$1" -eq "504" ]; then printf "Status: 504 Gateway Timeout\n"
> +    else
> +        printf "Status: 500 Internal Server Error\n"
> +    fi
> +    printf "Content-Type: text/plain\n"

Style????? (I won't repeat this comment for the rest of this script)

I briefly wondered "oh, are t/lib-httpd/* scripts excempt from the
coding guidelines?" but a quick look at them tells me that that is
not the case.

> +}
> +
> +# read in path split into cmoponents
> +IFS='/'
> +tokens=($PATH_INFO)
> +
> +# break out code/retry parts of path
> +nonce=${tokens[1]}
> +times=${tokens[2]}
> +code=${tokens[3]}
> +retry=${tokens[4]}

You said /bin/sh upfront.  Don't use non-POSIX shell arrays.

> +
> +# get redirect path
> +cnt=0
> +path=""
> +for ((ii=0; ii < ${#PATH_INFO}; ii++)); do
> +    if [ "${PATH_INFO:${ii}:1}" == "/" ]; then
> +        let cnt=${cnt}+1
> +    fi
> +    if [ "${cnt}" -eq 5 ]; then
> +        path="${PATH_INFO:${ii}}"
> +        break
> +    fi
> +done
> +
> +# leave a cookie for this request/retry count
> +state_file="request_${REMOTE_ADDR}_${nonce}_${times}_${code}_${retry}"
> +
> +if [ ! -f "$state_file" ]; then
> +    echo 0 > "$state_file"
> +fi
> +
> +
> +read cnt < "$state_file"
> +if [ "$cnt" -lt "$times" ]; then
> +    let cnt=cnt+1
> +    echo "$cnt" > "$state_file"
> +
> +    # return error
> +    print_status "$code"
> +    if [ "$retry" -ge "0" ]; then
> +        printf "Retry-After: %s\n" "$retry"
> +    fi
> +else
> +    # redirect
> +    print_status 302
> +    printf "Location: %s?%s\n" "$path" "${QUERY_STRING}"
> +fi
> +
> +echo
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 7df3c5373a..71d4307001 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -756,6 +756,15 @@ test_expect_success 'partial clone using HTTP' '
>  	partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
>  '
>  
> +test_expect_success 'partial clone using HTTP with redirect' '
> +    _NONCE=`gen_nonce` && export _NONCE &&
> +    curl "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server" > /dev/null &&
> +    curl "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server" > /dev/null &&
> +    curl "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server" > /dev/null &&

These lines are not indented with HT?

Don't redirect test output to /dev/null, which is done by test_expect_success
for us.  >/dev/null makes it less useful to run the test under "-v" option.

> +	partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server"
> +'
> +
> +
>  # DO NOT add non-httpd-specific tests here, because the last part of this
>  # test script is only executed when httpd is available and enabled.