On 2023-01-12 12:35, Victoria Dye wrote: > 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. 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@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 >
