Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Aug 7, 2018, 17:31 UTC
Message-ID: <1424a96b-26e8-aaa8-8e3c-ed84d58c26af@talktalk.net>
In-Reply-To: <CAP8UFD2_oxn=auXb1tYCb5xED5YmKU3W=fCrpMsyxdZBfs2qcw@mail.gmail.com>

Hi Christian On 07/08/18 17:28, Christian Couder wrote:

Show 14 quoted lines

> On Tue, Aug 7, 2018 at 6:15 PM, Phillip Wood <phillip.wood@talktalk.net> wrote:
>> On 07/08/18 16:25, Christian Couder wrote:
>>>
>>> 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.
> 
> I vaguely remember we tried to check those return values but it didn't
> work well sometimes.
>

I think there's no point in checking the return value after a successful read, or when writing and the file is being closed (and then possibly removed) in some clean-up in an error path, but if the code cares about ensuring the data is written then I think it should be checking the return value, close_tempfile_gently() does for example.

In the sequencer code there are some examples where it probably should be checking the return value, and places such as rewrite_file() and write_message() (which uses the lockfile api) where the return code is checked (there are a couple of recently added callers of write_message() in the code to recreate octopus merges that don't check its return value but all the other callers do).

Best Wishes

Phillip