Re: [PATCH v5 05/10] test-http-server: add HTTP error response function

[Matthew John Cheetham <mjcheetham@outlook.com>]

On 2023-01-12 12:35, Victoria Dye wrote:

> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Introduce a function to the test-http-server test helper to write more
>> full and valid HTTP error responses, including all the standard response
>> headers like `Server` and `Date`.
> 
> It took me a second to figure out, but this patch combines the content of
> patches 4, 5, and 6 from the last iteration. After squashing those three
> patches from v4 together locally, the range-diff is actually pretty simple
> (see below). 
> 
> Out of curiosity, why did you combine those patches? I don't feel strongly
> about changing it, but the smaller, incremental patches in the previous
> version were a bit easier to review.

This is my mistake. I didn't intend to do this, and agree splitting them is
easier to grok. I will restore this! My apologies! :-(

> In any case, this version addresses my feedback from [1], [2], and [3] - the
> explanatory comments are particularly helpful. Thanks!
> 
> [1] https://lore.kernel.org/git/7b7d1059-cecf-744d-6927-b41963b9e5a8@github.com/
> [2] https://lore.kernel.org/git/e957d4f4-fa94-7a68-f378-38e6ed131244@github.com/
> [3] https://lore.kernel.org/git/f99c381c-1d30-7c95-6158-cecd5321dafd@github.com/
> 
> Range diff v4 (patches 4-6, squashed) vs. v5 (this patch)
> 
> 4:  127827637e !  5:  6f66bf146b test-http-server: add HTTP error response function
>     @@ Commit message
>      
>       ## t/helper/test-http-server.c ##
>      @@ t/helper/test-http-server.c: enum worker_result {
>     - 	WR_STOP_THE_MUSIC = (WR_IO_ERROR | WR_HANGUP),
>     + 	WR_HANGUP   = 1<<1,
>       };
>       
>      +/*
>     @@ t/helper/test-http-server.c: enum worker_result {
>      +		hp = strbuf_detach(&h, NULL);
>      +		string_list_append(&req->header_list, hp);
>      +
>     -+		/* store common request headers separately */
>     ++		/* also store common request headers as struct req members */
>      +		if (skip_prefix(hp, "Content-Type: ", &hv)) {
>      +			req->content_type = hv;
>      +		} else if (skip_prefix(hp, "Content-Length: ", &hv)) {
>     @@ t/helper/test-http-server.c: enum worker_result {
>      +
>      +	if (!initialized) {
>      +		smart_http_regex = xmalloc(sizeof(*smart_http_regex));
>     ++		/*
>     ++		 * This regular expression matches all dumb and smart HTTP
>     ++		 * requests that are currently in use, and defined in
>     ++		 * Documentation/gitprotocol-http.txt.
>     ++		 *
>     ++		 */
>      +		if (regcomp(smart_http_regex, "^/(HEAD|info/refs|"
>      +			    "objects/info/[^/]+|git-(upload|receive)-pack)$",
>      +			    REG_EXTENDED)) {
>     @@ t/helper/test-http-server.c: enum worker_result {
>      +		!regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0);
>      +}
>      +
>     -+static enum worker_result do__git(struct req *req, const char *user)
>     ++static enum worker_result do__git(struct req *req)
>      +{
>      +	const char *ok = "HTTP/1.1 200 OK\r\n";
>      +	struct child_process cp = CHILD_PROCESS_INIT;
>      +	int res;
>      +
>     -+	if (write(1, ok, strlen(ok)) < 0)
>     ++	/*
>     ++	 * Note that we always respond with a 200 OK response even if the
>     ++	 * http-backend process exits with an error. This helper is intended
>     ++	 * only to be used to exercise the HTTP auth handling in the Git client,
>     ++	 * and specifically around authentication (not handled by http-backend).
>     ++	 *
>     ++	 * If we wanted to respond with a more 'valid' HTTP response status then
>     ++	 * we'd need to buffer the output of http-backend, wait for and grok the
>     ++	 * exit status of the process, then write the HTTP status line followed
>     ++	 * by the http-backend output. This is outside of the scope of this test
>     ++	 * helper's use at time of writing.
>     ++	 *
>     ++	 * The important auth responses (401) we are handling prior to getting
>     ++	 * to this point.
>     ++	 */
>     ++	if (write(STDOUT_FILENO, ok, strlen(ok)) < 0)
>      +		return error(_("could not send '%s'"), ok);
>      +
>     -+	if (user)
>     -+		strvec_pushf(&cp.env, "REMOTE_USER=%s", user);
>     -+
>      +	strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method);
>      +	strvec_pushf(&cp.env, "PATH_TRANSLATED=%s",
>      +			req->uri_path.buf);
>     @@ t/helper/test-http-server.c: enum worker_result {
>      +	cp.git_cmd = 1;
>      +	strvec_push(&cp.args, "http-backend");
>      +	res = run_command(&cp);
>     -+	close(1);
>     -+	close(0);
>     ++	close(STDOUT_FILENO);
>     ++	close(STDIN_FILENO);
>      +	return !!res;
>      +}
>      +
>      +static enum worker_result dispatch(struct req *req)
>      +{
>      +	if (is_git_request(req))
>     -+		return do__git(req, NULL);
>     ++		return do__git(req);
>      +
>     -+	return send_http_error(1, 501, "Not Implemented", -1, NULL,
>     ++	return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL,
>      +			       WR_OK | WR_HANGUP);
>      +}
>      +
>     @@ t/helper/test-http-server.c: enum worker_result {
>       	char *client_port = getenv("REMOTE_PORT");
>       	enum worker_result wr = WR_OK;
>      @@ t/helper/test-http-server.c: static enum worker_result worker(void)
>     - 	set_keep_alive(0);
>     + 	set_keep_alive(0, logerror);
>       
>       	while (1) {
>     --		if (write_in_full(1, response, strlen(response)) < 0) {
>     +-		if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
>      -			logerror("unable to write response");
>      -			wr = WR_IO_ERROR;
>      -		}
>      +		req__release(&req);
>      +
>     -+		alarm(init_timeout ? init_timeout : timeout);
>     ++		alarm(timeout);
>      +		wr = req__read(&req, 0);
>      +		alarm(0);
>      +
>     -+		if (wr & WR_STOP_THE_MUSIC)
>     ++		if (wr != WR_OK)
>      +			break;
>       
>      +		wr = dispatch(&req);
>     - 		if (wr & WR_STOP_THE_MUSIC)
>     + 		if (wr != WR_OK)
>       			break;
>       	}
>      
>     @@ t/t5556-http-auth.sh (new)
>      +
>      +test_description='test http auth header and credential helper interop'
>      +
>     ++TEST_NO_CREATE_REPO=1
>      +. ./test-lib.sh
>      +
>      +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
>      +
>      +# Setup a repository
>      +#
>     -+REPO_DIR="$(pwd)"/repo
>     ++REPO_DIR="$TRASH_DIRECTORY"/repo
>      +
>      +# Setup some lookback URLs where test-http-server will be listening.
>      +# We will spawn it directly inside the repo directory, so we avoid
>     @@ t/t5556-http-auth.sh (new)
>      +# The server will shutdown if/when we delete it (this is easier than
>      +# killing it by PID).
>      +#
>     -+PID_FILE="$(pwd)"/pid-file.pid
>     -+SERVER_LOG="$(pwd)"/OUT.server.log
>     ++PID_FILE="$TRASH_DIRECTORY"/pid-file.pid
>     ++SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log
>      +
>      +PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
>      +
>     @@ t/t5556-http-auth.sh (new)
>      +
>      +test_expect_success 'http auth anonymous no challenge' '
>      +	test_when_finished "per_test_cleanup" &&
>     -+	start_http_server --allow-anonymous &&
>     ++	start_http_server &&
>      +
>      +	# Attempt to read from a protected repository
>      +	git ls-remote $ORIGIN_URL
>