Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()
- From
Jeff King <peff@peff.net>
- Date
- Aug 10, 2016, 15:01 UTC
- Message-ID
- <20160810150139.lpxyrqkr53s5f4sx@sigill.intra.peff.net>
- In-Reply-To
- <E6D0E830-764D-42E5-9F69-340F718B98B2@gmail.com>
On Wed, Aug 10, 2016 at 04:10:02PM +0200, Lars Schneider wrote:
Show 28 quoted lines
>
> > On 10 Aug 2016, at 15:43, Jeff King <peff@peff.net> wrote:
> >
> > On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschneider@gmail.com wrote:
> >
> >> +int packet_write_gently_fmt(int fd, const char *fmt, ...)
> >> +{
> >> + static struct strbuf buf = STRBUF_INIT;
> >> + va_list args;
> >> +
> >> + strbuf_reset(&buf);
> >> + va_start(args, fmt);
> >> + format_packet(1, &buf, 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).
Show 7 quoted lines
> > 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