> On 25 Aug 2016, at 23:41, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> larsxschneider@xxxxxxxxx writes:
>
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>>
>> packet_write_fmt() would die in case of a write error even though for
>> some callers an error would be acceptable. Add packet_write_fmt_gently()
>> which writes a formatted pkt-line and returns `0` for success and `-1`
>> for an error.
>>
>> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx>
>> ---
>> pkt-line.c | 12 ++++++++++++
>> pkt-line.h | 1 +
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/pkt-line.c b/pkt-line.c
>> index e8adc0f..3e8b2fb 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -137,6 +137,18 @@ void packet_write_fmt(int fd, const char *fmt, ...)
>> write_or_die(fd, buf.buf, buf.len);
>> }
>>
>> +int packet_write_fmt_gently(int fd, const char *fmt, ...)
>> +{
>> + static struct strbuf buf = STRBUF_INIT;
>> + va_list args;
>> +
>> + strbuf_reset(&buf);
>> + va_start(args, fmt);
>> + format_packet(&buf, fmt, args);
>> + va_end(args);
>> + return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
>> +}
>
> Even though its only a handful lines, it is a bit ugly to have a
> completely copied implementation only to have _gently(). I suspect
> that you should be able to
>
> static int packet_write_fmt_1(int fd, int gently,
> const char *fmt, va_list args)
> {
> struct strbuf buf = STRBUF_INIT;
> size_t count;
>
> format_packet(&buf, fmt, args);
>
> count = write_in_full(fd, buf.buf, buf.len);
> if (count == buf.len)
> return 0;
> if (!gently) {
> check_pipe(errno);
> die_errno("write error");
> }
> return -1;
> }
>
> and then share that between the existing one:
>
> void packet_write_fmt(int fd, const char *fmt, ...)
> {
> va_list args;
> va_start(args, fmt);
> packet_write_fmt_1(fd, 0, fmt, args);
> va_end(args);
> }
>
> and the new one:
>
> void packet_write_fmt_gently(int fd, const char *fmt, ...)
> {
> int status;
> va_list args;
> va_start(args, fmt);
> status = packet_write_fmt_1(fd, 1, fmt, args);
> va_end(args);
> return status;
> }
I agree with your criticism of the code duplication.
However, I thought it would be OK, as Peff already
tried to refactor it...
http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f4sx@xxxxxxxxxxxxxxxxxxxxx/
... and I got the impression you agreed with Peff:
http://public-inbox.org/git/xmqqvaz84g9y.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx/
I will try to refactor it according to your suggestion above.
Would "packet_write_fmt_1()" be an acceptable name or should
I come up with something more expressive?
Thanks you,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html