> On 30 Jul 2016, at 14:29, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
>
> W dniu 30.07.2016 o 01:37, larsxschneider@xxxxxxxxx pisze:
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>>
>> 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 <larsxschneider@xxxxxxxxx>
>> ---
>> 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);
>> }
>>
>>
>
--
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