Re: [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
From: Phillip Wood <hidden>
Date: 2018-10-11 11:25:40
On 07/10/2018 20:54, Alban Gruin wrote:
quoted hunk ↗ jump to hunk
This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. 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. Signed-off-by: Alban Gruin <redacted> --- sequencer.c | 56 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 23 deletions(-)diff --git a/sequencer.c b/sequencer.c index 8dda61927c..6d998f21a4 100644 --- a/sequencer.c +++ b/sequencer.c@@ -4370,34 +4370,21 @@ int sequencer_make_script(FILE *out, int argc, const char **argv, return 0; } -/* - * Add commands after pick and (series of) squash/fixup commands - * in the todo list. - */ -int sequencer_add_exec_commands(const char *commands) +static void todo_list_add_exec_commands(struct todo_list *todo_list, + const char *commands) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - struct strbuf *buf = &todo_list.buf; + struct strbuf *buf = &todo_list->buf; size_t offset = 0, commands_len = strlen(commands); int i, insert; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); - - if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); - } - /* * 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. */ insert = -1; - for (i = 0; i < todo_list.nr; i++) { - enum todo_command command = todo_list.items[i].command; + for (i = 0; i < todo_list->nr; i++) { + enum todo_command command = todo_list->items[i].command; if (insert >= 0) { /* skip fixup/squash chains */@@ -4408,7 +4395,7 @@ int sequencer_add_exec_commands(const char *commands) continue; } strbuf_insert(buf, - todo_list.items[insert].offset_in_buf + + todo_list->items[insert].offset_in_buf + offset, commands, commands_len); offset += commands_len; insert = -1;@@ -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 Best Wishes Phillip
+
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+ const char *todo_file = rebase_path_todo();
+ struct todo_list todo_list = TODO_LIST_INIT;
+ int res;
+
+ if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+ return error(_("could not read '%s'."), todo_file);
+
+ if (todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
+ todo_list_release(&todo_list);
+ return error(_("unusable todo list: '%s'"), todo_file);
+ }
+
+ todo_list_add_exec_commands(&todo_list, commands);
+ res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0);
todo_list_release(&todo_list);
- return i;
+
+ return res;
}
int transform_todos(unsigned flags)