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.
[...]
> --- 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.)
[...]
> --- 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
