Re: [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list

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

Le 12/10/2018 à 11:54, Phillip Wood a écrit :
> On 11/10/2018 17:57, Alban Gruin wrote:
> > Hi Phillip,
> > 
> > thanks for taking the time to review my patches.
> > 
> > Le 11/10/2018 à 13:25, Phillip Wood a écrit :
> >> On 07/10/2018 20:54, Alban Gruin wrote:
> >>> @@ -4419,15 +4406,38 @@ int sequencer_add_exec_commands(const char
> >>> *commands)
> >>>       }
> >>>         /* insert or append final <commands> */
> >>> -    if (insert >= 0 && insert < todo_list.nr)
> >>> -        strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
> >>> +    if (insert >= 0 && insert < todo_list->nr)
> >>> +        strbuf_insert(buf, todo_list->items[insert].offset_in_buf +
> >>>                     offset, commands, commands_len);
> >>>       else if (insert >= 0 || !offset)
> >>>           strbuf_add(buf, commands, commands_len);
> >>>   -    i = write_message(buf->buf, buf->len, todo_file, 0);
> >>> +    if (todo_list_parse_insn_buffer(buf->buf, todo_list))
> >>> +        BUG("unusable todo list");}
> >> 
> >> It is a shame to have to re-parse the todo list, I wonder how difficult
> >> it would be to adjust the todo_list item array as the exec commands are
> >> inserted. The same applies to the next couple of patches
> > 
> > Good question.
> > 
> > This function inserts an `exec' command after every `pick' command.
> > These commands are stored in a dynamically allocated list, grew with
> > ALLOW_GROW().
> > 
> > If we want to keep the current structure, we would have to grow the size
> > of the list by 1 and move several element to the end every time we want
> > to add an `exec' command.  It would not be very effective.  Perhaps I
> > should use a linked list here, instead.  It may also work well with
> > rearrange_squash() and skip_unnecessary_picks().
> > 
> > Maybe we could even get rid of the strbuf at some point.
> 
> Another way would be to use the strbuf as a string pool rather than a
> copy of the text of the file. There could be a write_todo_list()
> function that takes a todo list and some flags, iterates over the items
> in the todo list and writes the file. The flags would specify whether to
> append the todo help and whether to abbreviate the object ids (so there
> is no need for a separate call to transform_todos()). Then
> add_exec_commands() could allocate a new array of todo items which it
> builds up as it works through the original list and replaces the
> original list with the new one at the end. The text of the exec items
> can be added to the end of the strbuf (we only need one copy of the exec
> text with this scheme). rearrange_squash() can use a temporary array to
> build a new list as well or just memmove() things but that might be
> slower if there is a lot of rearrangement to do. skip_unecessary_picks()
> could just set the current item to the one we want to start with.
> 

This sounds good, and it looks like the solution dscho proposed on IRC a few 
hours ago[0].  I will try to do this.

[0] http://colabti.org/irclogger/irclogger_log/git-devel?date=2018-10-12#l46

Cheers,
Alban