steadmon@xxxxxxxxxx writes:
> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).
>
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.
>
> Signed-off-by: Josh Steadmon <steadmon@xxxxxxxxxx>
> ---
> remote-curl.c | 4 +++-
> t/lib-httpd.sh | 1 +
> t/lib-httpd/apache.conf | 4 ++++
> t/lib-httpd/error-smart-http.sh | 3 +++
> t/t5551-http-fetch-smart.sh | 5 +++++
> 5 files changed, 16 insertions(+), 1 deletion(-)
> create mode 100644 t/lib-httpd/error-smart-http.sh
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..bb3a86505e 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
> } else if (maybe_smart &&
> last->len > 5 && starts_with(last->buf + 4, "version 2")) {
> last->proto_git = 1;
> - }
> + } else if (maybe_smart && last->len > 5 &&
> + starts_with(last->buf + 4, "ERR "))
> + die(_("remote error: %s"), last->buf + 8);
Two observations and a half.
- All of these if/else if/ blocks (currently 2, now you added the
third one) are to special case the "maybe_smart" case. Perhaps
the whole thing should be restrucutred to
if (maybe_smart) {
if ...
else if ...
else if ...
}
- All conditions skip the first four bytes in last->buf and does
starts_with (the first branch is "starts_with("#") in disguise).
Can we do something to the hardcoded magic number 4, which looks
somewhat irritating?
- This is less of the problem with the suggested change in the
first bullet item above, but the current code structure makes
readers wonder if maybe_start that starts as 1 is turned off
somewhere in the if/else if/ cascade when we find out that we are
not dealing with smart HTTP after all. That however is not what
is happening. The "maybe" variable is primarily there to avoid
repeating the logic that determined its initial value in the
early part of the function. The last->proto_git field is used to
record if we are using smart HTTP or not.
Otherwise the change looks sensible.
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index a8729f8232..4e946623bb 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -131,6 +131,7 @@ prepare_httpd() {
> mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
> cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
> install_script broken-smart-http.sh
> + install_script error-smart-http.sh
> install_script error.sh
> install_script apply-one-time-sed.sh
>
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 581c010d8f..6de2bc775c 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/
> </LocationMatch>
> ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
> ScriptAlias /broken_smart/ broken-smart-http.sh/
> +ScriptAlias /error_smart/ error-smart-http.sh/
> ScriptAlias /error/ error.sh/
> ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
> <Directory ${GIT_EXEC_PATH}>
> @@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
> <Files broken-smart-http.sh>
> Options ExecCGI
> </Files>
> +<Files error-smart-http.sh>
> + Options ExecCGI
> +</Files>
> <Files error.sh>
> Options ExecCGI
> </Files>
> diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh
> new file mode 100644
> index 0000000000..0a1af318f7
> --- /dev/null
> +++ b/t/lib-httpd/error-smart-http.sh
> @@ -0,0 +1,3 @@
> +printf "Content-Type: text/%s\n" "html"
> +echo
> +printf "%s" "0019ERR server-side error"
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 8630b0cc39..ba83e567e5 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '
> ! grep "=> Send data" err
> '
>
> +test_expect_success 'server-side error detected' '
> + test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&
> + grep "server-side error" actual
> +'
> +
> stop_httpd
> test_done
