On Wed, Jan 18 2023, 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`.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
> ---
> 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.
> +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)
> +{
> + 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...
> + 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...
> + 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.
> 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.
