| Message ID | 20191123172046.16359-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: don't re-read todo for revert and cherry-pick | expand |
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@chromium.org> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- 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 > >
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. > 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?
On 24/11/2019 04:49, Junio C Hamano wrote: Thanks for working on this Gábor > 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. > >> 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. > >> 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) >> 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 &&
Phillip Wood <phillip.wood123@gmail.com> writes: >>> - } 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)... I first thought that unsetting *check_todo in do_pick_commit(), when !is_rebase_i(), was a clean solution. But sadly it is *not* a godo equivalent to Gábor's patch, because check_todo can be set to true without looking at is_rebase_i() in pick_commits() [*1*]. To ignore that setting where the variable's value is used, the hunk we see above in the beginning of this message is necessary. That was what I meant. I think the "This is what I had before" patch matches my "I first thought" version, so we were on the same page and both wrong ;-) Thanks. [Footnote] *1* I still do not know why a47ba3c7 ("rebase -i: check for updated todo after squash and reword", 2019-08-19) sets check_todo to true without looking at is_rebase_i(). If this unconditonal setting in TODO_EXEC did not exist, I think your "This is what I had before" patch would have been equivalent to Gábor's patch.
Hi Junio On 25/11/2019 01:10, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >>>> - } 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)... > > I first thought that unsetting *check_todo in do_pick_commit(), when > !is_rebase_i(), was a clean solution. But sadly it is *not* a godo > equivalent to Gábor's patch, because check_todo can be set to true > without looking at is_rebase_i() in pick_commits() [*1*]. That only happens if we're running a rebase as exec commands are not used by cherry-pick and revert so the unconditional setting that remains after my suggested fix should be safe. Prior to a47ba3c7 ("rebase -i: check for updated todo after squash and reword", 2019-08-19) we always checked for an updated todo list after processing an exec command without checking is_rebase_i() Best Wishes Phillip > To ignore > that setting where the variable's value is used, the hunk we see > above in the beginning of this message is necessary. > > That was what I meant. I think the "This is what I had before" > patch matches my "I first thought" version, so we were on the same > page and both wrong ;-) > > Thanks. > > > [Footnote] > > *1* I still do not know why a47ba3c7 ("rebase -i: check for updated > todo after squash and reword", 2019-08-19) sets check_todo to true > without looking at is_rebase_i(). If this unconditonal setting in > TODO_EXEC did not exist, I think your "This is what I had before" > patch would have been equivalent to Gábor's patch.
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
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(-)