[PATCH] pkt-line: do not report packet write errors twice

[PATCH] pkt-line: do not report packet write errors twice

[Matheus Tavares]

On write() errors, packet_write() dies with the same error message that
is already printed by its callee, packet_write_gently(). This produces
an unnecessarily verbose and repetitive output:

error: packet write failed
fatal: packet write failed: <strerror() message>

In addition to that, packet_write_gently() does not always fulfill its
caller expectation that errno will be properly set before a non-zero
return. In particular, that is not the case for a "data exceeds max
packet size" error. So, in this case, packet_write() will call
die_errno() and print a strerror(errno) message that might be totally
unrelated to the actual error.

Fix both those issues by turning packet_write() and
packet_write_gently() into wrappers to a lower level function which is
now responsible to either error() or die() as requested by its caller.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 pkt-line.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 0194137528..39c9ca4212 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -194,13 +194,18 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	return status;
 }
 
-static int packet_write_gently(const int fd_out, const char *buf, size_t size)
+static int do_packet_write(const int fd_out, const char *buf, size_t size,
+			   int gentle)
 {
 	char header[4];
 	size_t packet_size;
 
-	if (size > LARGE_PACKET_DATA_MAX)
-		return error(_("packet write failed - data exceeds max packet size"));
+	if (size > LARGE_PACKET_DATA_MAX) {
+		if (gentle)
+			return error(_("packet write failed - data exceeds max packet size"));
+		else
+			die(_("packet write failed - data exceeds max packet size"));
+	}
 
 	packet_trace(buf, size, 1);
 	packet_size = size + 4;
@@ -215,15 +220,23 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 	 */
 
 	if (write_in_full(fd_out, header, 4) < 0 ||
-	    write_in_full(fd_out, buf, size) < 0)
-		return error(_("packet write failed"));
+	    write_in_full(fd_out, buf, size) < 0) {
+		if (gentle)
+			return error_errno(_("packet write failed"));
+		else
+			die_errno(_("packet write failed"));
+	}
 	return 0;
 }
 
+static int packet_write_gently(const int fd_out, const char *buf, size_t size)
+{
+	return do_packet_write(fd_out, buf, size, 1);
+}
+
 void packet_write(int fd_out, const char *buf, size_t size)
 {
-	if (packet_write_gently(fd_out, buf, size))
-		die_errno(_("packet write failed"));
+	do_packet_write(fd_out, buf, size, 0);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
-- 
2.30.1

Re: [PATCH] pkt-line: do not report packet write errors twice

[Junio C Hamano]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> On write() errors, packet_write() dies with the same error message that
> is already printed by its callee, packet_write_gently(). This produces
> an unnecessarily verbose and repetitive output:
>
> error: packet write failed
> fatal: packet write failed: <strerror() message>
>
> In addition to that, packet_write_gently() does not always fulfill its
> caller expectation that errno will be properly set before a non-zero
> return. In particular, that is not the case for a "data exceeds max
> packet size" error. So, in this case, packet_write() will call
> die_errno() and print a strerror(errno) message that might be totally
> unrelated to the actual error.
>
> Fix both those issues by turning packet_write() and
> packet_write_gently() into wrappers to a lower level function which is
> now responsible to either error() or die() as requested by its caller.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  pkt-line.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)

Nicely done.  Duplicated error message literals do look, eh,
repetitious, though.

I wonder if something like this on top would be an improvement.

Upon seeing "return error(_(VARIABLE_NAME))", it may be distracting
that you now have to move to where the actual message is defined
while following the logic of the code, but as long as the variable
name captures the essense of what the message says, it may be OK.

I dunno.


 pkt-line.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git c/pkt-line.c w/pkt-line.c
index 39c9ca4212..d357c74fcd 100644
--- c/pkt-line.c
+++ w/pkt-line.c
@@ -199,12 +199,16 @@ static int do_packet_write(const int fd_out, cons
 {
 	char header[4];
 	size_t packet_size;
+	static const char size_error_message[] =
+		N_("packet write failed - data exceeds max packet size");
+	static const char write_error_message[] =
+		N_("packet write failed");
 
 	if (size > LARGE_PACKET_DATA_MAX) {
 		if (gentle)
-			return error(_("packet write failed - data exceeds max packet size"));
+			return error(_(size_error_message));
 		else
-			die(_("packet write failed - data exceeds max packet size"));
+			die(_(size_error_message));
 	}
 
 	packet_trace(buf, size, 1);
@@ -222,9 +226,9 @@ static int do_packet_write(const int fd_out, const
 	if (write_in_full(fd_out, header, 4) < 0 ||
 	    write_in_full(fd_out, buf, size) < 0) {
 		if (gentle)
-			return error_errno(_("packet write failed"));
+			return error_errno(_(write_error_message));
 		else
-			die_errno(_("packet write failed"));
+			die_errno(_(write_error_message));
 	}
 	return 0;
 }

Re: [PATCH] pkt-line: do not report packet write errors twice

[Matheus Tavares Bernardino]

On Thu, Apr 15, 2021 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > On write() errors, packet_write() dies with the same error message that
> > is already printed by its callee, packet_write_gently(). This produces
> > an unnecessarily verbose and repetitive output:
> >
> > error: packet write failed
> > fatal: packet write failed: <strerror() message>
> >
> > In addition to that, packet_write_gently() does not always fulfill its
> > caller expectation that errno will be properly set before a non-zero
> > return. In particular, that is not the case for a "data exceeds max
> > packet size" error. So, in this case, packet_write() will call
> > die_errno() and print a strerror(errno) message that might be totally
> > unrelated to the actual error.
> >
> > Fix both those issues by turning packet_write() and
> > packet_write_gently() into wrappers to a lower level function which is
> > now responsible to either error() or die() as requested by its caller.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> >  pkt-line.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
>
> Nicely done.  Duplicated error message literals do look, eh,
> repetitious, though.
>
> I wonder if something like this on top would be an improvement.
>
> Upon seeing "return error(_(VARIABLE_NAME))", it may be distracting
> that you now have to move to where the actual message is defined
> while following the logic of the code, but as long as the variable
> name captures the essense of what the message says, it may be OK.
>
> I dunno.
>
>
>  pkt-line.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git c/pkt-line.c w/pkt-line.c
> index 39c9ca4212..d357c74fcd 100644
> --- c/pkt-line.c
> +++ w/pkt-line.c
> @@ -199,12 +199,16 @@ static int do_packet_write(const int fd_out, cons
>  {
>         char header[4];
>         size_t packet_size;
> +       static const char size_error_message[] =
> +               N_("packet write failed - data exceeds max packet size");
> +       static const char write_error_message[] =
> +               N_("packet write failed");
>
>         if (size > LARGE_PACKET_DATA_MAX) {
>                 if (gentle)
> -                       return error(_("packet write failed - data exceeds max packet size"));
> +                       return error(_(size_error_message));
>                 else
> -                       die(_("packet write failed - data exceeds max packet size"));
> +                       die(_(size_error_message));
>         }
>
>         packet_trace(buf, size, 1);
> @@ -222,9 +226,9 @@ static int do_packet_write(const int fd_out, const
>         if (write_in_full(fd_out, header, 4) < 0 ||
>             write_in_full(fd_out, buf, size) < 0) {
>                 if (gentle)
> -                       return error_errno(_("packet write failed"));
> +                       return error_errno(_(write_error_message));
>                 else
> -                       die_errno(_("packet write failed"));
> +                       die_errno(_(write_error_message));
>         }
>         return 0;
>  }

Nice! :) Maybe we could also avoid the static strings without
repeating the literals by making `do_packet_write()` receive a `struct
strbuf *err` and save the error message in it? Then the two callers
can decide whether to pass it to error() or die() accordingly.

Re: [PATCH] pkt-line: do not report packet write errors twice

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> Nice! :) Maybe we could also avoid the static strings without
> repeating the literals by making `do_packet_write()` receive a `struct
> strbuf *err` and save the error message in it? Then the two callers
> can decide whether to pass it to error() or die() accordingly.

Sorry, but I do not understand what benefit we are trying to gain by
introducing an extra strbuf (which I assume is used to strbuf_add()
the error message into).  Wouldn't the caller need two messages and
flip between <error,error_errno> vs <die,die_errno> pair?

Re: [PATCH] pkt-line: do not report packet write errors twice

[Matheus Tavares]

On Thu, Apr 15, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:
>
> > Nice! :) Maybe we could also avoid the static strings without
> > repeating the literals by making `do_packet_write()` receive a `struct
> > strbuf *err` and save the error message in it? Then the two callers
> > can decide whether to pass it to error() or die() accordingly.
>
> Sorry, but I do not understand what benefit we are trying to gain by
> introducing an extra strbuf (which I assume is used to strbuf_add()
> the error message into).  Wouldn't the caller need two messages and
> flip between <error,error_errno> vs <die,die_errno> pair?

Hmm, I'm not sure I understand what you mean by the two messages, but
what I was thinking of is something like this (on top of my original
patch):

diff --git a/pkt-line.c b/pkt-line.c
index 39c9ca4212..98304ce374 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -195,16 +195,14 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 }

 static int do_packet_write(const int fd_out, const char *buf, size_t size,
-			   int gentle)
+			   struct strbuf *err)
 {
 	char header[4];
 	size_t packet_size;

 	if (size > LARGE_PACKET_DATA_MAX) {
-		if (gentle)
-			return error(_("packet write failed - data exceeds max packet size"));
-		else
-			die(_("packet write failed - data exceeds max packet size"));
+		strbuf_addstr(err, _("packet write failed - data exceeds max packet size"));
+		return -1;
 	}

 	packet_trace(buf, size, 1);
@@ -221,22 +219,28 @@ static int do_packet_write(const int fd_out, const char *buf, size_t size,

 	if (write_in_full(fd_out, header, 4) < 0 ||
 	    write_in_full(fd_out, buf, size) < 0) {
-		if (gentle)
-			return error_errno(_("packet write failed"));
-		else
-			die_errno(_("packet write failed"));
+		strbuf_addf(err, _("packet write failed: %s"), strerror(errno));
+		return -1;
 	}
 	return 0;
 }

 static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 {
-	return do_packet_write(fd_out, buf, size, 1);
+	struct strbuf err = STRBUF_INIT;
+	if (do_packet_write(fd_out, buf, size, &err)) {
+		error("%s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
+	return 0;
 }

 void packet_write(int fd_out, const char *buf, size_t size)
 {
-	do_packet_write(fd_out, buf, size, 0);
+	struct strbuf err = STRBUF_INIT;
+	if (do_packet_write(fd_out, buf, size, &err))
+		die("%s", err.buf);
 }

 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)

Re: [PATCH] pkt-line: do not report packet write errors twice

[Junio C Hamano]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> On Thu, Apr 15, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:
>>
>> > Nice! :) Maybe we could also avoid the static strings without
>> > repeating the literals by making `do_packet_write()` receive a `struct
>> > strbuf *err` and save the error message in it? Then the two callers
>> > can decide whether to pass it to error() or die() accordingly.
>>
>> Sorry, but I do not understand what benefit we are trying to gain by
>> introducing an extra strbuf (which I assume is used to strbuf_add()
>> the error message into).  Wouldn't the caller need two messages and
>> flip between <error,error_errno> vs <die,die_errno> pair?
>
> Hmm, I'm not sure I understand what you mean by the two messages, but
> what I was thinking of is something like this (on top of my original
> patch):

Ah, OK, you meant that you'd make do_packet_write() handle
die/error_errno itself by manually calling strerror(errno).

One small downside with the approach is that do_packet_write() needs
to hardcode how die/error_errno() mixes the strerror() with the
caller supplied error message, but I do not care too strongly either
way.  My original motivation of suggesting the rewrite was to avoid
overlong lines in the main flow of the code by replacing the long
messages with variable names that are much shorter, so as long as
that is done, I am fine either way.

Thanks.

[PATCH v2] pkt-line: do not report packet write errors twice

[Matheus Tavares]

On write() errors, packet_write() dies with the same error message that
is already printed by its callee, packet_write_gently(). This produces
an unnecessarily verbose and repetitive output:

error: packet write failed
fatal: packet write failed: <strerror() message>

In addition to that, packet_write_gently() does not always fulfill its
caller expectation that errno will be properly set before a non-zero
return. In particular, that is not the case for a "data exceeds max
packet size" error. So, in this case, packet_write() will call
die_errno() and print an strerror(errno) message that might be totally
unrelated to the actual error.

Fix both those issues by turning packet_write() and
packet_write_gently() into wrappers to a common lower level function
that doesn't print the error message, but instead returns it on a buffer
for the caller to die() or error() as appropriate.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 pkt-line.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 0194137528..98304ce374 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -194,13 +194,16 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	return status;
 }
 
-static int packet_write_gently(const int fd_out, const char *buf, size_t size)
+static int do_packet_write(const int fd_out, const char *buf, size_t size,
+			   struct strbuf *err)
 {
 	char header[4];
 	size_t packet_size;
 
-	if (size > LARGE_PACKET_DATA_MAX)
-		return error(_("packet write failed - data exceeds max packet size"));
+	if (size > LARGE_PACKET_DATA_MAX) {
+		strbuf_addstr(err, _("packet write failed - data exceeds max packet size"));
+		return -1;
+	}
 
 	packet_trace(buf, size, 1);
 	packet_size = size + 4;
@@ -215,15 +218,29 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 	 */
 
 	if (write_in_full(fd_out, header, 4) < 0 ||
-	    write_in_full(fd_out, buf, size) < 0)
-		return error(_("packet write failed"));
+	    write_in_full(fd_out, buf, size) < 0) {
+		strbuf_addf(err, _("packet write failed: %s"), strerror(errno));
+		return -1;
+	}
+	return 0;
+}
+
+static int packet_write_gently(const int fd_out, const char *buf, size_t size)
+{
+	struct strbuf err = STRBUF_INIT;
+	if (do_packet_write(fd_out, buf, size, &err)) {
+		error("%s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
 	return 0;
 }
 
 void packet_write(int fd_out, const char *buf, size_t size)
 {
-	if (packet_write_gently(fd_out, buf, size))
-		die_errno(_("packet write failed"));
+	struct strbuf err = STRBUF_INIT;
+	if (do_packet_write(fd_out, buf, size, &err))
+		die("%s", err.buf);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
-- 
2.30.1

Re: [PATCH v2] pkt-line: do not report packet write errors twice

[Junio C Hamano]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> On write() errors, packet_write() dies with the same error message that
> is already printed by its callee, packet_write_gently(). This produces
> an unnecessarily verbose and repetitive output:
>
> error: packet write failed
> fatal: packet write failed: <strerror() message>
>
> In addition to that, packet_write_gently() does not always fulfill its
> caller expectation that errno will be properly set before a non-zero
> return. In particular, that is not the case for a "data exceeds max
> packet size" error. So, in this case, packet_write() will call
> die_errno() and print an strerror(errno) message that might be totally
> unrelated to the actual error.
>
> Fix both those issues by turning packet_write() and
> packet_write_gently() into wrappers to a common lower level function
> that doesn't print the error message, but instead returns it on a buffer
> for the caller to die() or error() as appropriate.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  pkt-line.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)

Nicely done.  Thanks, will queue.

>
> diff --git a/pkt-line.c b/pkt-line.c
> index 0194137528..98304ce374 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -194,13 +194,16 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>  	return status;
>  }
>  
> -static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +static int do_packet_write(const int fd_out, const char *buf, size_t size,
> +			   struct strbuf *err)
>  {
>  	char header[4];
>  	size_t packet_size;
>  
> -	if (size > LARGE_PACKET_DATA_MAX)
> -		return error(_("packet write failed - data exceeds max packet size"));
> +	if (size > LARGE_PACKET_DATA_MAX) {
> +		strbuf_addstr(err, _("packet write failed - data exceeds max packet size"));
> +		return -1;
> +	}
>  
>  	packet_trace(buf, size, 1);
>  	packet_size = size + 4;
> @@ -215,15 +218,29 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>  	 */
>  
>  	if (write_in_full(fd_out, header, 4) < 0 ||
> -	    write_in_full(fd_out, buf, size) < 0)
> -		return error(_("packet write failed"));
> +	    write_in_full(fd_out, buf, size) < 0) {
> +		strbuf_addf(err, _("packet write failed: %s"), strerror(errno));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +{
> +	struct strbuf err = STRBUF_INIT;
> +	if (do_packet_write(fd_out, buf, size, &err)) {
> +		error("%s", err.buf);
> +		strbuf_release(&err);
> +		return -1;
> +	}
>  	return 0;
>  }
>  
>  void packet_write(int fd_out, const char *buf, size_t size)
>  {
> -	if (packet_write_gently(fd_out, buf, size))
> -		die_errno(_("packet write failed"));
> +	struct strbuf err = STRBUF_INIT;
> +	if (do_packet_write(fd_out, buf, size, &err))
> +		die("%s", err.buf);
>  }
>  
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...)