On 24/11/2019 04:49, Junio C Hamano wrote:

Thanks for working on this Gábor

SZEDER Gábor [off-list ref] writes:

quoted

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.

quoted

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?

I think we should update the stat data after we write the todo list. 
ag/sequencer-todo-updates changes the rebase startup sequence to avoid 
the initial read that populates the stat data. There's a subtle 
difference in the todo list handling between rabese and 
cherry-pick/revert. The list written by rebase does not include the 
commit currently being picked but it does for cherry-pick/revert. This 
means that rebase is not subject to this bug in 
ag/sequencer-todo-updates as although it re-reads the todo list because 
the stat data is zero the list does not contain the commit we've just 
picked.

quoted

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

I'd be happier not to set check_todo in the first place unless we're 
rebasing (though one could argue that Gábor's version is more future-proof)

quoted

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?

I'm not sure what you mean by this

This is what I had before I saw Gábor's patch (the tests are pretty 
similar but I think we should check that the messages of all the picks 
are actually edited with --edit - that does not seem to be checked by 
the current tests) Note this does not include updating the stat data 
after writing the todo list yet as I've only just thought about that.
Gábor are you happy to take this forward?

Best Wishes

Phillip

--- 8> ---

diff --git a/sequencer.c b/sequencer.c
index f2abe2a366..b363b45366 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1999,7 +1999,7 @@ static int do_pick_commit(struct repository *r,
                         res = do_commit(r, msg_file, author, opts, flags);
                 else
                         res = error(_("unable to parse commit author"));
-               *check_todo = !!(flags & EDIT_MSG);
+               *check_todo = is_rebase_i(opts) && !!(flags & EDIT_MSG);
                 if (!res && reword) {
  fast_forward_edit:
                         res = run_git_commit(r, NULL, opts, EDIT_MSG |

diff --git a/t/t3508-cherry-pick-many-commits.sh 

b/t/t3508-cherry-pick-many-commits.sh
index b457333e18..fd9d626779 100755

--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh

@@ -40,6 +40,31 @@ test_expect_success 'cherry-pick first..fourth works' '
         check_head_differs_from fourth
  '

+test_expect_success 'cherry-pick --edit first..fourth works' '
+       git checkout -f master &&
+       git reset --hard first &&
+       test_tick &&
+       GIT_EDITOR="echo edited >>" git cherry-pick --edit first..fourth &&
+       git log --pretty=format:%B -3 >actual &&
+       cat >expect <<-\EOF &&
+       fourth
+
+       edited
+
+       third
+
+       edited
+
+       second
+
+       edited
+       EOF
+       test_cmp expect actual &&
+       git diff --quiet other &&
+       git diff --quiet HEAD other &&
+       check_head_differs_from fourth
+'
+
  test_expect_success 'cherry-pick three one two works' '
         git checkout -f first &&
         test_commit one &&

@@ -153,6 +178,37 @@ test_expect_success 'revert first..fourth works' '
         git diff --quiet HEAD first
  '

+test_expect_success 'revert --edit first..fourth works' '
+       git checkout -f master &&
+       git reset --hard fourth &&
+       test_tick &&
+       GIT_EDITOR="echo edited >>" git revert --edit first..fourth &&
+       git log --pretty=format:%B -3 >actual &&
+       cat >expect <<-EOF &&
+       Revert "second"
+
+       This reverts commit $(git rev-parse second).
+
+       edited
+
+       Revert "third"
+
+       This reverts commit $(git rev-parse third).
+
+       edited

'

+test_expect_success 'revert --edit first..fourth works' '
+       git checkout -f master &&
+       git reset --hard fourth &&
+       test_tick &&
+       GIT_EDITOR="echo edited >>" git revert --edit first..fourth &&
+       git log --pretty=format:%B -3 >actual &&
+       cat >expect <<-EOF &&
+       Revert "second"
+
+       This reverts commit $(git rev-parse second).
+
+       edited
+
+       Revert "third"
+
+       This reverts commit $(git rev-parse third).
+
+       edited
+
+       Revert "fourth"
+
+       This reverts commit $(git rev-parse fourth).
+
+       edited
+       EOF
+       test_cmp expect actual &&
+       git diff --quiet first &&
+       git diff --cached --quiet first &&
+       git diff --quiet HEAD first
+'
+
  test_expect_success 'revert ^first fourth works' '
         git checkout -f master &&
         git reset --hard fourth &&