On 2023-01-18 03:07, Ævar Arnfjörð Bjarmason wrote:
>
> 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");
Argh! Thanks again for catching these. I shall address them.
> 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.
In reality this only helps simplify the code in the case of a simple static
header like "Cache-Control: private". There's no `string_list_appendf` or
similar where I need to append a header that contains dynamic information
(date, content length, etc).
Building the `strbuf` directly, and specifying the CRLF seems a lot easier IMO.
>> 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.
Thanks,
Matthew
