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

From: Jeff King <peff@peff.net>
Date: Sep 15, 2016, 19:44 UTC
Message-ID: <20160915194443.x7zvkkryvworqcxt@sigill.intra.peff.net>
In-Reply-To: <385016D3-8298-4273-81E7-876CB25B2789@gmail.com>

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

Show 25 quoted lines

> >>>> +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