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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

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.