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
