Hi Alban
On 10/02/2019 13:26, 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@xxxxxxxxx>
> ---
> sequencer.c | 82 +++++++++++++----------------------------------------
> 1 file changed, 19 insertions(+), 63 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index eb8a622af0..eacaf52250 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4717,52 +4717,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 *base_oid)
Thanks for renaming that parameter, I think it's much clearer.
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));
> }
> @@ -4771,42 +4741,26 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output
> if (item->commit->parents->next)
> break; /* merge commit */
> parent_oid = &item->commit->parents->item->object.oid;
> - if (!oideq(parent_oid, output_oid))
> + if (!oideq(parent_oid, base_oid))
> break;
> - oidcpy(output_oid, &item->commit->object.oid);
> + oidcpy(base_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(base_oid, peek_command(todo_list, 0));
> }
>
> - todo_list_release(&todo_list);
> -
> return 0;
> }
>
> @@ -4879,6 +4833,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);
> @@ -4887,9 +4846,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;
>
>
