Re: [PATCH v3 29/35] pkt-line: add packet_buf_write_len function

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 27, 2018, 23:11 UTC
Message-ID: <20180227231145.GF174036@aiede.svl.corp.google.com>
In-Reply-To: <20180207011312.189834-30-bmwill@google.com>

Brandon Williams wrote:

> Add the 'packet_buf_write_len()' function which allows for writing an
> arbitrary length buffer into a 'struct strbuf' and formatting it in
> packet-line format.

Makes sense.

[...]

Show 7 quoted lines

> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -26,6 +26,7 @@ void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_delim(struct strbuf *buf);
>  void packet_write(int fd_out, const char *buf, size_t size);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> +void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);

I wonder if we should rename packet_buf_write to something like packet_buf_writef. Right now there's a kind of confusing collection of functions without much symmetry.

Alternatively, the _buf_ ones could become strbuf_* functions:

	strbuf_add_packet(&buf, data, len);
	strbuf_addf_packet(&buf, fmt, ...);

That would make it clearer that these append to buf.

I'm just thinking out loud. For this series, the API you have here looks fine, even if it is a bit inconsistent. (In other words, even if you agree with me, this would probably be best addressed as a patch on top.)

[...]

Show 17 quoted lines

> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -215,6 +215,22 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
>  	va_end(args);
>  }
>  
> +void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
> +{
> +	size_t orig_len, n;
> +
> +	orig_len = buf->len;
> +	strbuf_addstr(buf, "0000");
> +	strbuf_add(buf, data, len);
> +	n = buf->len - orig_len;
> +
> +	if (n > LARGE_PACKET_MAX)
> +		die("protocol error: impossibly long line");

Could the error message describe the long line (e.g.

		...impossibly long line %.*s...", 256, data);

)?

> +
> +	set_packet_header(&buf->buf[orig_len], n);
> +	packet_trace(buf->buf + orig_len + 4, n - 4, 1);

Could do, more simply:

	packet_trace(data, len, 1);

Thanks, Jonathan