Re: [PATCH v2 04/16] sequencer: introduce todo_list_write_to_file()

From: Alban Gruin <alban.gruin@gmail.com>
Date: Nov 1, 2018, 23:31 UTC
Message-ID: <8b235228-fa53-cf5d-6ece-b28d030e1751@gmail.com>
In-Reply-To: <03475c29-5317-b105-6102-5cae3a5ae926@talktalk.net>

Hi Phillip,

Le 30/10/2018 à 17:28, Phillip Wood a écrit :

Show 37 quoted lines

> Hi Alban
> 
> I like the direction this is going, it is an improvement on re-scanning
> the list at the end of each function.
> 
> On 27/10/2018 22:29, Alban Gruin wrote:
>> This introduce a new function to recreate the text of a todo list from
>> its commands, and then to write it to the disk.  This will be useful in
>> the future, the buffer of a todo list won’t be treated as a strict
>> mirror of the todo file by some of its functions once they will be
>> refactored.
> 
> I'd suggest rewording this slightly, maybe something like
> 
> This introduces a new function to recreate the text of a todo list from
> its commands and write it to a file. This will be useful as the next few
> commits will change the use of the buffer in struct todo_list so it will
> no-longer be a mirror of the file on disk.
> 
>> This functionnality can already be found in todo_list_transform(), but
> 
> s/functionnality/functionality/
> 
>> it is specifically made to replace the buffer of a todo list, which is
>> not the desired behaviour.  Thus, the part of todo_list_transform() that
>> actually creates the buffer is moved to a new function,
>> todo_list_to_strbuf().  The rest is unused, and so is dropped.
>>
>> todo_list_write_to_file() can also take care to append the help text to
> 
> s/care to append/care of appending/
> 
>> the buffer before writing it to the disk, or to write only the first n
>> items of the list.
> 
> Why/when do we only want to write a subset of the items?
>

In skip_unnecessary_picks(), in patch [10/16]. It needs to write the elements of the todo list that were already done in the `done' file.

Show 13 quoted lines

> […]
>> +int todo_list_write_to_file(struct todo_list *todo_list, const char
>> *file,
>> +                const char *shortrevisions, const char *shortonto,
>> +                int command_count, int append_help, int num, unsigned
>> flags)
> 
> This is a really long argument list which makes it easy for callers to
> get the parameters in the wrong order. I think append_help could
> probably be folded into the flags, I'm not sure what the command_count
> is used for but I've only read the first few patches. Maybe it would be
> better to pass a struct so we have named fields.
>

You’re right, command_count is not really needed since we pass the complete todo list.

The only bit that irks me is that, if I stop passing command_count, I would have to call count_commands() twice in complete_action(): once to check if there are any commands in the todo list, and again inside of todo_list_write_to_file() (see [09/16].)

Perhaps I could move this check before calling todo_list_rearrange_squash()?

As a sidenote, this is not why I added command_count to the parameters of todo_list_write_to_file(). It was a confusion of my part.

> Best Wishes
> 
> Phillip
>

Cheers, Alban