Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
> On 10 Aug 2016, at 15:43, Jeff Kingwrote: > > On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote: > >> +int packet_write_gently_fmt(int fd, const char *fmt, ...) >> +{ >> +static struct strbuf buf = STRBUF_INIT; >> +va_list args; >> + >> +strbuf_reset(); >> +va_start(args, fmt); >> +format_packet(1, , fmt, args); >> +va_end(args); >> +packet_trace(buf.buf + 4, buf.len - 4, 1); >> +return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); >> +} > > Could the end of this function just be: > > return packet_write_gently(fd, buf.buf, buf.len); > > ? I guess we'd prefer to avoid that, because it incurs an extra > memmove() of the data. I don't think the memmove would be that expensive. However, format_packet() already creates the packet_header and packet_write_gently would do the same again, no? > Similarly, I'd think this could share code with the non-gentle form > (which should be able to just call this and die() if returns an error). > Though sometimes the va_list transformation makes that awkward. Yeah, the code duplication annoyed me, too. va_list was the reason I did it that way. Do you think that is something that needs to be addressed in the series? Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
Jeff Kingwrites: > On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote: > >> +int packet_write_gently_fmt(int fd, const char *fmt, ...) >> +{ >> +static struct strbuf buf = STRBUF_INIT; >> +va_list args; >> + >> +strbuf_reset(); >> +va_start(args, fmt); >> +format_packet(1, , fmt, args); >> +va_end(args); >> +packet_trace(buf.buf + 4, buf.len - 4, 1); >> +return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); >> +} > > Could the end of this function just be: > > return packet_write_gently(fd, buf.buf, buf.len); > > ? I guess we'd prefer to avoid that, because it incurs an extra > memmove() of the data. > > Similarly, I'd think this could share code with the non-gentle form > (which should be able to just call this and die() if returns an error). > Though sometimes the va_list transformation makes that awkward. Yes. Also regarding the naming, please have "_gently" at the end; that is how all other function families with _gently variant are named, I think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
On Wed, Aug 10, 2016 at 04:10:02PM +0200, Lars Schneider wrote: > > > On 10 Aug 2016, at 15:43, Jeff Kingwrote: > > > > On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote: > > > >> +int packet_write_gently_fmt(int fd, const char *fmt, ...) > >> +{ > >> + static struct strbuf buf = STRBUF_INIT; > >> + va_list args; > >> + > >> + strbuf_reset(); > >> + va_start(args, fmt); > >> + format_packet(1, , fmt, args); > >> + va_end(args); > >> + packet_trace(buf.buf + 4, buf.len - 4, 1); > >> + return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); > >> +} > > > > Could the end of this function just be: > > > > return packet_write_gently(fd, buf.buf, buf.len); > > > > ? I guess we'd prefer to avoid that, because it incurs an extra > > memmove() of the data. > > I don't think the memmove would be that expensive. However, format_packet() > already creates the packet_header and packet_write_gently would do the same > again, no? Yeah, I think you would want extra refactoring to have a shared common function. I took a stab at it, but the result ends up pretty ugly; the amount of boilerplate exceeds the duplication here (the really nasty thing is that format_packet() is hard to split up, because the part you want to switch out is in the middle, but it needs to keep some context between the start and the end. In a higher level language you'd pass it a callback to fill in the strbuf in the middle, but in C that just ends up horrible). > > Similarly, I'd think this could share code with the non-gentle form > > (which should be able to just call this and die() if returns an error). > > Though sometimes the va_list transformation makes that awkward. > > Yeah, the code duplication annoyed me, too. va_list was the reason I did it > that way. Do you think that is something that needs to be addressed in the > series? No, I don't think it needs to be. It's just a case of making sure that the internals don't grow too crufty and unmanageable for future maintainability. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote: > +int packet_write_gently_fmt(int fd, const char *fmt, ...) > +{ > + static struct strbuf buf = STRBUF_INIT; > + va_list args; > + > + strbuf_reset(); > + va_start(args, fmt); > + format_packet(1, , fmt, args); > + va_end(args); > + packet_trace(buf.buf + 4, buf.len - 4, 1); > + return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); > +} Could the end of this function just be: return packet_write_gently(fd, buf.buf, buf.len); ? I guess we'd prefer to avoid that, because it incurs an extra memmove() of the data. Similarly, I'd think this could share code with the non-gentle form (which should be able to just call this and die() if returns an error). Though sometimes the va_list transformation makes that awkward. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
From: Lars Schneiderpacket_write() would die in case of a write error even though for some callers an error would be acceptable. Add packet_write_gently_fmt() which writes a formatted pkt-line and returns `0` for success and `-1` for an error. Signed-off-by: Lars Schneider --- pkt-line.c | 13 + pkt-line.h | 1 + 2 files changed, 14 insertions(+) diff --git a/pkt-line.c b/pkt-line.c index 4f25748..a8207dd 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -142,6 +142,19 @@ void packet_write(int fd, const char *fmt, ...) write_or_die(fd, buf.buf, buf.len); } +int packet_write_gently_fmt(int fd, const char *fmt, ...) +{ + static struct strbuf buf = STRBUF_INIT; + va_list args; + + strbuf_reset(); + va_start(args, fmt); + format_packet(1, , fmt, args); + va_end(args); + packet_trace(buf.buf + 4, buf.len - 4, 1); + return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); +} + int packet_write_gently(const int fd_out, const char *buf, size_t size) { if (size > PKTLINE_DATA_MAXLEN) diff --git a/pkt-line.h b/pkt-line.h index 88584f1..82676f4 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -23,6 +23,7 @@ void packet_flush(int fd); void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); void packet_buf_flush(struct strbuf *buf); void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3))); +int packet_write_gently_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); /* * Read a packetized line into the buffer, which must be at least size bytes -- 2.9.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
Lars Schneiderwrites: >>> Similarly, I'd think this could share code with the non-gentle form >>> (which should be able to just call this and die() if returns an error). >>> Though sometimes the va_list transformation makes that awkward. >> >> Yes. > > Peff just posted that he tried the shared code idea but the result > ended up ugly. OK. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
> On 10 Aug 2016, at 19:18, Junio C Hamanowrote: > > Jeff King writes: > >> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote: >> >>> +int packet_write_gently_fmt(int fd, const char *fmt, ...) >>> +{ >>> + static struct strbuf buf = STRBUF_INIT; >>> + va_list args; >>> + >>> + strbuf_reset(); >>> + va_start(args, fmt); >>> + format_packet(1, , fmt, args); >>> + va_end(args); >>> + packet_trace(buf.buf + 4, buf.len - 4, 1); >>> + return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); >>> +} >> >> Could the end of this function just be: >> >> return packet_write_gently(fd, buf.buf, buf.len); >> >> ? I guess we'd prefer to avoid that, because it incurs an extra >> memmove() of the data. >> >> Similarly, I'd think this could share code with the non-gentle form >> (which should be able to just call this and die() if returns an error). >> Though sometimes the va_list transformation makes that awkward. > > Yes. Peff just posted that he tried the shared code idea but the result ended up ugly. > Also regarding the naming, please have "_gently" at the end; that is > how all other function families with _gently variant are named, I > think. OK, I will rename them. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html