On 02/27, Jonathan Nieder wrote:
> 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.)
Yeah I agree that an api change is needed, but yeah it can be done on
top of this series.
>
> [...]
> > --- 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.
> )?
>
> > +
> > + 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
