From: Lars Schneider Date: Mon, 01 Aug 2016 12:18:36 GMT Subject: Re: [PATCH v3 04/10] pkt-line: call packet_trace() only if a packet is actually send Message-ID: <52A5703D-AF9E-405D-A4F8-E30A7B923400@gmail.com> In-Reply-To: <786f0b8e-29f0-3dd3-7bb4-5f6558f8ec84@gmail.com> > On 30 Jul 2016, at 14:29, Jakub Narębski wrote: > > W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze: >> From: Lars Schneider >> >> The packet_trace() call is not ideal in format_packet() as we would print > > Style; I think the following is more readable: > > The packet_trace() call in format_packet() is not ideal, as we would... Agreed! >> a trace when a packet is formatted and (potentially) when the packet is >> actually send. This was no problem up until now because format_packet() >> was only used by one function. Fix it by moving the trace call into the >> function that actally sends the packet. > > s/actally/actually/ Thanks! > I don't buy this explanation. If you want to trace packets, you might > do it on input (when formatting packet), or on output (when writing > packet). It's when there are more than one formatting function, but > one writing function, then placing trace call in write function means > less code duplication; and of course the reverse. > > Another issue is that something may happen between formatting packet > and sending it, and we probably want to packet_trace() when packet > is actually send. > > Neither of those is visible in commit message. The packet_trace() call in format_packet() is not ideal, as we would print a trace when a packet is formatted and (potentially) when the same packet is actually written. This was no problem up until now because packet_write(), the function that uses format_packet() and writes the formatted packet, did not trace the packet. This developer believes that trace calls should only happen when a packet is actually written as the packet could be modified between formatting and writing. Therefore the trace call was moved from format_packet() to packet_write(). -- Better? > >> >> Signed-off-by: Lars Schneider >> --- >> pkt-line.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/pkt-line.c b/pkt-line.c >> index 1728690..32c0a34 100644 >> --- a/pkt-line.c >> +++ b/pkt-line.c >> @@ -126,7 +126,6 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args) >> die("protocol error: impossibly long line"); >> >> set_packet_header(&out->buf[orig_len], n); >> - packet_trace(out->buf + orig_len + 4, n - 4, 1); >> } >> >> void packet_write(int fd, const char *fmt, ...) >> @@ -138,6 +137,7 @@ void packet_write(int fd, const char *fmt, ...) >> va_start(args, fmt); >> format_packet(&buf, fmt, args); >> va_end(args); >> + packet_trace(buf.buf + 4, buf.len - 4, 1); >> write_or_die(fd, buf.buf, buf.len); >> } >> >> >