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

> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> 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(-)
> 
> All the earlier steps in this series are necessary because the
> inconsistencies caused by not doing things the code updated by the
> earlier steps do (e.g. leaving number of commits recorded in
> total_nr and done_nr stale) were masked by re-reading the info from
> the on-disk file.  And this step exposes these inconsistencies
> because the extra reading from the file no longer happens.
> 
> That is my reading of the series---did I get it right?
> 

> I wonder if that can be stated clearly in the log message of the
> earlier steps.
> 
> For example, by reading 1/5 or 2/5 alone, it would be natural for
> readers to wonder why the original code that did not update done_nr
> correctly terminated after going through the todo list (you would
> expect that when done reaches total you are finished, so if one of
> these variables are not maintained correctly, your termination
> condition may be borked), and then by knowing that the current code
> more-or-less works correctly by not terminating too early or barfing
> to say it ran out of things to do prematurely, the next thing
> readers would wonder is if these patches introduce new bugs by
> incrementing these variables twice (iow, "does the author of the
> patch missed other places where these variables are correctly
> updated?")
> 
> 1/5 for example talks about the variable mostly being used by
> prompts, which may be useful to reduce worries by readers about
> correctness (iow, "well, if it is only showing wrong number in the
> prompts and does not affect correctness otherwise, perhaps that was
> why the current code that is buggy did not behave incorrectly").
> But I suspect that it is not why these changes in the earlier steps
> are acceptable or are right things to do.

>  So, perhaps replace the
> "these are used only in prompts and the numbers being wrong does not
> affect correctness" comments from them with:
> 
>     By forgetting to update the variable, the original code made it
>     not reflect the reality, but this flaw was masked by the code
>     calling (sometimes unnecessarily) read_todo_list() again to
>     update the variable to its correct value before the next action
>     happens.  At the end of this series, the unnecessary call will
>     be removed and the inconsistency addressed by this patch would
>     start to matter.
> 
> or something like that (assuming that the answer to the first
> question I asked in this message is "yes", that is), or a more
> concise version of it?
>