Re: ag/sequencer-todo-updates , was Re: What's cooking in git.git (Nov 2019, #03; Tue, 19)

[Jonathan Tan <jonathantanmy@google.com>]

> > * ag/sequencer-todo-updates (2019-10-28) 6 commits
> >  - SQUASH??? tentative leakfix
> >  - sequencer: directly call pick_commits() from complete_action()
> >  - rebase: fill `squash_onto' in get_replay_opts()
> >  - sequencer: move the code writing total_nr on the disk to a new function
> >  - sequencer: update `done_nr' when skipping commands in a todo list
> >  - sequencer: update `total_nr' when adding an item to a todo list
> > 
> >  Reduce unnecessary reading of state variables back from the disk
> >  during sequener operation.
> > 
> >  Is the leakfix patch at the tip the only thing that needs to
> >  prepare the topic ready for 'next'?
> > 
> 
> Yes, it is.

I took a look at this. Some comments:

- Commit message 1 refers to read_todo_list() which doesn't exist.
  Should it be read_populate_todo()?
- Commit 3's todo_list_write_total_nr() could just take an int instead
  of the full "struct todo_list *".

And overall, I wish that there was more descriptions of the code paths
involved, especially in commits 4 and 5. In commit 4, I can see that
run_rebase_interactive() calls get_replay_opts() then
sequencer_continue() (so sequencer_continue() receives whatever
get_replay_opts() outputs - and this commit adds the initialization of
"squash_onto" therein), which calls pick_commits() (which uses
"squash_onto"). I would have liked the commit message to verify that in
sequencer_continue(), before pick_commits(), "squash_onto" was never
written to, so it is crucial for get_replay_opts() to fill
"squash_onto".

And in commit 5, I noticed some analysis from Phillip Wood [1] but I
would have liked more details. For example,

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

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://public-inbox.org/git/212cdc0d-8cf3-9172-d405-39b3868e6ca4@gmail.com/

Having said all that, I'm not opposed to this being in "next" (except
that the commit message 1 probably should be updated), since it seems to
me that the analysis has already been done, and is merely unwritten.