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

> 
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> 
>> 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.
> 
>> +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.