Subject: Re: [PATCH v6 05/12] test-http-server: add HTTP error response function

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Wed, 18 Jan 2023 12:07:52 +0100

On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:

> From: Matthew John Cheetham <[email-masked]>
>
> 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 <[email-masked]>
> ---
>  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.