Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 27 Sep 2019, Phillip Wood wrote:

> Hi Alban
>
> Thanks for removing some more unnecessary work reloading the the todo list.
>
> On 25/09/2019 21:13, Alban Gruin wrote:
> > 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.

All true. Could this careful analysis maybe be included in the commit
message (with `s/staring/starting/`)?

Thanks,
Dscho

>
> Thanks
>
> Phillip
>
> > +	todo_list_write_total_nr(&new_todo);
> > +	res = pick_commits(r, &new_todo, opts);
> > +	todo_list_release(&new_todo);
> > +
> > +	return res;
> >   }
> >
> >   struct subject2item_entry {
> >
>