Re: [PATCH v5 05/10] test-http-server: add HTTP error response function
- From
Victoria Dye <vdye@github.com>
- Date
- Jan 12, 2023, 20:35 UTC
- Message-ID
- <0a5f4195-600f-d099-5879-bbd7629285b2@github.com>
- In-Reply-To
- <6f66bf146b4d6e4044b3c6c2224795918191bc3f.1673475190.git.gitgitgadget@gmail.com>
Matthew John Cheetham via GitGitGadget wrote:
Show 5 quoted lines
> 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