> > * 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@xxxxxxxxx/ 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.
