SZEDER Gábor <szeder.dev@xxxxxxxxx> 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?
