From: Lars Schneider Date: Thu, 15 Sep 2016 16:42:58 GMT Subject: Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently() Message-ID: <385016D3-8298-4273-81E7-876CB25B2789@gmail.com> In-Reply-To: > On 13 Sep 2016, at 23:44, Junio C Hamano wrote: > > Lars Schneider writes: > >>> On 13 Sep 2016, at 00:30, Junio C Hamano wrote: >>> >>> larsxschneider@gmail.com writes: >>> >>>> From: Lars Schneider >>>> >>>> packet_flush() would die in case of a write error even though for some >>>> callers an error would be acceptable. Add packet_flush_gently() which >>>> writes a pkt-line flush packet and returns `0` for success and `-1` for >>>> failure. >>>> ... >>>> +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; >>> >>> It is more idiomatic to do >>> >>> return error(...); >>> >>> but more importantly, does the caller even want an error message >>> unconditionally printed here? >>> >>> 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? Thanks, Lars