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

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:

quoted

@@ -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