Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C

> Hi Alban
> 
> On 09/08/18 14:30, Alban Gruin wrote:
>> Hi Phillip,
>>
>> Le 08/08/2018 à 18:04, Phillip Wood a écrit :
>>>>>> +int edit_todo_list(unsigned flags)
>>>>>> +{
>>>>>> +    struct strbuf buf = STRBUF_INIT;
>>>>>> +    const char *todo_file = rebase_path_todo();
>>>>>> +    FILE *todo;
>>>>>> +
>>>>>> +    if (strbuf_read_file(&buf, todo_file, 0) < 0)
>>>>>> +        return error_errno(_("could not read '%s'."), todo_file);
>>>>>> +
>>>>>> +    strbuf_stripspace(&buf, 1);
>>>>>> +    todo = fopen_or_warn(todo_file, "w");
>>>>>
>>>>> This truncates the existing file, if there are any errors writing the
>>>>> new one then the user has lost the old one. write_message() in
>>>>> sequencer.c avoids this problem by writing a new file and then 
>>>>> renaming
>>>>> it if the write is successful, maybe it is worth exporting it so it 
>>>>> can
>>>>> be used elsewhere.
>>>>>
>>>>>> +    if (!todo) {
>>>>>> +        strbuf_release(&buf);
>>>>>> +        return 1;
>>>>>> +    }
>>>>>> +
>>>>>> +    strbuf_write(&buf, todo);
>>>>>> +    fclose(todo);
>>>>>
>>>>> There needs to be some error checking after the write and the close
>>>>> (using write_message() would mean you only have to check for errors in
>>>>> one place)
>>>>>
>>>>
>>>> Right.  Should I find a new nawe for write_message()?
>>>
>>> That might be a good idea, I'm not sure what it should be though, maybe
>>> write_file()?, perhaps someone else might have a better suggestion.
>>>
>>
>> write_file() already exists in wrapper.c.  I wondered, as this make use
>> of the lockfile API, perhaps it could be moved to lockfile.{c,h}, and
>> renamed to something like write_file_with_lock().
> 
> Hmm, I'm not sure about that, to me the use of the lockfile api is just 
> a way to get the semantics of an atomic update to the file. Looking in 
> wrapper.c there is write_file_buf() which could maybe be changed to to 
> an atomic update. Looking at the current callers to that function they 
> look like they would work with such a change (sadly none of them bother 
> to check the return value to see if the write was successful). I don't 
> think we need to worry about them being broken by having a stale 
> lockfile left over if there's an error as I think the lockfile api 
> automatically cleans them up but I'm not an expert on that api so it 
> would be worth checking with someone like Johannes or Jeff King (I seem 
> to remember him working in that area relatively recently).

> Best Wishes
> 
> Phillip
> 
> 
>> Cheers,
>> Alban
>>
>