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.
Best Wishes
Phillip
>
>> Best Wishes
>>
>> Phillip
>>
>
> Cheers,
> Alban
>
