Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()

[Jeff King <peff@peff.net>]

On Thu, Sep 15, 2016 at 05:42:58PM +0100, Lars Schneider wrote:

> >>>> +int packet_flush_gently(int fd)
> >>>> +{
> >>>> +	packet_trace("0000", 4, 1);
> >>>> +	if (write_in_full(fd, "0000", 4) == 4)
> >>>> +		return 0;
> >>>> +	error("flush packet write failed");
> >>>> +	return -1;
> [...]
> >>> I suspect that it is a strong sign that the caller wants to be in
> >>> control of when and what error message is produced; otherwise it
> >>> wouldn't be calling the _gently() variant, no?
> >> 
> >> Agreed!
> > 
> > I am also OK with the current form, too.  Those who need to enhance
> > it to packet_flush_gently(int fd, int quiet) can come later.
> 
> "caller wants to be in control [...] otherwise it wouldn't be calling 
> the _gently() variant" convinced me. I would like to change it like
> this:
> 
> 	trace_printf_key(&trace_packet, "flush packet write failed");
> 	return -1;
> 
> Objections?

I'm not sure that a trace makes sense, because it means that 99% of the
time we are silent. AFAICT, the question is not "sometimes the user
needs to see an error and sometimes not, and they should decide before
starting the program". It is "sometimes the caller will report the error
to the user as appropriate, and sometimes we need to do so". And only
the calling code knows which is which.

So the "right" pattern is either:

  1. Return -1 and the caller is responsible for telling the user.

or

  2. Return -1 and stuff the error into an error strbuf, so it can be
     passed up the call chain easily (and callers do not have to come up
     with their own wording).

But if all current callers would just call error() themselves anyway,
then it's OK to punt on this and let somebody else handle it later if
they add a new caller who wants different behavior (and that is what
Junio was saying above, I think).

-Peff