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

Le 12/10/2018 à 11:54, Phillip Wood a écrit :

On 11/10/2018 17:57, Alban Gruin wrote:

quoted

Hi Phillip,

thanks for taking the time to review my patches.

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

quoted

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.

Another way would be to use the strbuf as a string pool rather than a
copy of the text of the file. There could be a write_todo_list()
function that takes a todo list and some flags, iterates over the items
in the todo list and writes the file. The flags would specify whether to
append the todo help and whether to abbreviate the object ids (so there
is no need for a separate call to transform_todos()). Then
add_exec_commands() could allocate a new array of todo items which it
builds up as it works through the original list and replaces the
original list with the new one at the end. The text of the exec items
can be added to the end of the strbuf (we only need one copy of the exec
text with this scheme). rearrange_squash() can use a temporary array to
build a new list as well or just memmove() things but that might be
slower if there is a lot of rearrangement to do. skip_unecessary_picks()
could just set the current item to the one we want to start with.

This sounds good, and it looks like the solution dscho proposed on IRC a few 
hours ago[0].  I will try to do this.

[0] http://colabti.org/irclogger/irclogger_log/git-devel?date=2018-10-12#l46

Cheers,
Alban