git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: steadmon@google.com
Cc: git@vger.kernel.org
Subject: Re: [PATCH] remote-curl: die on server-side errors
Date: Tue, 13 Nov 2018 11:52:12 +0900	[thread overview]
Message-ID: <xmqqbm6ty9ib.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com> (steadmon's message of "Mon, 12 Nov 2018 14:44:56 -0800")

steadmon@google.com 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@google.com>
> ---
>  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

  parent reply	other threads:[~2018-11-13  2:52 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-12 22:44 [PATCH] remote-curl: die on server-side errors steadmon
2018-11-12 22:55 ` Stefan Beller
2018-11-13  2:52 ` Junio C Hamano [this message]
2018-11-13  3:02 ` Junio C Hamano
2018-11-13 22:15   ` Josh Steadmon
2018-11-13 14:26 ` Jeff King
2018-11-13 22:25   ` Josh Steadmon
2018-11-14  0:49     ` Jeff King
2018-11-14  7:00       ` Jeff King
2018-11-15 21:51         ` Josh Steadmon
2018-11-16  8:44           ` [PATCH 0/3] remote-curl smart-http discovery cleanup Jeff King
2018-11-16  8:47             ` [PATCH 1/3] remote-curl: refactor smart-http discovery Jeff King
2018-11-16 20:27               ` Josh Steadmon
2019-02-05 23:29               ` Junio C Hamano
2019-02-06 19:16                 ` Jeff King
2019-02-06 19:18                   ` Jeff King
2019-02-06 19:29                     ` Josh Steadmon
2019-02-06 20:42                       ` Junio C Hamano
2019-02-06 21:14                       ` Jeff King
2019-02-06 19:18                   ` [PATCH 2/3] remote-curl: tighten "version 2" check for smart-http Jeff King
2019-02-06 19:19                   ` [PATCH 3/3] t5551: test server-side ERR packet Jeff King
2019-02-06 22:19                   ` [PATCH 1/3] remote-curl: refactor smart-http discovery Junio C Hamano
2018-11-16  8:48             ` [PATCH 2/3] remote-curl: tighten "version 2" check for smart-http Jeff King
2018-11-16 20:28               ` Josh Steadmon
2018-11-16  8:49             ` [PATCH 3/3] remote-curl: die on server-side errors Jeff King
2018-11-16 20:04             ` [PATCH 0/3] remote-curl smart-http discovery cleanup Josh Steadmon
2018-12-12  0:25             ` [PATCH v3 0/4] Unify pkt-line error handling and refactor smart-http Josh Steadmon
2018-12-12  0:25               ` [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context Josh Steadmon
2018-12-12 11:02                 ` Jeff King
2018-12-13  1:17                   ` Masaya Suzuki
2018-12-13  8:04                     ` Jeff King
2018-12-13 22:18                     ` Josh Steadmon
2018-12-17 21:33                       ` Jeff King
2018-12-19 23:30                         ` Josh Steadmon
2018-12-20 15:49                           ` Jeff King
2018-12-12  0:25               ` [PATCH v3 2/4] remote-curl: refactor smart-http discovery Josh Steadmon
2018-12-12  0:25               ` [PATCH v3 3/4] remote-curl: tighten "version 2" check for smart-http Josh Steadmon
2018-12-12  0:25               ` [PATCH v3 4/4] lib-httpd, t5551: check server-side HTTP errors Josh Steadmon
2018-12-12  8:43               ` [PATCH v3 0/4] Unify pkt-line error handling and refactor smart-http Junio C Hamano
2018-11-13 14:30 ` [PATCH] remote-curl: die on server-side errors Junio C Hamano
2018-11-13 22:28   ` Josh Steadmon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqbm6ty9ib.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).