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

["Jakub Narębski" <jnareb@gmail.com>]

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> packet_write_fmt_gently() uses format_packet() which lets the caller
> only send string data via "%s". That means it cannot be used for
> arbitrary data that may contain NULs.
> 
> Add packet_write_gently() which writes arbitrary data and does not die
> in case of an error. The function is used by other pkt-line functions in
> a subsequent patch.

Nice; obviously needed for sending binary data.

I wonder how send-pack / receive-pack handles sending binary files.
Though this is outside of scope of this patch series, it is something
to think about for later.

> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  pkt-line.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index 19f0271..fc0ac12 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,22 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>  	return status;
>  }
>  
> +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?

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

> +
> +	if (size > sizeof(packet_write_buffer) - 4) {

First, wouldn't the following be more readable:

  +	if (size + 4 > LARGE_PACKET_MAX) {

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

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

Or perhaps use 'data_size' and 'packet_size', where 'packet_size = data_size + 4'.
But that might be too chatty for variable names ;-)

> +		return 0;
> +	return error("packet write failed");
> +}

Compared to previous iterations, where there were two versions
of this function, IIRC sharing no common code: one taking buffer
which had to be with place for packet size info, one with a separate
local buffer for packet size only and using two writes.  This
version uses static buffer (thus not thread-safe, I think; and
not reentrant), and memcpy.

Anyway, if reentrant / thread-safe version would be required,
or not doing memcpy turns out to be important with respect to
performance, we can provide with the *_r version:

  static int packet_write_gently_r(const int fd_out, const char *data, size_t size,
                                   char *restrict buf)

We can check if 'buf + 4 == data', and if it is, we can skip
memcpy() as an optimization.

This is something for the future, but not very important for
having this patch series accepted.

> +
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
>  {
>  	va_list args;
> 

Best,
-- 
Jakub Narębski