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