Hi,
On Sat, 23 Nov 2019, SZEDER Gábor wrote:
> 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@xxxxxxxxxxxx>
> Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
> ---
The patch and the commit message (thank you so much for the detail!) look
very good to me.
> 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.
Yup, makes sense.
Thanks,
Dscho
>
> 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
>
>
