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

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