| Message ID | 20181007195418.25752-11-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: > Just like complete_action(), edit_todo_list() used a > function (transform_todo_file()) that read the todo-list from the disk > and wrote it back, resulting in useless disk accesses. > > This changes edit_todo_list() to call directly todo_list_transform() > instead. > > Signed-off-by: Alban Gruin <alban.gruin@gmail.com> > --- > rebase-interactive.c | 40 +++++++++++++++++++--------------------- > 1 file changed, 19 insertions(+), 21 deletions(-) > > diff --git a/rebase-interactive.c b/rebase-interactive.c > index 7c7f720a3d..f42d48e192 100644 > --- a/rebase-interactive.c > +++ b/rebase-interactive.c > @@ -78,39 +78,37 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, > > int edit_todo_list(unsigned flags) > { > - struct strbuf buf = STRBUF_INIT; > const char *todo_file = rebase_path_todo(); > + struct todo_list todo_list = TODO_LIST_INIT; > + int res = 0; > > - if (strbuf_read_file(&buf, todo_file, 0) < 0) > + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > return error_errno(_("could not read '%s'."), todo_file); > > - strbuf_stripspace(&buf, 1); > - if (write_message(buf.buf, buf.len, todo_file, 0)) { > - strbuf_release(&buf); > - return -1; > - } > - > - strbuf_release(&buf); > + strbuf_stripspace(&todo_list.buf, 1); > + if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) > + todo_list_transform(&todo_list, flags | TODO_LIST_SHORTEN_IDS); > > - transform_todo_file(flags | TODO_LIST_SHORTEN_IDS); > - > - if (strbuf_read_file(&buf, todo_file, 0) < 0) > - return error_errno(_("could not read '%s'."), todo_file); > + append_todo_help(1, 0, &todo_list.buf); > > - append_todo_help(1, 0, &buf); I think this patch is fine, I was just wondering if you meant to move the call to append_todo_help() above the blank line? Best Wishes Phillip > - if (write_message(buf.buf, buf.len, todo_file, 0)) { > - strbuf_release(&buf); > + if (write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0)) { > + todo_list_release(&todo_list); > return -1; > } > > - strbuf_release(&buf); > - > - if (launch_sequence_editor(todo_file, NULL, NULL)) > + strbuf_reset(&todo_list.buf); > + if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { > + todo_list_release(&todo_list); > return -1; > + } > > - transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)); > + if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { > + todo_list_transform(&todo_list, flags & ~(TODO_LIST_SHORTEN_IDS)); > + res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0); > + } > > - return 0; > + todo_list_release(&todo_list); > + return res; > } > > define_commit_slab(commit_seen, unsigned char); >
Le 11/10/2018 à 17:16, Phillip Wood a écrit : > On 07/10/2018 20:54, Alban Gruin wrote: >> Just like complete_action(), edit_todo_list() used a >> function (transform_todo_file()) that read the todo-list from the disk >> and wrote it back, resulting in useless disk accesses. >> >> This changes edit_todo_list() to call directly todo_list_transform() >> instead. >> >> Signed-off-by: Alban Gruin <alban.gruin@gmail.com> >> --- >> rebase-interactive.c | 40 +++++++++++++++++++--------------------- >> 1 file changed, 19 insertions(+), 21 deletions(-) >> >> diff --git a/rebase-interactive.c b/rebase-interactive.c >> index 7c7f720a3d..f42d48e192 100644 >> --- a/rebase-interactive.c >> +++ b/rebase-interactive.c >> @@ -78,39 +78,37 @@ void append_todo_help(unsigned edit_todo, unsigned >> keep_empty, >> int edit_todo_list(unsigned flags) >> { >> - struct strbuf buf = STRBUF_INIT; >> const char *todo_file = rebase_path_todo(); >> + struct todo_list todo_list = TODO_LIST_INIT; >> + int res = 0; >> - if (strbuf_read_file(&buf, todo_file, 0) < 0) >> + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) >> return error_errno(_("could not read '%s'."), todo_file); >> - strbuf_stripspace(&buf, 1); >> - if (write_message(buf.buf, buf.len, todo_file, 0)) { >> - strbuf_release(&buf); >> - return -1; >> - } >> - >> - strbuf_release(&buf); >> + strbuf_stripspace(&todo_list.buf, 1); >> + if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) >> + todo_list_transform(&todo_list, flags | TODO_LIST_SHORTEN_IDS); >> - transform_todo_file(flags | TODO_LIST_SHORTEN_IDS); >> - >> - if (strbuf_read_file(&buf, todo_file, 0) < 0) >> - return error_errno(_("could not read '%s'."), todo_file); >> + append_todo_help(1, 0, &todo_list.buf); >> - append_todo_help(1, 0, &buf); > > I think this patch is fine, I was just wondering if you meant to move > the call to append_todo_help() above the blank line? > No > Best Wishes > > Phillip > Cheers, Alban
diff --git a/rebase-interactive.c b/rebase-interactive.c index 7c7f720a3d..f42d48e192 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -78,39 +78,37 @@ void append_todo_help(unsigned edit_todo, unsigned keep_empty, int edit_todo_list(unsigned flags) { - struct strbuf buf = STRBUF_INIT; const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT; + int res = 0; - if (strbuf_read_file(&buf, todo_file, 0) < 0) + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) return error_errno(_("could not read '%s'."), todo_file); - strbuf_stripspace(&buf, 1); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); - return -1; - } - - strbuf_release(&buf); + strbuf_stripspace(&todo_list.buf, 1); + if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) + todo_list_transform(&todo_list, flags | TODO_LIST_SHORTEN_IDS); - transform_todo_file(flags | TODO_LIST_SHORTEN_IDS); - - if (strbuf_read_file(&buf, todo_file, 0) < 0) - return error_errno(_("could not read '%s'."), todo_file); + append_todo_help(1, 0, &todo_list.buf); - append_todo_help(1, 0, &buf); - if (write_message(buf.buf, buf.len, todo_file, 0)) { - strbuf_release(&buf); + if (write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0)) { + todo_list_release(&todo_list); return -1; } - strbuf_release(&buf); - - if (launch_sequence_editor(todo_file, NULL, NULL)) + strbuf_reset(&todo_list.buf); + if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { + todo_list_release(&todo_list); return -1; + } - transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)); + if (!todo_list_parse_insn_buffer(todo_list.buf.buf, &todo_list)) { + todo_list_transform(&todo_list, flags & ~(TODO_LIST_SHORTEN_IDS)); + res = write_message(todo_list.buf.buf, todo_list.buf.len, todo_file, 0); + } - return 0; + todo_list_release(&todo_list); + return res; } define_commit_slab(commit_seen, unsigned char);
Just like complete_action(), edit_todo_list() used a function (transform_todo_file()) that read the todo-list from the disk and wrote it back, resulting in useless disk accesses. This changes edit_todo_list() to call directly todo_list_transform() instead. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- rebase-interactive.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-)