> On 15 Sep 2016, at 21:44, Jeff King <peff@xxxxxxxx> 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).
OK. I'll go with 1. then.
Thanks,
Lars