Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick

> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
>> When 'git revert' or 'git cherry-pick --edit' is invoked with multiple
>> commits, then after editing the first commit message is finished both
> 
> "commits, then after editing the first commit message, both of" I
> would say.
> 
>> these commands should continue with processing the second commit and
>> launch another editor for its commit message, assuming there are
>> no conflicts, of course.
>>
>> Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:
>> check for updated todo after squash and reword, 2019-08-19): after
>> editing the first commit message is finished, both 'git revert' and
>> 'git cherry-pick --edit' exit with error, claiming that "nothing to
>> commit, working tree clean".
>> ...
>>    - When invoking 'git revert' or 'git cherry-pick --edit' with
>>      multiple commits they don't read a todo list file but assemble the
>>      todo list in memory, thus the associated stat data used to check
>>      whether the file has been updated is all zeroed out initially.
>>
>>      Then the sequencer writes all instructions (including the very
>>      first) to the todo file, executes the first 'revert/pick'
>>      instruction, and after the user finished editing the commit
>>      message the changes of a47ba3c777 kick in, and it checks whether
>>      the todo file has been modified.  The initial all-zero stat data
>>      obviously differs from the todo file's current stat data, so the
>>      sequencer concludes that the file has been modified.  Technically
>>      it is not wrong, of course, because the file just has been written
>>      indeed by the sequencer itself, though the file's contents still
>>      match what the sequencer was invoked with in the beginning.
>>      Consequently, after re-reading the todo file the sequencer
>>      executes the same first instruction _again_, thus ending up in
>>      that "nothing to commit" situation.
> 
> Hmph, that makes it sound as if the right fix is to re-read after
> writing the first version of the todo file out, so that the stat
> data matches reality and tells us that it has never been modified?

> 
>> The todo list was never meant to be edited during multi-commit 'git
>> revert' or 'cherry-pick' operations, so perform that "has the todo
>> file been modified" check only when the sequencer was invoked as part
>> of an interactive rebase.
> 
> OK.  That is a valid fix, I think, but the explanation given in the
> second bullet point gives a wrong impression that it is merely a
> workaround (iow, we are assuming that the user would behave, instead
> of making sure we can detect when the user touches the list), when
> it is *not*.
> 

>> diff --git a/sequencer.c b/sequencer.c
>> index 2adcf5a639..3b05d0277d 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -3791,7 +3791,7 @@ static int pick_commits(struct repository *r,
>>   							item->commit,
>>   							arg, item->arg_len,
>>   							opts, res, 0);
>> -		} else if (check_todo && !res) {
>> +		} else if (is_rebase_i(opts) && check_todo && !res) {
> 
> It is a bit sad that setting of check_todo is not something a single
> helper function can decide, so that this is_rebase_i(opts) can be
> taken into account when that helper function (the logical place
> would be do_pick_commit()) decides to set (or not set) check_todo.
> 
> Unfortunately, that is not sufficient, I suspect.  Why did a47ba3c7
> ("rebase -i: check for updated todo after squash and reword",
> 2019-08-19) decide to flip check_todo on when running TODO_EXEC?