On 02/27, Jonathan Nieder wrote:

Brandon Williams wrote:

quoted

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

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

Yeah I agree that an api change is needed, but yeah it can be done on
top of this series.

[...]

quoted

--- 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);

I was reusing the error msg as it appears in another part of this file.

)?

quoted

+
+	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);

I'll change this.

Thanks,
Jonathan

-- 
Brandon Williams