Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[Lars Schneider]

> On 10 Aug 2016, at 15:43, Jeff King  wrote:
> 
> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote:
> 
>> +int packet_write_gently_fmt(int fd, const char *fmt, ...)
>> +{
>> +static struct strbuf buf = STRBUF_INIT;
>> +va_list args;
>> +
>> +strbuf_reset();
>> +va_start(args, fmt);
>> +format_packet(1, , fmt, args);
>> +va_end(args);
>> +packet_trace(buf.buf + 4, buf.len - 4, 1);
>> +return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
>> +}
> 
> Could the end of this function just be:
> 
>  return packet_write_gently(fd, buf.buf, buf.len);
> 
> ? I guess we'd prefer to avoid that, because it incurs an extra
> memmove() of the data.

I don't think the memmove would be that expensive. However, format_packet()
already creates the packet_header and packet_write_gently would do the same
again, no?


> Similarly, I'd think this could share code with the non-gentle form
> (which should be able to just call this and die() if returns an error).
> Though sometimes the va_list transformation makes that awkward.

Yeah, the code duplication annoyed me, too. va_list was the reason I did it
that way. Do you think that is something that needs to be addressed in the
series?

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[Junio C Hamano]

Jeff King  writes:

> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote:
>
>> +int packet_write_gently_fmt(int fd, const char *fmt, ...)
>> +{
>> +static struct strbuf buf = STRBUF_INIT;
>> +va_list args;
>> +
>> +strbuf_reset();
>> +va_start(args, fmt);
>> +format_packet(1, , fmt, args);
>> +va_end(args);
>> +packet_trace(buf.buf + 4, buf.len - 4, 1);
>> +return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
>> +}
>
> Could the end of this function just be:
>
>   return packet_write_gently(fd, buf.buf, buf.len);
>
> ? I guess we'd prefer to avoid that, because it incurs an extra
> memmove() of the data.
>
> Similarly, I'd think this could share code with the non-gentle form
> (which should be able to just call this and die() if returns an error).
> Though sometimes the va_list transformation makes that awkward.

Yes.

Also regarding the naming, please have "_gently" at the end; that is
how all other function families with _gently variant are named, I
think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[Jeff King]

On Wed, Aug 10, 2016 at 04:10:02PM +0200, Lars Schneider wrote:

> 
> > On 10 Aug 2016, at 15:43, Jeff King  wrote:
> > 
> > On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote:
> > 
> >> +int packet_write_gently_fmt(int fd, const char *fmt, ...)
> >> +{
> >> +  static struct strbuf buf = STRBUF_INIT;
> >> +  va_list args;
> >> +
> >> +  strbuf_reset();
> >> +  va_start(args, fmt);
> >> +  format_packet(1, , fmt, args);
> >> +  va_end(args);
> >> +  packet_trace(buf.buf + 4, buf.len - 4, 1);
> >> +  return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
> >> +}
> > 
> > Could the end of this function just be:
> > 
> >  return packet_write_gently(fd, buf.buf, buf.len);
> > 
> > ? I guess we'd prefer to avoid that, because it incurs an extra
> > memmove() of the data.
> 
> I don't think the memmove would be that expensive. However, format_packet()
> already creates the packet_header and packet_write_gently would do the same
> again, no?

Yeah, I think you would want extra refactoring to have a shared common
function. I took a stab at it, but the result ends up pretty ugly; the
amount of boilerplate exceeds the duplication here (the really nasty
thing is that format_packet() is hard to split up, because the part you
want to switch out is in the middle, but it needs to keep some context
between the start and the end. In a higher level language you'd pass it
a callback to fill in the strbuf in the middle, but in C that just ends
up horrible).

> > Similarly, I'd think this could share code with the non-gentle form
> > (which should be able to just call this and die() if returns an error).
> > Though sometimes the va_list transformation makes that awkward.
> 
> Yeah, the code duplication annoyed me, too. va_list was the reason I did it
> that way. Do you think that is something that needs to be addressed in the
> series?

No, I don't think it needs to be. It's just a case of making sure that
the internals don't grow too crufty and unmanageable for future
maintainability.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[Jeff King]

On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote:

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

Could the end of this function just be:

  return packet_write_gently(fd, buf.buf, buf.len);

? I guess we'd prefer to avoid that, because it incurs an extra
memmove() of the data.

Similarly, I'd think this could share code with the non-gentle form
(which should be able to just call this and die() if returns an error).
Though sometimes the va_list transformation makes that awkward.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[larsxschneider]

From: Lars Schneider 

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

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

diff --git a/pkt-line.c b/pkt-line.c
index 4f25748..a8207dd 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -142,6 +142,19 @@ void packet_write(int fd, const char *fmt, ...)
write_or_die(fd, buf.buf, buf.len);
 }
 
+int packet_write_gently_fmt(int fd, const char *fmt, ...)
+{
+   static struct strbuf buf = STRBUF_INIT;
+   va_list args;
+
+   strbuf_reset();
+   va_start(args, fmt);
+   format_packet(1, , fmt, args);
+   va_end(args);
+   packet_trace(buf.buf + 4, buf.len - 4, 1);
+   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 > PKTLINE_DATA_MAXLEN)
diff --git a/pkt-line.h b/pkt-line.h
index 88584f1..82676f4 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -23,6 +23,7 @@ void packet_flush(int fd);
 void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 
2, 3)));
 void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
+int packet_write_gently_fmt(int fd, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
 
 /*
  * Read a packetized line into the buffer, which must be at least size bytes
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[Junio C Hamano]

Lars Schneider  writes:

>>> Similarly, I'd think this could share code with the non-gentle form
>>> (which should be able to just call this and die() if returns an error).
>>> Though sometimes the va_list transformation makes that awkward.
>> 
>> Yes.
>
> Peff just posted that he tried the shared code idea but the result
> ended up ugly.

OK.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/15] pkt-line: add packet_write_gently_fmt()

[Lars Schneider]

> On 10 Aug 2016, at 19:18, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> On Wed, Aug 10, 2016 at 03:04:01PM +0200, larsxschnei...@gmail.com wrote:
>> 
>>> +int packet_write_gently_fmt(int fd, const char *fmt, ...)
>>> +{
>>> +   static struct strbuf buf = STRBUF_INIT;
>>> +   va_list args;
>>> +
>>> +   strbuf_reset();
>>> +   va_start(args, fmt);
>>> +   format_packet(1, , fmt, args);
>>> +   va_end(args);
>>> +   packet_trace(buf.buf + 4, buf.len - 4, 1);
>>> +   return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
>>> +}
>> 
>> Could the end of this function just be:
>> 
>>  return packet_write_gently(fd, buf.buf, buf.len);
>> 
>> ? I guess we'd prefer to avoid that, because it incurs an extra
>> memmove() of the data.
>> 
>> Similarly, I'd think this could share code with the non-gentle form
>> (which should be able to just call this and die() if returns an error).
>> Though sometimes the va_list transformation makes that awkward.
> 
> Yes.

Peff just posted that he tried the shared code idea but the result
ended up ugly.


> Also regarding the naming, please have "_gently" at the end; that is
> how all other function families with _gently variant are named, I
> think.

OK, I will rename them.

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html