Re: [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently()

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

larsxschneider@gmail.com writes:

quoted

From: Lars Schneider <redacted>

packet_write_fmt() would die in case of a write error even though for
some callers an error would be acceptable. Add packet_write_fmt_gently()
which writes a formatted pkt-line and returns `0` for success and `-1`
for an error.

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

diff --git a/pkt-line.c b/pkt-line.c
index e8adc0f..3e8b2fb 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -137,6 +137,18 @@ void packet_write_fmt(int fd, const char *fmt, ...)

	write_or_die(fd, buf.buf, buf.len);
}

+int packet_write_fmt_gently(int fd, const char *fmt, ...)
+{
+	static struct strbuf buf = STRBUF_INIT;
+	va_list args;
+
+	strbuf_reset(&buf);
+	va_start(args, fmt);
+	format_packet(&buf, fmt, args);
+	va_end(args);
+	return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
+}

Even though its only a handful lines, it is a bit ugly to have a
completely copied implementation only to have _gently().  I suspect
that you should be able to

	static int packet_write_fmt_1(int fd, int gently,
					const char *fmt, va_list args)
       {
		struct strbuf buf = STRBUF_INIT;
		size_t count;

		format_packet(&buf, fmt, args);
		
		count = write_in_full(fd, buf.buf, buf.len);
               if (count == buf.len)
               	return 0;
		if (!gently) {
			check_pipe(errno);
               	die_errno("write error");
		}
               return -1;
	}

and then share that between the existing one:

	void packet_write_fmt(int fd, const char *fmt, ...)
       {
		va_list args;
	        va_start(args, fmt);
               packet_write_fmt_1(fd, 0, fmt, args);
               va_end(args);
	}

and the new one:

	void packet_write_fmt_gently(int fd, const char *fmt, ...)
       {
		int status;
		va_list args;
	        va_start(args, fmt);
               status = packet_write_fmt_1(fd, 1, fmt, args);
               va_end(args);
		return status;
	}

I agree with your criticism of the code duplication. 

However, I thought it would be OK, as Peff already 
tried to refactor it...
http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f4sx@sigill.intra.peff.net/

... and I got the impression you agreed with Peff:
http://public-inbox.org/git/xmqqvaz84g9y.fsf@gitster.mtv.corp.google.com/


I will try to refactor it according to your suggestion above. 
Would "packet_write_fmt_1()" be an acceptable name or should 
I come up with something more expressive?

Thanks you,
Lars