Re: [PATCH v3 04/10] pkt-line: call packet_trace() only if a packet is actually send

On 30 Jul 2016, at 14:29, Jakub Narębski [off-list ref] wrote:

W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze:

quoted

From: Lars Schneider <redacted>

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!

quoted

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?

quoted

Signed-off-by: Lars Schneider <redacted>
---
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);
}