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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 24, 2019, 04:49 UTC
Message-ID: <xmqqk17p280y.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191123172046.16359-1-szeder.dev@gmail.com>

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.

Show 28 quoted lines

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

Show 10 quoted lines

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