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

[Victoria Dye <vdye@github.com>]

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.

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