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