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

[Phillip Wood <phillip.wood@talktalk.net>]

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
>