| Message ID | 20190129150159.10588-12-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: refactor functions working on a todo_list | expand |
Hi Alban On 29/01/2019 15:01, Alban Gruin wrote: > This refactors skip_unnecessary_picks() to work on a todo_list. As this > function is only called by complete_action() (and thus is not used by > rebase -p), the file-handling logic is completely dropped here. > > Instead of truncating the todo list’s buffer, the items are moved to > the beginning of the list, eliminating the need to reparse the list. > This also means its buffer cannot be directly written to the disk. > > rewrite_file() is then removed, as it is now unused. > > Signed-off-by: Alban Gruin <alban.gruin@gmail.com> > --- > sequencer.c | 78 ++++++++++++----------------------------------------- > 1 file changed, 17 insertions(+), 61 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 2a43ca685b..a817afffa9 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4661,52 +4661,22 @@ int check_todo_list_from_file(struct repository *r) > return res; > } > > -static int rewrite_file(const char *path, const char *buf, size_t len) > -{ > - int rc = 0; > - int fd = open(path, O_WRONLY | O_TRUNC); > - if (fd < 0) > - return error_errno(_("could not open '%s' for writing"), path); > - if (write_in_full(fd, buf, len) < 0) > - rc = error_errno(_("could not write to '%s'"), path); > - if (close(fd) && !rc) > - rc = error_errno(_("could not close '%s'"), path); > - return rc; > -} > - It's great to see that going > /* skip picking commits whose parents are unchanged */ > -static int skip_unnecessary_picks(struct repository *r, struct object_id *output_oid) > +static int skip_unnecessary_picks(struct repository *r, > + struct todo_list *todo_list, > + struct object_id *output_oid) output_oid is a bit misleading now as we now feed the function the onto commit with that parameter. Perhaps we could rename it to base_oid or something like that (I've been working on getting rebase -i to start without forking rebase--interactive and as part of that re-factoring I've changed the caller of this function to take a struct commit* rather than a string I got tripped up by this) Best Wishes Phillip > { > - const char *todo_file = rebase_path_todo(); > - struct strbuf buf = STRBUF_INIT; > - struct todo_list todo_list = TODO_LIST_INIT; > struct object_id *parent_oid; > - int fd, i; > - > - if (!read_oneliner(&buf, rebase_path_onto(), 0)) > - return error(_("could not read 'onto'")); > - if (get_oid(buf.buf, output_oid)) { > - strbuf_release(&buf); > - return error(_("need a HEAD to fixup")); > - } > - strbuf_release(&buf); > - > - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) > - return -1; > - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { > - todo_list_release(&todo_list); > - return -1; > - } > + int i; > > - for (i = 0; i < todo_list.nr; i++) { > - struct todo_item *item = todo_list.items + i; > + for (i = 0; i < todo_list->nr; i++) { > + struct todo_item *item = todo_list->items + i; > > if (item->command >= TODO_NOOP) > continue; > if (item->command != TODO_PICK) > break; > if (parse_commit(item->commit)) { > - todo_list_release(&todo_list); > return error(_("could not parse commit '%s'"), > oid_to_hex(&item->commit->object.oid)); > } > @@ -4720,37 +4690,21 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output > oidcpy(output_oid, &item->commit->object.oid); > } > if (i > 0) { > - int offset = get_item_line_offset(&todo_list, i); > const char *done_path = rebase_path_done(); > > - fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); > - if (fd < 0) { > - error_errno(_("could not open '%s' for writing"), > - done_path); > - todo_list_release(&todo_list); > - return -1; > - } > - if (write_in_full(fd, todo_list.buf.buf, offset) < 0) { > + if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) { > error_errno(_("could not write to '%s'"), done_path); > - todo_list_release(&todo_list); > - close(fd); > return -1; > } > - close(fd); > > - if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, > - todo_list.buf.len - offset) < 0) { > - todo_list_release(&todo_list); > - return -1; > - } > + MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); > + todo_list->nr -= i; > + todo_list->current = 0; > > - todo_list.current = i; > - if (is_fixup(peek_command(&todo_list, 0))) > - record_in_rewritten(output_oid, peek_command(&todo_list, 0)); > + if (is_fixup(peek_command(todo_list, 0))) > + record_in_rewritten(output_oid, peek_command(todo_list, 0)); > } > > - todo_list_release(&todo_list); > - > return 0; > } > > @@ -4823,6 +4777,11 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > return -1; > } > > + if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) { > + todo_list_release(&new_todo); > + return error(_("could not skip unnecessary pick commands")); > + } > + > if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1, > flags & ~(TODO_LIST_SHORTEN_IDS))) { > todo_list_release(&new_todo); > @@ -4831,9 +4790,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > > todo_list_release(&new_todo); > > - if (opts->allow_ff && skip_unnecessary_picks(r, &oid)) > - return error(_("could not skip unnecessary pick commands")); > - > if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head)) > return -1; > >
diff --git a/sequencer.c b/sequencer.c index 2a43ca685b..a817afffa9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4661,52 +4661,22 @@ int check_todo_list_from_file(struct repository *r) return res; } -static int rewrite_file(const char *path, const char *buf, size_t len) -{ - int rc = 0; - int fd = open(path, O_WRONLY | O_TRUNC); - if (fd < 0) - return error_errno(_("could not open '%s' for writing"), path); - if (write_in_full(fd, buf, len) < 0) - rc = error_errno(_("could not write to '%s'"), path); - if (close(fd) && !rc) - rc = error_errno(_("could not close '%s'"), path); - return rc; -} - /* skip picking commits whose parents are unchanged */ -static int skip_unnecessary_picks(struct repository *r, struct object_id *output_oid) +static int skip_unnecessary_picks(struct repository *r, + struct todo_list *todo_list, + struct object_id *output_oid) { - const char *todo_file = rebase_path_todo(); - struct strbuf buf = STRBUF_INIT; - struct todo_list todo_list = TODO_LIST_INIT; struct object_id *parent_oid; - int fd, i; - - if (!read_oneliner(&buf, rebase_path_onto(), 0)) - return error(_("could not read 'onto'")); - if (get_oid(buf.buf, output_oid)) { - strbuf_release(&buf); - return error(_("need a HEAD to fixup")); - } - strbuf_release(&buf); - - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) - return -1; - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { - todo_list_release(&todo_list); - return -1; - } + int i; - for (i = 0; i < todo_list.nr; i++) { - struct todo_item *item = todo_list.items + i; + for (i = 0; i < todo_list->nr; i++) { + struct todo_item *item = todo_list->items + i; if (item->command >= TODO_NOOP) continue; if (item->command != TODO_PICK) break; if (parse_commit(item->commit)) { - todo_list_release(&todo_list); return error(_("could not parse commit '%s'"), oid_to_hex(&item->commit->object.oid)); } @@ -4720,37 +4690,21 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output oidcpy(output_oid, &item->commit->object.oid); } if (i > 0) { - int offset = get_item_line_offset(&todo_list, i); const char *done_path = rebase_path_done(); - fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); - if (fd < 0) { - error_errno(_("could not open '%s' for writing"), - done_path); - todo_list_release(&todo_list); - return -1; - } - if (write_in_full(fd, todo_list.buf.buf, offset) < 0) { + if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) { error_errno(_("could not write to '%s'"), done_path); - todo_list_release(&todo_list); - close(fd); return -1; } - close(fd); - if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, - todo_list.buf.len - offset) < 0) { - todo_list_release(&todo_list); - return -1; - } + MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i); + todo_list->nr -= i; + todo_list->current = 0; - todo_list.current = i; - if (is_fixup(peek_command(&todo_list, 0))) - record_in_rewritten(output_oid, peek_command(&todo_list, 0)); + if (is_fixup(peek_command(todo_list, 0))) + record_in_rewritten(output_oid, peek_command(todo_list, 0)); } - todo_list_release(&todo_list); - return 0; } @@ -4823,6 +4777,11 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla return -1; } + if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) { + todo_list_release(&new_todo); + return error(_("could not skip unnecessary pick commands")); + } + if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS))) { todo_list_release(&new_todo); @@ -4831,9 +4790,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla todo_list_release(&new_todo); - if (opts->allow_ff && skip_unnecessary_picks(r, &oid)) - return error(_("could not skip unnecessary pick commands")); - if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head)) return -1;
This refactors skip_unnecessary_picks() to work on a todo_list. As this function is only called by complete_action() (and thus is not used by rebase -p), the file-handling logic is completely dropped here. Instead of truncating the todo list’s buffer, the items are moved to the beginning of the list, eliminating the need to reparse the list. This also means its buffer cannot be directly written to the disk. rewrite_file() is then removed, as it is now unused. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- sequencer.c | 78 ++++++++++++----------------------------------------- 1 file changed, 17 insertions(+), 61 deletions(-)