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

> When 'git revert' or 'git cherry-pick --edit' is invoked with multiple
> commits, then after editing the first commit message is finished both
> 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".
>
> The reason for the changed behaviour is twofold:
>
>   - Prior to a47ba3c777 the up-to-dateness of the todo list file was
>     only checked after 'exec' instructions, and that commit moved
>     those checks to the common code path.  The intention was that this
>     check should be performed after instructions spawning an editor
>     ('squash' and 'reword') as well, so the ongoing 'rebase -i'
>     notices when the user runs a 'git rebase --edit-todo' while
>     squashing/rewording a commit message.
>
>     However, as it happened that check is now performed even after
>     'revert' and 'pick' instructions when they involved editing the
>     commit message.  And 'revert' by default while 'pick' optionally
>     (with 'git cherry-pick --edit') involves editing the commit
>     message.
>
>   - 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.
>
> 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.
>
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---

> On Sat, Nov 23, 2019 at 09:53:51AM +0000, Phillip Wood wrote:
> > Thanks, I suspect it's missing an 'if (is_rebase_i(opts))' I'll take a look
> > later and come up with a fix
>
> That missing condition was my hunch yesterday evening as well, and
> while it did fix my tests and didn't break anything else, it took some
> time to wrap my head around some of the subtleties that are going on
> in the sequencer.  That's why the commit message ended up this long as
> it did.
>
> In the end I decided to add the new tests to
> 't3429-rebase-edit-todo.sh', because, though these new tests don't
> actually check 'rebase', that is the one test script focusing on
> (re-)reading the todo file in particular.

>
>  sequencer.c                 |  2 +-
>  t/t3429-rebase-edit-todo.sh | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 1 deletion(-)
>
> 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) {
>  			struct stat st;
>
>  			if (stat(get_todo_path(opts), &st)) {
> diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
> index 8739cb60a7..1679f2563d 100755
> --- a/t/t3429-rebase-edit-todo.sh
> +++ b/t/t3429-rebase-edit-todo.sh
> @@ -52,4 +52,34 @@ test_expect_success 'todo is re-read after reword and squash' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success 're-reading todo doesnt interfere with revert --edit' '
> +	git reset --hard third &&
> +
> +	git revert --edit third second &&
> +
> +	cat >expect <<-\EOF &&
> +	Revert "second"
> +	Revert "third"
> +	third
> +	second
> +	first
> +	EOF
> +	git log --format="%s" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 're-reading todo doesnt interfere with cherry-pick --edit' '
> +	git reset --hard first &&
> +
> +	git cherry-pick --edit second third &&
> +
> +	cat >expect <<-\EOF &&
> +	third
> +	second
> +	first
> +	EOF
> +	git log --format="%s" >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.24.0.532.ge18579ded8
>
>