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

[Jonathan Tan <jonathantanmy@google.com>]

> Currently, complete_action(), used by builtin/rebase.c to start a new
> rebase, calls sequencer_continue() to do it.  Before the former calls
> pick_commits(), it
> 
>  - calls read_and_refresh_cache() -- this is unnecessary here as we've
>    just called require_clean_work_tree() in complete_action()

require_clean_work_tree() and read_and_refresh_cache() seem to be
different functions - can you explain why running the former is a good
substitute for running the latter?

>  - calls read_populate_opts() -- this is unnecessary as we're starting a
>    new rebase, so `opts' is fully populated

My comment from [1] has not been addressed. Quoting it here:

> So complete_action() (the function modified in this commit) is called
> only by do_interactive_rebase() (in builtin/rebase.c), which is only
> called by run_rebase_interactive() (in builtin/rebase.c) when command is
> ACTION_NONE, so indeed, we're starting a new rebase. But where the
> options fully populated? I see that in do_interactive_rebase(), it is
> initialized with get_replay_opts(), but that seems different from
> read_populate_opts().

[1] https://lore.kernel.org/git/20191119204146.168001-1-jonathantanmy@google.com/

>  - loads the todo list -- this is unnecessary as we've just populated
>    the todo list in complete_action()

Both functions indeed have their own todo lists that they pass to
pick_commits(), but I don't see (at least, by glancing at the code) that
both these todo lists are identical.

>  - commits any staged changes -- this is unnecessary as we're starting a
>    new rebase, so there are no staged changes
>  - calls record_in_rewritten() -- this is unnecessary as we're starting
>    a new rebase.

OK - I don't know enough about the rebase mechanism to verify these, but
these seem reasonable to me.