From: Phillip Wood <phillip.wood@talktalk.net>
Date: Tue, 07 Aug 2018 16:15:48 GMT
Subject: Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Message-ID: <6832700d-d7be-1df1-8b24-6e0290de63ed@talktalk.net>
In-Reply-To: <CAP8UFD3KbDrvU3zj24F7FF9ui2X75Vih4CoYE=cCZEWMsKP9Fw@mail.gmail.com>

Hi Christian
On 07/08/18 16:25, Christian Couder wrote:
> Hi Phillip,
> 
> On Tue, Aug 7, 2018 at 3:57 PM, Phillip Wood <phillip.wood@talktalk.net> wrote:
>>
>> On 31/07/18 18:59, Alban Gruin wrote:
>>>
>>> +
>>> +     ret = fputs(buf.buf, todo);
>>
>> It is not worth changing the patch just for this but strbuf_write()
>> might be clearer (you use it in a later patch)
>>
>>> +     if (ret < 0)
>>> +             error_errno(_("could not append help text to '%s'"), rebase_path_todo());
>>> +
>>> +     fclose(todo);
>>
>> You should definitely check the return value and return an error if
>> appropriate as fputs() might not actually write any data until you try
>> and close the file.
> 
> I agree about checking the return value from fputs(), but it seems to
> me that we don't usually check the value of fclose().

A quick grep shows you're right, there are only a handful of places 
where the return value of fclose() is checked (there aren't many checks 
for the return value of close() either), I'm don't think that is safe 
though given that write errors may only show up when the file gets 
flushed by closing it.

Best Wishes

Phillip

> Thanks,
> Christian.
>