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

[Junio C Hamano <gitster@pobox.com>]

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;
 }