Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
Hi Alban
Thanks for removing some more unnecessary work reloading the the todo list.
On 25/09/2019 21:13, Alban Gruin wrote:
Show 26 quoted lines
> Currently, complete_action() calls sequencer_continue() to do the
> rebase. Even though the former already has the todo list, the latter
> loads it from the disk and parses it. Calling directly pick_commits()
> from complete_action() avoids this unnecessary round trip.
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
> sequencer.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index ec7ea8d9e5..b395dd6e11 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> return error_errno(_("could not write '%s'"), todo_file);
> }
>
> - todo_list_release(&new_todo);
> -
> if (checkout_onto(r, opts, onto_name, &oid, orig_head))
> return -1;
>
> if (require_clean_work_tree(r, "rebase", "", 1, 1))
> return -1;
>
> - return sequencer_continue(r, opts);sequencer_continue does a number of things before calling pick_commits(). It
- calls read_and_refresh_cache() - this is unnecessary here as we've
just called require_clean_work_tree()
- calls read_populate_opts() - this is unnecessary as we're staring a
new rebase so opts is fully populated
- loads the todo list - this is unnecessary as we've just populated
the todo list
- commits any staged changes - this is unnecessary as we're staring a
new rebase so there are no staged changes
- calls record_in_rewritten() - this is unnecessary as we're starting
a new rebase
So I agree that this patch is correct.
Thanks
Phillip
Show 9 quoted lines
> + todo_list_write_total_nr(&new_todo);
> + res = pick_commits(r, &new_todo, opts);
> + todo_list_release(&new_todo);
> +
> + return res;
> }
>
> struct subject2item_entry {
>