Re: [PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Hi, On Fri, 30 Nov 2018, Phillip Wood wrote: > > diff --git a/sequencer.c b/sequencer.c > > index 900899ef20..11692d0b98 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -4394,24 +4394,29 @@ 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, > > + struct string_list *commands) > > { > > - const char *todo_file = rebase_path_todo(); > > - struct todo_list todo_list = TODO_LIST_INIT; > > - struct strbuf *buf = _list.buf; > > - size_t offset = 0, commands_len = strlen(commands); > > - int i, insert; > > + struct strbuf *buf = _list->buf; > > + const char *old_buf = buf->buf; > > + size_t base_length = buf->len; > > + int i, insert, nr = 0, alloc = 0; > > + struct todo_item *items = NULL, *base_items = NULL; > > > > - if (strbuf_read_file(_list.buf, todo_file, 0) < 0) > > - return error(_("could not read '%s'."), todo_file); > > + for (i = 0; i < commands->nr; ++i) { > > + strbuf_addstr(buf, commands->items[i].string); > > + strbuf_addch(buf, '\n'); > > + } > > This could cause buf->buf to be reallocated in which case all the > todo_list_item.arg pointers will be invalidated. I guess it is a good time for me to admit that the `arg` idea was not my best. Maybe it would be good to convert that from a pointer to an offset, e.g. `size_t arg_offset_in_buf;`? Or maybe we should just drop the `_in_buf` suffix and clarify in a comment next to the declaration of the fields that they are offsets in the strbuf? Ciao, Dscho
Re: [PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Hi Alban Sorry it has taken me a while to look at the latest iteration. I like the changes to pass a list of strings for the exec commands. I've only had a chance to take a quick look, but I've got a couple of comments below On 09/11/2018 08:07, Alban Gruin wrote: This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. 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(). todo_list_add_exec_commands() and sequencer_add_exec_commands() are modified to take a string list instead of a string -- one item for each command. This makes it easier to insert a new command to the todo list for each command to execute. sequencer_add_exec_commands() still reads the todo list from the disk, as it is needed by rebase -p. complete_action() still uses sequencer_add_exec_commands() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 15 +++-- sequencer.c | 111 +- sequencer.h | 4 +- 3 files changed, 83 insertions(+), 47 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index c1d87c0fe6..1fb5a43c0d 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -65,7 +65,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *onto, const char *onto_name, const char *squash_onto, const char *head_name, const char *restrict_revision, char *raw_strategies, -const char *cmd, unsigned autosquash) +struct string_list *commands, unsigned autosquash) { int ret; const char *head_hash = NULL; @@ -115,7 +115,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, else { discard_cache(); ret = complete_action(opts, flags, shortrevisions, onto_name, onto, - head_hash, cmd, autosquash); + head_hash, commands, autosquash); } free(revisions); @@ -138,6 +138,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL, *squash_onto = NULL, *upstream = NULL, *head_name = NULL, *switch_to = NULL, *cmd = NULL; + struct string_list commands = STRING_LIST_INIT_DUP; char *raw_strategies = NULL; enum { NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH, @@ -220,6 +221,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) warning(_("--[no-]rebase-cousins has no effect without " "--rebase-merges")); + if (cmd && *cmd) { + string_list_split(, cmd, '\n', -1); + if (strlen(commands.items[commands.nr - 1].string) == 0) + --commands.nr; + } + switch (command) { case NONE: if (!onto && !upstream) @@ -227,7 +234,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = do_interactive_rebase(, flags, switch_to, upstream, onto, onto_name, squash_onto, head_name, restrict_revision, - raw_strategies, cmd, autosquash); + raw_strategies, , autosquash); break; case SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; @@ -261,7 +268,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = rearrange_squash(); break; case ADD_EXEC: - ret = sequencer_add_exec_commands(cmd); + ret = sequencer_add_exec_commands(); break; default: BUG("invalid command '%d'", command); diff --git a/sequencer.c b/sequencer.c index 900899ef20..11692d0b98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4394,24 +4394,29 @@ 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, +
[PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. 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(). todo_list_add_exec_commands() and sequencer_add_exec_commands() are modified to take a string list instead of a string -- one item for each command. This makes it easier to insert a new command to the todo list for each command to execute. sequencer_add_exec_commands() still reads the todo list from the disk, as it is needed by rebase -p. complete_action() still uses sequencer_add_exec_commands() for now. This will be changed in a future commit. Signed-off-by: Alban Gruin --- builtin/rebase--interactive.c | 15 +++-- sequencer.c | 111 +- sequencer.h | 4 +- 3 files changed, 83 insertions(+), 47 deletions(-) diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index c1d87c0fe6..1fb5a43c0d 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -65,7 +65,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, const char *onto, const char *onto_name, const char *squash_onto, const char *head_name, const char *restrict_revision, char *raw_strategies, -const char *cmd, unsigned autosquash) +struct string_list *commands, unsigned autosquash) { int ret; const char *head_hash = NULL; @@ -115,7 +115,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, else { discard_cache(); ret = complete_action(opts, flags, shortrevisions, onto_name, onto, - head_hash, cmd, autosquash); + head_hash, commands, autosquash); } free(revisions); @@ -138,6 +138,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL, *squash_onto = NULL, *upstream = NULL, *head_name = NULL, *switch_to = NULL, *cmd = NULL; + struct string_list commands = STRING_LIST_INIT_DUP; char *raw_strategies = NULL; enum { NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH, @@ -220,6 +221,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) warning(_("--[no-]rebase-cousins has no effect without " "--rebase-merges")); + if (cmd && *cmd) { + string_list_split(, cmd, '\n', -1); + if (strlen(commands.items[commands.nr - 1].string) == 0) + --commands.nr; + } + switch (command) { case NONE: if (!onto && !upstream) @@ -227,7 +234,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = do_interactive_rebase(, flags, switch_to, upstream, onto, onto_name, squash_onto, head_name, restrict_revision, - raw_strategies, cmd, autosquash); + raw_strategies, , autosquash); break; case SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; @@ -261,7 +268,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = rearrange_squash(); break; case ADD_EXEC: - ret = sequencer_add_exec_commands(cmd); + ret = sequencer_add_exec_commands(); break; default: BUG("invalid command '%d'", command); diff --git a/sequencer.c b/sequencer.c index 900899ef20..11692d0b98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4394,24 +4394,29 @@ 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, + struct string_list *commands) { - const char *todo_file = rebase_path_todo(); - struct todo_list todo_list = TODO_LIST_INIT; - struct strbuf *buf = _list.buf; - size_t offset = 0,