Re: [PATCH v6 05/12] test-http-server: add HTTP error response function
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 18, 2023, 11:07 UTC
- Message-ID
- <230118.86fsc8xffg.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <79805f042b984bb8ca7c9aaf6a15f8101037c375.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
Show 18 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`.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
> t/helper/test-http-server.c | 58 +++++++++++++++++++++++++++++++++----
> 1 file changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
> index 11071b1dd89..6cdac223a55 100644
> --- a/t/helper/test-http-server.c
> +++ b/t/helper/test-http-server.c
> @@ -83,9 +83,59 @@ enum worker_result {
> WR_HANGUP = 1<<1,
> };...okey, this is the commit that makes use of WR_HANGUP. Whatever else we do, let's then squash that addition into this change.
Show 5 quoted lines
> +static enum worker_result send_http_error( > + int fd, > + int http_code, const char *http_code_name, > + int retry_after_seconds, struct string_list *response_headers, > + enum worker_result wr_in)
In general in this series you are mis-indenting argument lists. Our usual style is to wrap at 79 characters, then to align (with tabs and spaces) with the "(".
So in this case:
static enum worker_result send_http_error(int fd, int http_code, const char *http_code_name, int retry_after_seconds, struct string_list *response_headers, enum worker_result wr_in)
Show 8 quoted lines
> +{
> + struct strbuf response_header = STRBUF_INIT;
> + struct strbuf response_content = STRBUF_INIT;
> + struct string_list_item *h;
> + enum worker_result wr;
> +
> + strbuf_addf(&response_content, "Error: %d %s\r\n",
> + http_code, http_code_name);Ditto here, where "http_code" should go on the preceding line...
Show 5 quoted lines
> + if (retry_after_seconds > 0) > + strbuf_addf(&response_content, "Retry-After: %d\r\n", > + retry_after_seconds); > + > + strbuf_addf (&response_header, "HTTP/1.1 %d %s\r\n", http_code, http_code_name);
...and here there's a lack of such wrapping...
Show 7 quoted lines
> + strbuf_addstr(&response_header, "Cache-Control: private\r\n"); > + strbuf_addstr(&response_header, "Content-Type: text/plain\r\n"); > + strbuf_addf (&response_header, "Content-Length: %d\r\n", (int)response_content.len); > + if (retry_after_seconds > 0) > + strbuf_addf(&response_header, "Retry-After: %d\r\n", retry_after_seconds); > + strbuf_addf( &response_header, "Server: test-http-server/%s\r\n", git_version_string); > + strbuf_addf( &response_header, "Date: %s\r\n", show_date(time(NULL), 0, DATE_MODE(RFC2822)));
...here you're adding strange whitespace at the start of an argument list...
> + if (response_headers) > + for_each_string_list_item(h, response_headers) > + strbuf_addf(&response_header, "%s\r\n", h->string); > + strbuf_addstr(&response_header, "\r\n");
To comment on the code a bit, this whole thing would be more readable IMO if your own headers were also a "struct string_list". Yes we'd waste a bit more memory, but in this case that's fine..
I.e. don't add the "\r\n" every time, just:
string_list_append(&headers, "Cache-Control: private");
etc.
Then at the end you'd do e.g.:
add_headers(&buf, &headers); if (response_headers) add_headers(&buf, response_headers);
Where the add_headers() is a trivial "static" helper which does that for_each_string_list_item() loop above.
Show 7 quoted lines
> while (1) {
> - if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
> - logerror("unable to write response");
> - wr = WR_IO_ERROR;
> - }
> + wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1,
> + NULL, WR_OK | WR_HANGUP);This *does* use correct wrapping & indenation for a continuing argument list.