Re: [PATCH v6 07/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list

[Alban Gruin <alban.gruin@gmail.com>]

Le 01/02/2019 à 15:51, Phillip Wood a écrit :
>>> @@ -4473,38 +4475,62 @@ int sequencer_add_exec_commands(struct
>>> repository *r,
>>>       * those chains if there are any.
>>>       */
>>>      insert = -1;
>>> -    for (i = 0; i < todo_list.nr; i++) {
>>> -        enum todo_command command = todo_list.items[i].command;
>>> -
>>> -        if (insert >= 0) {
>>> -            /* skip fixup/squash chains */
>>> -            if (command == TODO_COMMENT)
>>> -                continue;
>>> -            else if (is_fixup(command)) {
>>> -                insert = i + 1;
>>> -                continue;
>>> -            }
>>> -            strbuf_insert(buf,
>>> -                      todo_list.items[insert].offset_in_buf +
>>> -                      offset, commands, commands_len);
>>
>> In a todo list that looks like
>> pick abc message
>> #pick cde empty commit
>> This inserts the exec command for the first pick above the commented
>> out pick. I think your translation puts it below the commented out
>> pick as it ignores the value of insert. I think it's probably easiest
>> to add an INSERT_ARRAY macro to insert it in the right place. An
>> alternative might be to track the last insert position and only copy
>> commands across when there is another exec to insert but that might
>> get complicated in cases such as
>>
>> pick abc message
>> #squash cde squash! message //empty commit for rewording
>> fixup 123 fixup! message
>> #pick 456 empty commit
> 
> Thinking about this, I'm not sure it can happen as the empty squash
> commit will be commented out before rearrange_squash() is called so I
> think it would actually look like
> 
> pick abc message
> fixup 123 fixup! message
> #pick cde squash! message
> #pick 456 empty commit
> 
> So I wonder if we can get away with treating insert as a flag and
> inserting exec commands when
> insert && command == TODO_COMMENT
> 
> We could probably do with some tests for this to be sure.
> 

I will try this approach.

-- Alban

> Best Wishes
> 
> Phillip
>