> On 10 Aug 2016, at 15:30, Jeff King <peff@xxxxxxxx> wrote: > > On Wed, Aug 10, 2016 at 03:24:38PM +0200, Lars Schneider wrote: > >>> On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@xxxxxxxxx wrote: >>> >>>> From: Lars Schneider <larsxschneider@xxxxxxxxx> >>>> >>>> The packet_trace() call is not ideal in format_packet() as we would print >>>> 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 actually sends the packet. >>> >>> It looks like there are two functions: packet_write() and >>> packet_buf_write(). >> >> I did not call trace in packet_buf_write() because this function does not >> perform any writes. > > Yes, but then who is responsible for the trace? The caller? >From my point of view the one that issues the write call. > And why is it a bad thing to do it some time other than writing? It is > if you format and then _don't_ write the packet, but the current callers > are not doing that. True, they don't do that. However, I don't think that is intuitive behavior for future callers. If I call "format" and then trace tells me that a packet was sent although it was not (yet). > >>> Your patch only touches one of them, and it looks like we would fail to >>> trace many packets (e.g., see receive-pack.c:report(), which uses >>> packet_buf_write() and then write()s out the result). >> >> I see. But isn't it confusing if packet_buf_write() issues a trace call? >> If I just call this function then nothing happens at all. Shouldn't the >> trace call be made in receive-pack.c:report() ? Or shouldn't receive-pack >> let pkt-line.c perform the write calls? > > How would report() do that without re-parsing each of the packets? We could add a function to pkt-line that takes a list of strings and generates a list of packets with a terminating flush packet out of it. Then it sends the packets. As a quick compromise we could also introduce a function "packet_buf_write_with_trace()" that wraps the "packet_buf_write()" and calls trace. However, I can imagine that would be perceived as ugly. I guess my point is that I stumbled over the un-intutiive format_packet() behavior and I wanted to improve the situation in a way that others don't run into this trap. If you think that is no issue then it would be OK for me if we leave the current behavior as is. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
