From: Phillip Wood <phillip.wood@talktalk.net>
Date: Wed, 08 Aug 2018 16:04:04 GMT
Subject: Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C
Message-ID: <75549397-9080-3b5a-8655-cea04065b2eb@talktalk.net>
In-Reply-To: <6826d318-b0ab-4e8e-e2ba-5425e3bf6f5f@gmail.com>

On 08/08/18 16:17, Alban Gruin wrote:
> Hi Phillip,
> 
> Le 07/08/2018 à 16:00, Phillip Wood a écrit :
>> On 31/07/18 18:59, Alban Gruin wrote:
>>> +
>>> +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.

Best Wishes

Phillip

> 
> Cheers,
> Alban
>