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

On 25 Sep 2016, at 13:26, Jakub Narębski [off-list ref] wrote:

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

quoted

From: Lars Schneider <redacted>
...

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

quoted

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

quoted

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

quoted

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

quoted

+	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