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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 26, 2019, 18:41 UTC
Message-ID: <20191126184103.146421-1-jonathantanmy@google.com>
In-Reply-To: <20191124174332.30887-6-alban.gruin@gmail.com>

Show 6 quoted lines

> 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:

Show 7 quoted lines

> 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.