Re: [PATCH v3 29/35] pkt-line: add packet_buf_write_len function
From: Jonathan Nieder <hidden>
Date: 2018-02-27 23:11:54
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. [...]
quoted hunk ↗ jump to hunk
--- 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.) [...]
quoted hunk ↗ jump to hunk
--- 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