On 25 Sep 2016, at 13:26, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
> W dniu 20.09.2016 o 21:02, larsxschneider@xxxxxxxxx pisze:
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>> ...
>>
>> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>
> I'm not sure what naming convention the rest of Git uses, but isn't
> it more like '*data' rather than '*buf' here?
pkt-line seems to use 'buf' or 'buffer' for everything else.
>> +{
>> + static char packet_write_buffer[LARGE_PACKET_MAX];
>
> I think there should be warning (as a comment before function
> declaration, or before function definition), that packet_write_gently()
> is not thread-safe (nor reentrant, but the latter does not matter here,
> I think).
>
> Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058
>
> This is not something terribly important; I guess git code has tons
> of functions not marked as thread-unsafe...
I agree that the function is not thread-safe. However, I can't find
an example in the Git source that marks a function as not thread-safe.
Unless is it explicitly stated in the coding guidelines I would prefer
not to start way to mark functions.
>> + if (size > sizeof(packet_write_buffer) - 4) {
>
> First, wouldn't the following be more readable:
>
> + if (size + 4 > LARGE_PACKET_MAX) {
Peff suggested that here:
http://public-inbox.org/git/20160810132814.gqnipsdwyzjmuqjy@xxxxxxxxxxxxxxxxxxxxx/
>> + return error("packet write failed - data exceeds max packet size");
>> + }
>
> Second, CodingGuidelines is against using braces (blocks) for one
> line conditionals: "We avoid using braces unnecessarily."
>
> But this is just me nitpicking.
Fixed.
>> + packet_trace(buf, size, 1);
>> + size += 4;
>> + set_packet_header(packet_write_buffer, size);
>> + memcpy(packet_write_buffer + 4, buf, size - 4);
>> + if (write_in_full(fd_out, packet_write_buffer, size) == size)
>
> Hmmm... in some places we use original size, in others (original) size + 4;
> perhaps it would be more readable to add a new local temporary variable
>
> size_t full_size = size + 4;
Agreed. I introduced "packet_size".
Thanks,
Lars