On Thu, Apr 15, 2021 at 5:06 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Matheus Tavares <matheus.bernardino@xxxxxx> 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@xxxxxx>
> > ---
> > 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.
