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

> On 15 Sep 2016, at 21:44, Jeff King <peff@peff.net> wrote:
> 
> 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).