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

From: Alban Gruin <alban.gruin@gmail.com>
Date: Oct 11, 2018, 16:57 UTC
Message-ID: <6c55857a-ce3c-f9fc-18c6-d7cef2976931@gmail.com>
In-Reply-To: <6188911e-fd21-4c7c-885d-a15d2b46aa86@talktalk.net>

Hi Phillip,

thanks for taking the time to review my patches.

Le 11/10/2018 à 13:25, Phillip Wood a écrit :

Show 20 quoted lines

> 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