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

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

Hi Johannes,

Le 02/10/2019 à 10:20, Johannes Schindelin a écrit :
> Hi,
> 
> On Fri, 27 Sep 2019, Phillip Wood wrote:
> 
>> Hi Alban
>>
>> Thanks for removing some more unnecessary work reloading the the todo list.
>>
>> On 25/09/2019 21:13, Alban Gruin wrote:
>>> 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(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index ec7ea8d9e5..b395dd6e11 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct
>>> replay_opts *opts, unsigned fla
>>>    	return error_errno(_("could not write '%s'"), todo_file);
>>>    }
>>>   -	todo_list_release(&new_todo);
>>> -
>>>    if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>>>     return -1;
>>>
>>>    if (require_clean_work_tree(r, "rebase", "", 1, 1))
>>>     return -1;
>>>   -	return sequencer_continue(r, opts);
>>
>> sequencer_continue does a number of things before calling pick_commits(). It
>>  - calls read_and_refresh_cache() - this is unnecessary here as we've just
>> called require_clean_work_tree()
>>  - calls read_populate_opts() - this is unnecessary as we're staring a new
>> rebase so opts is fully populated
>>  - loads the todo list - this is unnecessary as we've just populated the todo
>> list
>>  - commits any staged changes - this is unnecessary as we're staring a new
>> rebase so there are no staged changes
>>  - calls record_in_rewritten() - this is unnecessary as we're starting a new
>> rebase
>>
>> So I agree that this patch is correct.
> 
> All true. Could this careful analysis maybe be included in the commit
> message (with `s/staring/starting/`)?
> 

I will do so (same for your comment on 4/5) and resend this series as
soon as possible.

Cheers,
Alban



> Thanks,
> Dscho
> 
>>
>> Thanks
>>
>> Phillip
>>
>>> +	todo_list_write_total_nr(&new_todo);
>>> +	res = pick_commits(r, &new_todo, opts);
>>> +	todo_list_release(&new_todo);
>>> +
>>> +	return res;
>>>   }
>>>
>>>   struct subject2item_entry {
>>>
>>