Hi Alban
On 01/11/2018 23:31, Alban Gruin wrote:
> Le 30/10/2018 à 17:47, Phillip Wood a écrit :
>> On 27/10/2018 22:29, Alban Gruin wrote:
>>> This refactors sequencer_add_exec_commands() to work on a todo_list to
>>> avoid redundant reads and writes to the disk.
>>>
>>> An obvious way to do this would be to insert the `exec' command between
>>> the other commands, and reparse it once this is done. This is not what
>>> is done here. Instead, the command is appended to the buffer once, and
>>> a new list of items is created. Items from the old list are copied to
>>> it, and new `exec' items are appended when necessary. This eliminates
>>> the need to reparse the todo list, but this also means its buffer cannot
>>> be directly written to the disk, hence todo_list_write_to_disk().
>>
>> I'd reword this slightly, maybe
>>
>> Instead of just inserting the `exec' command between the other commands,
>> and re-parsing the buffer at the end the exec command is appended to the
>> buffer once, and a new list of items is created. Items from the old
>> list are copied across and new `exec' items are appended when necessary.
>> This eliminates the need to reparse the buffer, but this also means we
>> have to use todo_list_write_to_disk() to write the file.
>>
>>> sequencer_add_exec_commands() still reads the todo list from the disk,
>>> as it is needed by rebase -p. todo_list_add_exec_commands() works on a
>>> todo_list structure, and reparses it at the end.
>>
>> I think the saying 'reparses' is confusing as that is what we're trying
>> to avoid.
>>
>>> complete_action() still uses sequencer_add_exec_commands() for now.
>>> This will be changed in a future commit.
>>>
>>> Signed-off-by: Alban Gruin <alban.gruin@xxxxxxxxx>
>>> ---
>>> sequencer.c | 69 +++++++++++++++++++++++++++++++++++++----------------
>>> 1 file changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index e12860c047..12a3efeca8 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -4216,6 +4216,50 @@ int sequencer_make_script(FILE *out, int argc,
>>> const char **argv,
>>> return 0;
>>> }
>>> +static void todo_list_add_exec_commands(struct todo_list *todo_list,
>>> + const char *commands)
>>> +{
>>> + struct strbuf *buf = &todo_list->buf;
>>> + const char *old_buf = buf->buf;
>>> + size_t commands_len = strlen(commands + strlen("exec ")) - 1;
>>> + int i, first = 1, nr = 0, alloc = 0;
>>
>> Minor nit pick, I think it is clearer if first is initialized just
>> before the loop as it is in the deleted code below.
>>
>>> + struct todo_item *items = NULL,
>>> + base_item = {TODO_EXEC, NULL, 0, 0, commands_len, 0};
>>> +
>>> + strbuf_addstr(buf, commands);
>>> + base_item.offset_in_buf = buf->len - commands_len - 1;
>>> + base_item.arg = buf->buf + base_item.offset_in_buf;
>>
>> I think if the user gives --exec more than once on the command line then
>> commands will contain more than one exec command so this needs to parse
>> commands and create one todo_item for each command.
>>
>
> Ouch, you’re right. Thanks for the heads up.
I haven't looked how difficult it would be but it might be best to
change the option parsing to pass an array of strings containing the
exec commands rather than one long string so we can just loop over the
array here.
>
>>> +
>>> + /*
>>> + * Insert <commands> after every pick. Here, fixup/squash chains
>>> + * are considered part of the pick, so we insert the commands
>>> *after*
>>> + * those chains if there are any.
>>> + */
>>> + for (i = 0; i < todo_list->nr; i++) {
>>> + enum todo_command command = todo_list->items[i].command;
>>> + if (todo_list->items[i].arg)
>>> + todo_list->items[i].arg = todo_list->items[i].arg -
>>> old_buf + buf->buf;
>>> +
>>> + if (command == TODO_PICK && !first) {
>>> + ALLOC_GROW(items, nr + 1, alloc);
>>> + memcpy(items + nr++, &base_item, sizeof(struct todo_item));
>>
>> I think it would be clearer to say
>> items[nr++] = base_item;
>> rather than using memcpy. This applies below and to some of the other
>> patches as well. Also this needs to loop over all the base_items if the
>> user gave --exec more than once on the command line.
>>
>
> I agree with you, it’s way more readable, IMO. But for some reason, I
> thought it was not possible to assign a struct to another in C.
In general one needs to be careful as it is only does a shallow copy but
that is exactly what we want here. Having said that if we have an array
of exec commands to insert then it might be worth sticking with memcpy()
here so the whole array can be copied in one go.
Best Wishes
Phillip
>> Best Wishes
>>
>> Phillip
>>
>
> Cheers,
> Alban
>
