Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()

From: Lars Schneider <larsxschneider@gmail.com>
Date: Sep 26, 2016, 19:21 UTC
Message-ID: <4CF26590-7B88-4DB6-B6D4-A6CFE0FD6ABF@gmail.com>
In-Reply-To: <55c5a86e-4055-f36d-bbf1-e8bae919088a@gmail.com>

On 25 Sep 2016, at 13:26, Jakub Narębski <jnareb@gmail.com> wrote:

Show 8 quoted lines

> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> ...
>> 
>> +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.

Show 12 quoted lines

>> +{
>> +	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.

Show 5 quoted lines

>> +	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@sigill.intra.peff.net/

Show 7 quoted lines

>> +		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.

Show 10 quoted lines

>> +	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