From: Phillip Wood <phillip.wood@talktalk.net>
Date: Thu, 09 Aug 2018 16:09:35 GMT
Subject: Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C
Message-ID: <d8b203b6-6fbe-1751-3c85-a8e7991bd765@talktalk.net>
In-Reply-To: <329d89f1-5ade-aeec-40bc-cd7168822d3e@talktalk.net>

Hi Alban

On 09/08/18 16:35, Phillip Wood wrote:
> 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).

On second thoughts the current function dies rather than returning an 
error so it would be a bit of work to do the conversion properly. For 
now you could just make write_message() available outside sequencer.c 
and see if anyone comes up with a better name when v6 is reviewed.

Best Wishes

Phillip

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