| Message ID | 20190210132648.12821-8-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: refactor functions working on a todo_list | expand |
Hi Alban I think this is almost there, I've got a couple of small comments below. On 10/02/2019 13:26, 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 inserting the `exec' commands between the other commands and > re-parsing the buffer at the end, they are 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 <alban.gruin@gmail.com> > --- > builtin/rebase--interactive.c | 15 +++-- > sequencer.c | 109 +++++++++++++++++++++------------- > sequencer.h | 5 +- > 3 files changed, 82 insertions(+), 47 deletions(-) > > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c > index df19ccaeb9..c131fd4a27 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; > @@ -116,7 +116,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, > discard_cache(); > ret = complete_action(the_repository, opts, flags, > shortrevisions, onto_name, onto, > - head_hash, cmd, autosquash); > + head_hash, commands, autosquash); > } > > free(revisions); > @@ -139,6 +139,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, > @@ -221,6 +222,11 @@ 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(&commands, cmd, '\n', -1); I'd suggest a comment along the lines of /* As cmd always ends with a newline the last item is empty */ > + --commands.nr; Style: commands.nr-- > + } > + > switch (command) { > case NONE: > if (!onto && !upstream) > @@ -228,7 +234,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) > > ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, > onto_name, squash_onto, head_name, restrict_revision, > - raw_strategies, cmd, autosquash); > + raw_strategies, &commands, autosquash); > break; > case SKIP: { > struct string_list merge_rr = STRING_LIST_INIT_DUP; > @@ -262,11 +268,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) > ret = rearrange_squash(the_repository); > break; > case ADD_EXEC: > - ret = sequencer_add_exec_commands(the_repository, cmd); > + ret = sequencer_add_exec_commands(the_repository, &commands); > break; > default: > BUG("invalid command '%d'", command); > } Before freeing the string list it would be good to reset the number of items with commands.nr++ (with a comment explaining why) so the NUL in the last item gets freed. > + string_list_clear(&commands, 1); As we don't use item.util I think the second argument would be better as 0. > return !!ret; > } > diff --git a/sequencer.c b/sequencer.c > index 99e12c751e..82ca3432cd 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4505,21 +4505,27 @@ int sequencer_make_script(struct repository *r, FILE *out, > * Add commands after pick and (series of) squash/fixup commands > * in the todo list. > */ > -int sequencer_add_exec_commands(struct repository *r, > - 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 = &todo_list.buf; > - size_t offset = 0, commands_len = strlen(commands); > - int i, insert; > + struct strbuf *buf = &todo_list->buf; > + size_t base_offset = buf->len; > + int i, insert, nr = 0, alloc = 0; > + struct todo_item *items = NULL, *base_items = NULL; > > - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > - return error(_("could not read '%s'."), todo_file); > + base_items = xcalloc(commands->nr, sizeof(struct todo_item)); > + for (i = 0; i < commands->nr; i++) { > + size_t command_len = strlen(commands->items[i].string); > > - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { > - todo_list_release(&todo_list); > - return error(_("unusable todo list: '%s'"), todo_file); > + strbuf_addstr(buf, commands->items[i].string); > + strbuf_addch(buf, '\n'); > + > + base_items[i].command = TODO_EXEC; > + base_items[i].offset_in_buf = base_offset; > + base_items[i].arg_offset = base_offset + strlen("exec "); > + base_items[i].arg_len = command_len - strlen("exec "); > + > + base_offset += command_len + 1; > } > > /* > @@ -4527,39 +4533,60 @@ int sequencer_add_exec_commands(struct repository *r, > * 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; > - > - 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); > - offset += commands_len; > - insert = -1; I like the simplification of using insert as a flag. Perhaps we should document the assumptions. Maybe something like We insert the exec commands immediately after rearranging any fixups and before the user edits the list. This means that a fixup chain can never contain comments (any comments are empty picks that have been commented out the the user did not specify --keep-empty) and so it is safe to insert the exec command without looking at the command following the comment. > + insert = 0; > + for (i = 0; i < todo_list->nr; i++) { > + enum todo_command command = todo_list->items[i].command; > + if (insert && !is_fixup(command)) { > + ALLOC_GROW(items, nr + commands->nr, alloc); > + COPY_ARRAY(items + nr, base_items, commands->nr); > + nr += commands->nr; > + > + insert = 0; > } > > - if (command == TODO_PICK || command == TODO_MERGE) > - insert = i + 1; > + ALLOC_GROW(items, nr + 1, alloc); > + items[nr++] = todo_list->items[i]; > + > + if (command == TODO_PICK || command == TODO_MERGE || is_fixup(command)) I'm not sure what the is_fixup() is for, if the command is a fixup then it will have been preceded by a pick or merge so insert will already be 1 I feel a bit mean suggesting a reroll when we're on v7 already but I think these clean-ups would improve the maintainability of the code. I'll take a look at the rest of the changes to this series sometime this week. Best Wishes Phillip > + insert = 1; > } > > /* insert or append final <commands> */ > - 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); > + if (insert || nr == todo_list->nr) { > + ALLOC_GROW(items, nr + commands->nr, alloc); > + COPY_ARRAY(items + nr, base_items, commands->nr); > + nr += commands->nr; > + } > + > + free(base_items); > + FREE_AND_NULL(todo_list->items); > + todo_list->items = items; > + todo_list->nr = nr; > + todo_list->alloc = alloc; > +} > > - i = write_message(buf->buf, buf->len, todo_file, 0); > +int sequencer_add_exec_commands(struct repository *r, > + struct string_list *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_errno(_("could not read '%s'."), todo_file); > + > + if (todo_list_parse_insn_buffer(r, 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 = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, 0); > todo_list_release(&todo_list); > - return i; > + > + if (res) > + return error_errno(_("could not write '%s'."), todo_file); > + return 0; > } > > static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list, > @@ -4790,7 +4817,7 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output > > int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, > const char *shortrevisions, const char *onto_name, > - const char *onto, const char *orig_head, const char *cmd, > + const char *onto, const char *orig_head, struct string_list *commands, > unsigned autosquash) > { > const char *shortonto, *todo_file = rebase_path_todo(); > @@ -4809,8 +4836,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > if (autosquash && rearrange_squash(r)) > return -1; > > - if (cmd && *cmd) > - sequencer_add_exec_commands(r, cmd); > + if (commands->nr) > + sequencer_add_exec_commands(r, commands); > > if (strbuf_read_file(buf, todo_file, 0) < 0) > return error_errno(_("could not read '%s'."), todo_file); > diff --git a/sequencer.h b/sequencer.h > index 217353e9f0..87d04a3b9b 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -146,12 +146,13 @@ int sequencer_make_script(struct repository *r, FILE *out, int argc, > const char **argv, > unsigned flags); > > -int sequencer_add_exec_commands(struct repository *r, const char *command); > +int sequencer_add_exec_commands(struct repository *r, > + struct string_list *commands); > int transform_todo_file(struct repository *r, unsigned flags); > int check_todo_list_from_file(struct repository *r); > int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, > const char *shortrevisions, const char *onto_name, > - const char *onto, const char *orig_head, const char *cmd, > + const char *onto, const char *orig_head, struct string_list *commands, > unsigned autosquash); > int rearrange_squash(struct repository *r); > >
Hi Phillip, Le 12/02/2019 à 11:52, Phillip Wood a écrit : > Hi Alban > > I think this is almost there, I've got a couple of small comments below. > > On 10/02/2019 13:26, Alban Gruin wrote: >> -%<- >> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c >> index df19ccaeb9..c131fd4a27 100644 >> --- a/builtin/rebase--interactive.c >> +++ b/builtin/rebase--interactive.c >> -%<- >> @@ -221,6 +222,11 @@ 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(&commands, cmd, '\n', -1); > > I'd suggest a comment along the lines of > /* As cmd always ends with a newline the last item is empty */ > >> + --commands.nr; > > Style: commands.nr-- > >> + } >> + >> switch (command) { >> case NONE: >> if (!onto && !upstream) >> @@ -228,7 +234,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) >> >> ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, >> onto_name, squash_onto, head_name, restrict_revision, >> - raw_strategies, cmd, autosquash); >> + raw_strategies, &commands, autosquash); >> break; >> case SKIP: { >> struct string_list merge_rr = STRING_LIST_INIT_DUP; >> @@ -262,11 +268,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) >> ret = rearrange_squash(the_repository); >> break; >> case ADD_EXEC: >> - ret = sequencer_add_exec_commands(the_repository, cmd); >> + ret = sequencer_add_exec_commands(the_repository, &commands); >> break; >> default: >> BUG("invalid command '%d'", command); >> } > > Before freeing the string list it would be good to reset the number of > items with commands.nr++ (with a comment explaining why) so the NUL in > the last item gets freed. > >> + string_list_clear(&commands, 1); > > As we don't use item.util I think the second argument would be better as 0. > >> return !!ret; >> } FWIW I just stumbled across string_list_remove_empty_items(), which seems to do exactly the same thing, but that way we don’t have to do this kind of hacks. >> diff --git a/sequencer.c b/sequencer.c >> index 99e12c751e..82ca3432cd 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -4505,21 +4505,27 @@ int sequencer_make_script(struct repository *r, FILE *out, >> * Add commands after pick and (series of) squash/fixup commands >> * in the todo list. >> */ >> -int sequencer_add_exec_commands(struct repository *r, >> - 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 = &todo_list.buf; >> - size_t offset = 0, commands_len = strlen(commands); >> - int i, insert; >> + struct strbuf *buf = &todo_list->buf; >> + size_t base_offset = buf->len; >> + int i, insert, nr = 0, alloc = 0; >> + struct todo_item *items = NULL, *base_items = NULL; >> >> - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) >> - return error(_("could not read '%s'."), todo_file); >> + base_items = xcalloc(commands->nr, sizeof(struct todo_item)); >> + for (i = 0; i < commands->nr; i++) { >> + size_t command_len = strlen(commands->items[i].string); >> >> - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { >> - todo_list_release(&todo_list); >> - return error(_("unusable todo list: '%s'"), todo_file); >> + strbuf_addstr(buf, commands->items[i].string); >> + strbuf_addch(buf, '\n'); >> + >> + base_items[i].command = TODO_EXEC; >> + base_items[i].offset_in_buf = base_offset; >> + base_items[i].arg_offset = base_offset + strlen("exec "); >> + base_items[i].arg_len = command_len - strlen("exec "); >> + >> + base_offset += command_len + 1; >> } >> >> /* >> @@ -4527,39 +4533,60 @@ int sequencer_add_exec_commands(struct repository *r, >> * 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; >> - >> - 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); >> - offset += commands_len; >> - insert = -1; > > I like the simplification of using insert as a flag. Perhaps we should > document the assumptions. Maybe something like > > We insert the exec commands immediately after rearranging any fixups and > before the user edits the list. This means that a fixup chain can never > contain comments (any comments are empty picks that have been commented > out the the user did not specify --keep-empty) and so it is safe to > insert the exec command without looking at the command following the > comment. > I slightly reworded this and added it to the existing comment just before the for loop: /* * 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. * * As we insert the exec commands immediatly after rearranging * any fixups and before the user edits the list, a fixup chain * can never contain comments (any comments are empty picks that * have been commented out because the user did not specify * --keep-empty). So, it is safe to insert an exec command * without looking at the command following a comment. */ >> + insert = 0; >> + for (i = 0; i < todo_list->nr; i++) { >> + enum todo_command command = todo_list->items[i].command; >> + if (insert && !is_fixup(command)) { >> + ALLOC_GROW(items, nr + commands->nr, alloc); >> + COPY_ARRAY(items + nr, base_items, commands->nr); >> + nr += commands->nr; >> + >> + insert = 0; >> } >> >> - if (command == TODO_PICK || command == TODO_MERGE) >> - insert = i + 1; >> + ALLOC_GROW(items, nr + 1, alloc); >> + items[nr++] = todo_list->items[i]; >> + >> + if (command == TODO_PICK || command == TODO_MERGE || is_fixup(command)) > > I'm not sure what the is_fixup() is for, if the command is a fixup then > it will have been preceded by a pick or merge so insert will already be 1 > > I feel a bit mean suggesting a reroll when we're on v7 already but I > think these clean-ups would improve the maintainability of the code. > I'll take a look at the rest of the changes to this series sometime this > week. > > Best Wishes > > Phillip > Cheers, Alban
Hi Alban On 12/02/2019 15:21, Alban Gruin wrote: > Hi Phillip, > > Le 12/02/2019 à 11:52, Phillip Wood a écrit : >> Hi Alban >> >> I think this is almost there, I've got a couple of small comments below. >> >> On 10/02/2019 13:26, Alban Gruin wrote: >>> -%<- >>> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c >>> index df19ccaeb9..c131fd4a27 100644 >>> --- a/builtin/rebase--interactive.c >>> +++ b/builtin/rebase--interactive.c >>> -%<- >>> @@ -221,6 +222,11 @@ 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(&commands, cmd, '\n', -1); >> >> I'd suggest a comment along the lines of >> /* As cmd always ends with a newline the last item is empty */ >> >>> + --commands.nr; >> >> Style: commands.nr-- >> >>> + } >>> + >>> switch (command) { >>> case NONE: >>> if (!onto && !upstream) >>> @@ -228,7 +234,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) >>> >>> ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, >>> onto_name, squash_onto, head_name, restrict_revision, >>> - raw_strategies, cmd, autosquash); >>> + raw_strategies, &commands, autosquash); >>> break; >>> case SKIP: { >>> struct string_list merge_rr = STRING_LIST_INIT_DUP; >>> @@ -262,11 +268,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) >>> ret = rearrange_squash(the_repository); >>> break; >>> case ADD_EXEC: >>> - ret = sequencer_add_exec_commands(the_repository, cmd); >>> + ret = sequencer_add_exec_commands(the_repository, &commands); >>> break; >>> default: >>> BUG("invalid command '%d'", command); >>> } >> >> Before freeing the string list it would be good to reset the number of >> items with commands.nr++ (with a comment explaining why) so the NUL in >> the last item gets freed. >> >>> + string_list_clear(&commands, 1); >> >> As we don't use item.util I think the second argument would be better as 0. >> >>> return !!ret; >>> } > > FWIW I just stumbled across string_list_remove_empty_items(), which > seems to do exactly the same thing, but that way we don’t have to do > this kind of hacks. Good find, that sounds like a much better solution. > >>> diff --git a/sequencer.c b/sequencer.c >>> index 99e12c751e..82ca3432cd 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -4505,21 +4505,27 @@ int sequencer_make_script(struct repository *r, FILE *out, >>> * Add commands after pick and (series of) squash/fixup commands >>> * in the todo list. >>> */ >>> -int sequencer_add_exec_commands(struct repository *r, >>> - 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 = &todo_list.buf; >>> - size_t offset = 0, commands_len = strlen(commands); >>> - int i, insert; >>> + struct strbuf *buf = &todo_list->buf; >>> + size_t base_offset = buf->len; >>> + int i, insert, nr = 0, alloc = 0; >>> + struct todo_item *items = NULL, *base_items = NULL; >>> >>> - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) >>> - return error(_("could not read '%s'."), todo_file); >>> + base_items = xcalloc(commands->nr, sizeof(struct todo_item)); >>> + for (i = 0; i < commands->nr; i++) { >>> + size_t command_len = strlen(commands->items[i].string); >>> >>> - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { >>> - todo_list_release(&todo_list); >>> - return error(_("unusable todo list: '%s'"), todo_file); >>> + strbuf_addstr(buf, commands->items[i].string); >>> + strbuf_addch(buf, '\n'); >>> + >>> + base_items[i].command = TODO_EXEC; >>> + base_items[i].offset_in_buf = base_offset; >>> + base_items[i].arg_offset = base_offset + strlen("exec "); >>> + base_items[i].arg_len = command_len - strlen("exec "); >>> + >>> + base_offset += command_len + 1; >>> } >>> >>> /* >>> @@ -4527,39 +4533,60 @@ int sequencer_add_exec_commands(struct repository *r, >>> * 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; >>> - >>> - 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); >>> - offset += commands_len; >>> - insert = -1; >> >> I like the simplification of using insert as a flag. Perhaps we should >> document the assumptions. Maybe something like >> >> We insert the exec commands immediately after rearranging any fixups and >> before the user edits the list. This means that a fixup chain can never >> contain comments (any comments are empty picks that have been commented >> out the the user did not specify --keep-empty) and so it is safe to >> insert the exec command without looking at the command following the >> comment. >> > > I slightly reworded this and added it to the existing comment just > before the for loop: > > /* > * 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. > * > * As we insert the exec commands immediatly after rearranging > * any fixups and before the user edits the list, a fixup chain > * can never contain comments (any comments are empty picks that > * have been commented out because the user did not specify > * --keep-empty). So, it is safe to insert an exec command > * without looking at the command following a comment. > */ > That sounds good Best Wishes Phillip >>> + insert = 0; >>> + for (i = 0; i < todo_list->nr; i++) { >>> + enum todo_command command = todo_list->items[i].command; >>> + if (insert && !is_fixup(command)) { >>> + ALLOC_GROW(items, nr + commands->nr, alloc); >>> + COPY_ARRAY(items + nr, base_items, commands->nr); >>> + nr += commands->nr; >>> + >>> + insert = 0; >>> } >>> >>> - if (command == TODO_PICK || command == TODO_MERGE) >>> - insert = i + 1; >>> + ALLOC_GROW(items, nr + 1, alloc); >>> + items[nr++] = todo_list->items[i]; >>> + >>> + if (command == TODO_PICK || command == TODO_MERGE || is_fixup(command)) >> >> I'm not sure what the is_fixup() is for, if the command is a fixup then >> it will have been preceded by a pick or merge so insert will already be 1 >> >> I feel a bit mean suggesting a reroll when we're on v7 already but I >> think these clean-ups would improve the maintainability of the code. >> I'll take a look at the rest of the changes to this series sometime this >> week. >> >> Best Wishes >> >> Phillip >> > > Cheers, > Alban >
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index df19ccaeb9..c131fd4a27 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; @@ -116,7 +116,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags, discard_cache(); ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name, onto, - head_hash, cmd, autosquash); + head_hash, commands, autosquash); } free(revisions); @@ -139,6 +139,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, @@ -221,6 +222,11 @@ 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(&commands, cmd, '\n', -1); + --commands.nr; + } + switch (command) { case NONE: if (!onto && !upstream) @@ -228,7 +234,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto, onto_name, squash_onto, head_name, restrict_revision, - raw_strategies, cmd, autosquash); + raw_strategies, &commands, autosquash); break; case SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; @@ -262,11 +268,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) ret = rearrange_squash(the_repository); break; case ADD_EXEC: - ret = sequencer_add_exec_commands(the_repository, cmd); + ret = sequencer_add_exec_commands(the_repository, &commands); break; default: BUG("invalid command '%d'", command); } + string_list_clear(&commands, 1); return !!ret; } diff --git a/sequencer.c b/sequencer.c index 99e12c751e..82ca3432cd 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4505,21 +4505,27 @@ int sequencer_make_script(struct repository *r, FILE *out, * Add commands after pick and (series of) squash/fixup commands * in the todo list. */ -int sequencer_add_exec_commands(struct repository *r, - 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 = &todo_list.buf; - size_t offset = 0, commands_len = strlen(commands); - int i, insert; + struct strbuf *buf = &todo_list->buf; + size_t base_offset = buf->len; + int i, insert, nr = 0, alloc = 0; + struct todo_item *items = NULL, *base_items = NULL; - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) - return error(_("could not read '%s'."), todo_file); + base_items = xcalloc(commands->nr, sizeof(struct todo_item)); + for (i = 0; i < commands->nr; i++) { + size_t command_len = strlen(commands->items[i].string); - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { - todo_list_release(&todo_list); - return error(_("unusable todo list: '%s'"), todo_file); + strbuf_addstr(buf, commands->items[i].string); + strbuf_addch(buf, '\n'); + + base_items[i].command = TODO_EXEC; + base_items[i].offset_in_buf = base_offset; + base_items[i].arg_offset = base_offset + strlen("exec "); + base_items[i].arg_len = command_len - strlen("exec "); + + base_offset += command_len + 1; } /* @@ -4527,39 +4533,60 @@ int sequencer_add_exec_commands(struct repository *r, * 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; - - 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); - offset += commands_len; - insert = -1; + insert = 0; + for (i = 0; i < todo_list->nr; i++) { + enum todo_command command = todo_list->items[i].command; + if (insert && !is_fixup(command)) { + ALLOC_GROW(items, nr + commands->nr, alloc); + COPY_ARRAY(items + nr, base_items, commands->nr); + nr += commands->nr; + + insert = 0; } - if (command == TODO_PICK || command == TODO_MERGE) - insert = i + 1; + ALLOC_GROW(items, nr + 1, alloc); + items[nr++] = todo_list->items[i]; + + if (command == TODO_PICK || command == TODO_MERGE || is_fixup(command)) + insert = 1; } /* insert or append final <commands> */ - 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); + if (insert || nr == todo_list->nr) { + ALLOC_GROW(items, nr + commands->nr, alloc); + COPY_ARRAY(items + nr, base_items, commands->nr); + nr += commands->nr; + } + + free(base_items); + FREE_AND_NULL(todo_list->items); + todo_list->items = items; + todo_list->nr = nr; + todo_list->alloc = alloc; +} - i = write_message(buf->buf, buf->len, todo_file, 0); +int sequencer_add_exec_commands(struct repository *r, + struct string_list *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_errno(_("could not read '%s'."), todo_file); + + if (todo_list_parse_insn_buffer(r, 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 = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, 0); todo_list_release(&todo_list); - return i; + + if (res) + return error_errno(_("could not write '%s'."), todo_file); + return 0; } static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list, @@ -4790,7 +4817,7 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, - const char *onto, const char *orig_head, const char *cmd, + const char *onto, const char *orig_head, struct string_list *commands, unsigned autosquash) { const char *shortonto, *todo_file = rebase_path_todo(); @@ -4809,8 +4836,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla if (autosquash && rearrange_squash(r)) return -1; - if (cmd && *cmd) - sequencer_add_exec_commands(r, cmd); + if (commands->nr) + sequencer_add_exec_commands(r, commands); if (strbuf_read_file(buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); diff --git a/sequencer.h b/sequencer.h index 217353e9f0..87d04a3b9b 100644 --- a/sequencer.h +++ b/sequencer.h @@ -146,12 +146,13 @@ int sequencer_make_script(struct repository *r, FILE *out, int argc, const char **argv, unsigned flags); -int sequencer_add_exec_commands(struct repository *r, const char *command); +int sequencer_add_exec_commands(struct repository *r, + struct string_list *commands); int transform_todo_file(struct repository *r, unsigned flags); int check_todo_list_from_file(struct repository *r); int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, const char *shortrevisions, const char *onto_name, - const char *onto, const char *orig_head, const char *cmd, + const char *onto, const char *orig_head, struct string_list *commands, unsigned autosquash); int rearrange_squash(struct repository *r);
This refactors sequencer_add_exec_commands() to work on a todo_list to avoid redundant reads and writes to the disk. Instead of inserting the `exec' commands between the other commands and re-parsing the buffer at the end, they are 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 <alban.gruin@gmail.com> --- builtin/rebase--interactive.c | 15 +++-- sequencer.c | 109 +++++++++++++++++++++------------- sequencer.h | 5 +- 3 files changed, 82 insertions(+), 47 deletions(-)