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

[Jeff King]

On Fri, Aug 26, 2016 at 10:10:50AM -0700, Junio C Hamano wrote:

> Lars Schneider  writes:
> 
> > 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.lpxyrqkr53s5f...@sigill.intra.peff.net/
> >
> > ... and I got the impression you agreed with Peff:
> > http://public-inbox.org/git/xmqqvaz84g9y@gitster.mtv.corp.google.com/
> 
> The former does not exactly show how ugly it was, but I do not have
> to see it.  It is talking about eliminating the need for memcpy()
> and duplicated header generation code, which the suggestion you are
> responding to didn't even attempt.  If Peff said he tried an even
> more aggressive refactoring and it ended up too ugly to live, I
> believe him and agree with his assessment.

Right, what I found difficulty with was factoring out format_packet(). I
think the "gently" part is easy.

-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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Junio C Hamano]

Lars Schneider  writes:

> 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.lpxyrqkr53s5f...@sigill.intra.peff.net/
>
> ... and I got the impression you agreed with Peff:
> http://public-inbox.org/git/xmqqvaz84g9y@gitster.mtv.corp.google.com/

The former does not exactly show how ugly it was, but I do not have
to see it.  It is talking about eliminating the need for memcpy()
and duplicated header generation code, which the suggestion you are
responding to didn't even attempt.  If Peff said he tried an even
more aggressive refactoring and it ended up too ugly to live, I
believe him and agree with his assessment.

> 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?

The latter is preferrable, but we do not mind too strongly about
the name of file-scope static helper that will never be called
directly by anybody other than the two more public entry points the
helper was designed to serve.
--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Lars Schneider]

> On 25 Aug 2016, at 23:41, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> 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 
>> ---
>> 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();
>> +va_start(args, fmt);
>> +format_packet(, 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(, 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.lpxyrqkr53s5f...@sigill.intra.peff.net/

... and I got the impression you agreed with Peff:
http://public-inbox.org/git/xmqqvaz84g9y@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--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> 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 
> ---
>  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();
> + va_start(args, fmt);
> + format_packet(, 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(, 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;
}

>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
>  {
>   va_list args;
> diff --git a/pkt-line.h b/pkt-line.h
> index 1902fb3..3caea77 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -23,6 +23,7 @@ void packet_flush(int fd);
>  void packet_write_fmt(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_fmt_gently(int fd, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
>  
>  /*
>   * Read a packetized line into the buffer, which must be at least size bytes
--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Lars Schneider]

On 25 Aug 2016, at 20:12, Stefan Beller  wrote:

>> +int packet_write_fmt_gently(int fd, const char *fmt, ...)
>> +{
>> +   static struct strbuf buf = STRBUF_INIT;
>> +   va_list args;
>> +
>> +   strbuf_reset();
>> +   va_start(args, fmt);
>> +   format_packet(, fmt, args);
> 
> format_packet also takes care of tracing the contents,
> which was a bit unexpected to me.

To me, too :)
http://public-inbox.org/git/20160810143321.q7mjirgr5ynml...@sigill.intra.peff.net/

The series is already pretty large and therefore I decided to leave this as-is.

> Do we also want to trace failure?

You mean in all new *_gently() functions? Good idea!

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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Stefan Beller]

> +int packet_write_fmt_gently(int fd, const char *fmt, ...)
> +{
> +   static struct strbuf buf = STRBUF_INIT;
> +   va_list args;
> +
> +   strbuf_reset();
> +   va_start(args, fmt);
> +   format_packet(, fmt, args);

format_packet also takes care of tracing the contents,
which was a bit unexpected to me.
Do we also want to trace failure?
--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[larsxschneider]

From: Lars Schneider 

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 
---
 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();
+   va_start(args, fmt);
+   format_packet(, fmt, args);
+   va_end(args);
+   return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
+}
+
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
va_list args;
diff --git a/pkt-line.h b/pkt-line.h
index 1902fb3..3caea77 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -23,6 +23,7 @@ void packet_flush(int fd);
 void packet_write_fmt(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_fmt_gently(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