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

[Alban Gruin <alban.gruin@gmail.com>]

Hi Phillip,

Le 30/10/2018 à 17:28, Phillip Wood a écrit :
> 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.

> […]
>> +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