Matthew John Cheetham via GitGitGadget wrote: > From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx> > > 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@xxxxxxxxxx/ [2] https://lore.kernel.org/git/e957d4f4-fa94-7a68-f378-38e6ed131244@xxxxxxxxxx/ [3] https://lore.kernel.org/git/f99c381c-1d30-7c95-6158-cecd5321dafd@xxxxxxxxxx/ 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
