| Message ID | 20181007195418.25752-5-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: refactor functions working on a todo_list | expand |
On 07/10/2018 20:54, Alban Gruin wrote: > 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 <alban.gruin@gmail.com> > --- > 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) >
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: >> @@ -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
On 11/10/2018 17:57, Alban Gruin wrote: > 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: >>> @@ -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. Best Wishes Phillip > >> Best Wishes >> >> Phillip >> > > Cheers, > Alban >
Le 12/10/2018 à 11:54, Phillip Wood a écrit : > On 11/10/2018 17:57, Alban Gruin wrote: > > 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: > >>> @@ -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
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");} + +/* + * 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)
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 <alban.gruin@gmail.com> --- sequencer.c | 56 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 23 deletions(-)