RE: [PATCH v1 1/3] pkt-line: add packet_write_list_gently()

[Ben Peart <Ben.Peart@xxxxxxxxxxxxx>]

> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@xxxxxxxxx]
> Sent: Wednesday, March 22, 2017 4:21 PM
> To: Ben Peart <peartben@xxxxxxxxx>
> Cc: git@xxxxxxxxxxxxxxx; Ben Peart <Ben.Peart@xxxxxxxxxxxxx>;
> christian.couder@xxxxxxxxx; larsxschneider@xxxxxxxxx
> Subject: Re: [PATCH v1 1/3] pkt-line: add packet_write_list_gently()
> 
> Ben Peart <peartben@xxxxxxxxx> writes:
> 
> > Add packet_write_list_gently() which writes multiple lines in a single
> > call and then calls packet_flush_gently(). This is used later in this
> > patch series.
> 
> I can see how it would be convenient to have a function like this.
> I'd name it without _gently(), though.  We call something _gently() when we
> initially only have a function that dies hard on error and later want to have a
> variant that returns an error for the caller to handle.  You are starting
> without a dying variant (which is probably a preferable way to structure the
> API).
> 
> Also I am hesitant to take a function that does not take any "list"
> type argument and yet calls itself "write_list".  IOW, I'd expect something like
> these
> 
> 	write_list(int fd, const char **lines);
> 	write_list(int fd, struct string_list *lines);
> 
> given that name, and not "varargs, each of which is a line".  I am tempted to
> suggest
> 
> 	packet_writel(int fd, const char *line, ...);
> 
> noticing similarity with execl(), but perhaps others may be able to come up
> with better names.

Given there haven't been any better names suggested, I'll go ahead and update it to be packet_writel.

> 
> > Signed-off-by: Ben Peart <benpeart@xxxxxxxxxxxxx>
> > ---
> >  pkt-line.c | 19 +++++++++++++++++++
> >  pkt-line.h |  1 +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/pkt-line.c b/pkt-line.c
> > index d4b6bfe076..fccdac1352 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char
> *fmt, ...)
> >  	return status;
> >  }
> >
> > +int packet_write_list_gently(int fd, const char *line, ...) {
> > +	va_list args;
> > +	int err;
> > +	va_start(args, line);
> > +	for (;;) {
> > +		if (!line)
> > +			break;
> > +		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> > +			return -1;
> > +		err = packet_write_fmt_gently(fd, "%s\n", line);
> > +		if (err)
> > +			return err;
> > +		line = va_arg(args, const char*);
> > +	}
> > +	va_end(args);
> > +	return packet_flush_gently(fd);
> > +}
> > +
> >  static int packet_write_gently(const int fd_out, const char *buf,
> > size_t size)  {
> >  	static char packet_write_buffer[LARGE_PACKET_MAX];
> > diff --git a/pkt-line.h b/pkt-line.h
> > index 18eac64830..3674d04509 100644
> > --- a/pkt-line.h
> > +++ b/pkt-line.h
> > @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);  void
> > packet_buf_write(struct strbuf *buf, const char *fmt, ...)
> > __attribute__((format (printf, 2, 3)));  int packet_flush_gently(int
> > fd);  int packet_write_fmt_gently(int fd, const char *fmt, ...)
> > __attribute__((format (printf, 2, 3)));
> > +int packet_write_list_gently(int fd, const char *line, ...);
> >  int write_packetized_from_fd(int fd_in, int fd_out);  int
> > write_packetized_from_buf(const char *src_in, size_t len, int fd_out);