Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()

larsxschneider@gmail.com writes:

From: Lars Schneider <redacted>

packet_write_fmt() has two shortcomings. First, it 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. Second, it will
always die on error.

As you introduced _gently in 3/13, the latter is no longer a valid
excuse to add this function.  Just remove the sentence "Second, ...".

It may not matter all that much, but from code-reader's point of
view, when you know packet_write_buffer[] will contain things A and
B in this order, and when you have enough information to compute A
before stasrting to fill packet_write_buffer[], I would prefer to
see you actually fill the buffer in that natural order.

Do you anticipate future need of non-gently variant of this
function?  If so, perhaps a helper that takes a boolean "am I
working for the gently variant?" may help share more code.

+	return (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 : -1);
+}
+
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list args;

On 25 Aug 2016, at 23:50, Junio C Hamano [off-list ref] wrote:

larsxschneider@gmail.com writes:

quoted

From: Lars Schneider <redacted>

packet_write_fmt() has two shortcomings. First, it 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. Second, it will
always die on error.

As you introduced _gently in 3/13, the latter is no longer a valid
excuse to add this function.  Just remove the sentence "Second, ...".

Agreed!

quoted

Add packet_write_gently() which writes arbitrary data and returns `0`
for success and `-1` for an error. This function is used by other
pkt-line functions in a subsequent patch.

Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index cad26df..7e8a803 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -3,6 +3,7 @@

#include "run-command.h"

char packet_buffer[LARGE_PACKET_MAX];
+static char packet_write_buffer[LARGE_PACKET_MAX];
static const char *packet_trace_prefix = "git";
static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);
static struct trace_key trace_pack = TRACE_KEY_INIT(PACKFILE);

@@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)

	return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
}

+int packet_write_gently(const int fd_out, const char *buf, size_t size)
+{
+	if (size > sizeof(packet_write_buffer) - 4)
+		return -1;
+	packet_trace(buf, size, 1);
+	memmove(packet_write_buffer + 4, buf, size);
+	size += 4;
+	set_packet_header(packet_write_buffer, size);

It may not matter all that much, but from code-reader's point of
view, when you know packet_write_buffer[] will contain things A and
B in this order, and when you have enough information to compute A
before stasrting to fill packet_write_buffer[], I would prefer to
see you actually fill the buffer in that natural order.

I did that because when packet_write_stream_with_flush_from_fd()
calls packet_write_gently() then buf[] is actually packet_write_buffer[]:

https://github.com/larsxschneider/git/blob/d474e6a4c2523b87624a07111eb7a4f2dcd12426/pkt-line.c#L185-L192

Therefore I would override the first 4 bytes. However, the code evolved for
some reason in that way but looking at it now I think that is an obscure,
likely meaningless optimization. I'll use a separate buffer in 
packet_write_stream_with_flush_from_fd() and then fix the order here
following your advice.

Do you anticipate future need of non-gently variant of this
function?  If so, perhaps a helper that takes a boolean "am I
working for the gently variant?" may help share more code.

With helper you mean "an additional boolean parameter"? I don't 
see a need for a non-gently variant right now but I will
add this parameter if you think it is a good idea. How would the
signature look like?

int packet_write_gently(const int fd_out, const char *buf, size_t size, int gentle)

This would follow type_from_string_gently() in object.h.

Thanks,
Lars