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

[Alban Gruin <alban.gruin@gmail.com>]

Hi Junio,

Le 02/10/2019 à 04:38, Junio C Hamano a écrit :
> 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?
> 

Yes.

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

Interestingly, it is the only reason for these two patches.  If you skip
them both, t34??-*.sh will not fail, only t9903-bash-prompt.sh will.
This is because `total_nr' does not reflect the real number of items in
a todo list, but the number of steps, done *and* remaining.  When a
rebase is resumed, some commands may already have been run, and are no
longer part of the todo list.  So, it’s not used to determine whether or
not the rebase is done.  `nr' is used for this purpose.  `done_nr' and
`total_nr' are just used for user interaction.

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

I will do so.

Cheers,
Alban



> Thanks.
>