Alban Gruin <alban.gruin@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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?
Thanks.
