Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C
- From
Alban Gruin <alban.gruin@gmail.com>
- Date
- Aug 9, 2018, 13:30 UTC
- Message-ID
- <bc07317b-ba44-7a3c-7cb4-504b7dbc7c34@gmail.com>
- In-Reply-To
- <75549397-9080-3b5a-8655-cea04065b2eb@talktalk.net>
Hi Phillip,
Le 08/08/2018 à 18:04, Phillip Wood a écrit :
Show 36 quoted lines
>>>> +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().
Cheers, Alban