Re: [PATCH] http-backend: buffer headers before sending

[Junio C Hamano <gitster@pobox.com>]

Eric Wong <e@80x24.org> writes:

> diff --git a/http-backend.c b/http-backend.c
> index 0d59499..adc8c8c 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -75,55 +75,57 @@ static void format_write(int fd, const char *fmt, ...)
>  	write_or_die(fd, buffer, n);
>  }
>  
> -static void http_status(unsigned code, const char *msg)
> +static void http_status(struct strbuf *hdr, unsigned code, const char *msg)
>  {
> -	format_write(1, "Status: %u %s\r\n", code, msg);
> +	strbuf_addf(hdr, "Status: %u %s\r\n", code, msg);
>  }

The idea is to pass "struct strbuf *hdr" around the callchain to
helpers so that they can all append into it instead of doing
format_write(), which is sensible.  This pattern continues quite a
bit (omitted).

> -static void end_headers(void)
> +static void end_headers(struct strbuf *hdr)
>  {
> -	write_or_die(1, "\r\n", 2);
> +	strbuf_add(hdr, "\r\n", 2);
> +	write_or_die(1, hdr->buf, hdr->len);
> +	strbuf_release(hdr);
>  }

Then end_headers() is taught to drain the buffered headers.  The
helpers used by other helpers in the next level of abstraction that
emit the header and the body also need to take the buffer as their
parameter, like this one:

> -static void send_strbuf(const char *type, struct strbuf *buf)
> +static void send_strbuf(struct strbuf *hdr,
> +			const char *type, struct strbuf *buf)
>  {
> -	hdr_int(content_length, buf->len);
> -	hdr_str(content_type, type);
> -	end_headers();
> +	hdr_int(hdr, content_length, buf->len);
> +	hdr_str(hdr, content_type, type);
> +	end_headers(hdr);
>  	write_or_die(1, buf->buf, buf->len);
>  }

because their caller already has something to say in the header
part.

> +static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
> +{
> +	const char *proto = getenv("SERVER_PROTOCOL");
> +
> +	if (proto && !strcmp(proto, "HTTP/1.1")) {
> +		http_status(hdr, 405, "Method Not Allowed");
> +		hdr_str(hdr, "Allow",
> +			!strcmp(c->method, "GET") ? "GET, HEAD" : c->method);
> +	} else
> +		http_status(hdr, 400, "Bad Request");
> +	hdr_nocache(hdr);
> +	end_headers(hdr);
> +	return 0;
> +}
> +

This is like other helpers that respond with 4xx, e.g. forbidden(),
but it did not exist only because there was a single callsite that
needed this feature, which made it open-coded directly in main().
It was somewhat surprising to see a new helper, because the only
thing this patch changes logically is where the output is sent, but
this is a very sensible refactoring.

>  int cmd_main(int argc, const char **argv)
>  {
>  	char *method = getenv("REQUEST_METHOD");
> ...
> @@ -659,18 +681,8 @@ int cmd_main(int argc, const char **argv)
>  		if (!regexec(&re, dir, 1, out, 0)) {
>  			size_t n;
>  
> -			if (strcmp(method, c->method)) {
> -				const char *proto = getenv("SERVER_PROTOCOL");
> -				if (proto && !strcmp(proto, "HTTP/1.1")) {
> -					http_status(405, "Method Not Allowed");
> -					hdr_str("Allow", !strcmp(c->method, "GET") ?
> -						"GET, HEAD" : c->method);
> -				} else
> -					http_status(400, "Bad Request");
> -				hdr_nocache();
> -				end_headers();
> -				return 0;
> -			}
> +			if (strcmp(method, c->method))
> +				return bad_request(&hdr, c);

... and this is where it came from.  It could have been a separate
"preparatory cleanup" step, but it is so trivial that "while at it"
clean-up is perfectly fine.

Thanks, will queue.