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
