{"thread":{"id":"52319","subject":"git 2.24: git revert requires extra '--continue'?","startedAt":"2019-11-22T23:10:38Z","lastAt":"2019-11-25T16:40:52Z","messageCount":16,"participants":["Brian Norris","SZEDER Gábor","Phillip Wood","Johannes Schindelin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"386875","messageId":"CA+ASDXNQ+9i-6uCNDwN46cnHJeciiqQSX5Z6EaqB9pbseQhVFw@mail.gmail.com","threadId":"52319","inReplyTo":null,"subject":"git 2.24: git revert requires extra '--continue'?","fromName":"Brian Norris","fromEmail":"briannorris@chromium.org","sentAt":"2019-11-22T23:10:20Z","receivedAt":"2019-11-22T23:10:38Z","isPatch":false,"sender":{"key":"briannorris@chromium.org","avatar":null},"body":"Hi! I'm using git 2.24 (or, a variant of that packaged my distro -- I\ncan try to build my own if this is deemed not reproducible), and I\nfeel like I've been seeing a regression here:\n\nPreviously, when reverting multiple commits (with the default --edit\nbehavior), as soon as I'm done editing the first revert commit\nmessage, the second revert commit pops up an editor, and I'm on my\nway. Now, it seems to require an extra 'git revert --continue' prompt\nin between, yet it doesn't actually recommend that. This is highly\nconfusing, and seemingly unnecessary.\n\nAn annotated transcript provided below.\n\nNote that none of this happens with --no-edit; my reverts happen\nsmoothly, with no extra need for --continue.\n\nRegards,\nBrian\n\n$ mkdir tmp\n$ cd tmp\n/tmp$ git init\nInitialized empty Git repository in [...]/tmp/.git/\n/tmp$ touch foo\n/tmp$ git add foo\n/tmp$ echo bar >> foo\n/tmp$ git commit -am baz\n[master (root-commit) a388f78d9013] baz\n 1 file changed, 1 insertion(+)\n create mode 100644 foo\n/tmp$ echo pow >> foo\n/tmp$ git commit -am whizzbang\n[master 51753222dd9a] whizzbang\n 1 file changed, 1 insertion(+)\n/tmp$ echo pop >> foo\n/tmp$ git commit -am nothing\n[master 7153607b11e0] nothing\n 1 file changed, 1 insertion(+)\n/tmp$ git revert HEAD HEAD^\n## EDITOR pops up, as expected\n[master 586469974ec2] Revert \"nothing\"\n 1 file changed, 1 deletion(-)\nOn branch master\nRevert currently in progress.\n\nnothing to commit, working tree clean\n## Unexpected, confusing pause? No prompt to '--continue'\n/tmp$ git log --oneline\n586469974ec2 (HEAD -> master) Revert \"nothing\"\n7153607b11e0 nothing\n51753222dd9a whizzbang\na388f78d9013 baz\n/tmp$ git status\nOn branch master\nRevert currently in progress.\n (run \"git revert --continue\" to continue)\n (use \"git revert --skip\" to skip this patch)\n (use \"git revert --abort\" to cancel the revert operation)\n\nnothing to commit, working tree clean\n/tmp$ git revert --continue\n## EDITOR pops up, as expected\n[master b8dd23f61d07] Revert \"whizzbang\"\n 1 file changed, 1 deletion(-)\nOn branch master\nRevert currently in progress.\n\nnothing to commit, working tree clean\n## Unexpected state; both reverts happened, but revert is still in progress?\n/tmp$ git log --oneline\nb8dd23f61d07 (HEAD -> master) Revert \"whizzbang\"\n586469974ec2 Revert \"nothing\"\n7153607b11e0 nothing\n51753222dd9a whizzbang\na388f78d9013 baz\n/tmp$ git status\nOn branch master\nRevert currently in progress.\n (run \"git revert --continue\" to continue)\n (use \"git revert --skip\" to skip this patch)\n (use \"git revert --abort\" to cancel the revert operation)\n\nnothing to commit, working tree clean\n## OK...I'll run it one more time.\n/tmp$ git revert --continue\n/tmp$ git status\nOn branch master\nnothing to commit, working tree clean\n## *Now* I'm done\n/tmp$ git log --oneline\nb8dd23f61d07 (HEAD -> master) Revert \"whizzbang\"\n586469974ec2 Revert \"nothing\"\n7153607b11e0 nothing\n51753222dd9a whizzbang\na388f78d9013 baz\n"},{"id":"386884","messageId":"20191123003454.GA23183@szeder.dev","threadId":"52319","inReplyTo":"CA+ASDXNQ+9i-6uCNDwN46cnHJeciiqQSX5Z6EaqB9pbseQhVFw@mail.gmail.com","subject":"Re: git 2.24: git revert requires extra '--continue'?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-23T00:34:54Z","receivedAt":"2019-11-23T00:35:02Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Nov 22, 2019 at 03:10:20PM -0800, Brian Norris wrote:\n> Hi! I'm using git 2.24 (or, a variant of that packaged my distro -- I\n> can try to build my own if this is deemed not reproducible), and I\n> feel like I've been seeing a regression here:\n> \n> Previously, when reverting multiple commits (with the default --edit\n> behavior), as soon as I'm done editing the first revert commit\n> message, the second revert commit pops up an editor, and I'm on my\n> way. Now, it seems to require an extra 'git revert --continue' prompt\n> in between, yet it doesn't actually recommend that. This is highly\n> confusing, and seemingly unnecessary.\n\nThanks for the report, this is indeed a regression in v2.24.0: it\nbisects down to a47ba3c777 (rebase -i: check for updated todo after\nsquash and reword, 2019-08-19) [Cc'ing Phillip]. It's not specific to\n'git revert', but with a slight variation affects 'git cherry-pick' as\nwell.\n\n> An annotated transcript provided below.\n\nI transcribed your transcript into tests that can be run in our test\nframework and demonstrate this regression:\n\n --- >8 ---\n\n#!/bin/sh\n\ntest_description='test'\n\n. ./test-lib.sh\n\ntest_expect_success \"Brian's revert regression\" '\n\ttest_create_repo revert &&\n\t(\n\t\tcd revert &&\n\n\t\techo 1 >file &&\n\t\tgit add file &&\n\t\tgit commit -m first &&\n\t\techo 2 >file &&\n\t\tgit commit -am second &&\n\t\techo 3 >file &&\n\t\tgit commit -am third &&\n\n\t\tgit checkout -b branch &&\n\n\t\tgit revert --edit HEAD HEAD^ &&\n\n\t\techo 1 >expect &&\n\t\ttest_cmp expect file\n\t)\n'\n\ntest_expect_success \"a variant of Brian's regression for cherry-pick\" '\n\ttest_create_repo cherry-pick &&\n\t(\n\t\tcd cherry-pick &&\n\n\t\techo 1 >file &&\n\t\tgit add file &&\n\t\tgit commit -m first &&\n\t\techo 2 >file &&\n\t\tgit commit -am second &&\n\t\techo 3 >file &&\n\t\tgit commit -am third &&\n\n\t\tgit checkout -b branch HEAD^^ &&\n\n\t\tgit cherry-pick --edit master^ master &&\n\n\t\techo 3 >expect &&\n\t\ttest_cmp expect file\n\t)\n'\n\ntest_done\n\n --- >8 ---\n\nThey both succeed on a47ba3c777's parent, but fail on a47ba3c777 when\nthe 'git revert' or 'git cherry-pick' commands return with exit code 1\nafter reverting/cherry-picking the first commit instead of processing\nthe second commit:\n\n + git revert --edit HEAD HEAD^\n [branch 88ea48c] Revert \"third\"\n Author: A U Thor \n 1 file changed, 1 insertion(+), 1 deletion(-)\n On branch branch\n Revert currently in progress.\n \n nothing to commit, working tree clean\n error: last command exited with $?=1\n not ok 1 - Brian's revert regression\n\n\n\n + git cherry-pick --edit master^ master\n [branch 2cb3f74] second\n Author: A U Thor \n Date: Sat Nov 23 00:17:32 2019 +0000\n 1 file changed, 1 insertion(+), 1 deletion(-)\n On branch branch\n Cherry-pick currently in progress.\n \n nothing to commit, working tree clean\n The previous cherry-pick is now empty, possibly due to conflict\n resolution.\n If you wish to commit it anyway, use:\n \n git commit --allow-empty\n \n If you wish to skip this commit, use:\n \n git reset\n \n Then \"git cherry-pick --continue\" will resume cherry-picking\n the remaining commits.\n error: last command exited with $?=1\n not ok 2 - a variant of Brian's regression for cherry-pick\n\n\nThese test should probably be squeezed into\n't3508-cherry-pick-many-commits.sh', but Friday has just turned into\nSaturday around here, so that's enough from me for now.\n\n\n> Note that none of this happens with --no-edit; my reverts happen\n> smoothly, with no extra need for --continue.\n> \n> Regards,\n> Brian\n> \n> $ mkdir tmp\n> $ cd tmp\n> /tmp$ git init\n> Initialized empty Git repository in [...]/tmp/.git/\n> /tmp$ touch foo\n> /tmp$ git add foo\n> /tmp$ echo bar >> foo\n> /tmp$ git commit -am baz\n> [master (root-commit) a388f78d9013] baz\n> 1 file changed, 1 insertion(+)\n> create mode 100644 foo\n> /tmp$ echo pow >> foo\n> /tmp$ git commit -am whizzbang\n> [master 51753222dd9a] whizzbang\n> 1 file changed, 1 insertion(+)\n> /tmp$ echo pop >> foo\n> /tmp$ git commit -am nothing\n> [master 7153607b11e0] nothing\n> 1 file changed, 1 insertion(+)\n> /tmp$ git revert HEAD HEAD^\n> ## EDITOR pops up, as expected\n> [master 586469974ec2] Revert \"nothing\"\n> 1 file changed, 1 deletion(-)\n> On branch master\n> Revert currently in progress.\n> \n> nothing to commit, working tree clean\n> ## Unexpected, confusing pause? No prompt to '--continue'\n> /tmp$ git log --oneline\n> 586469974ec2 (HEAD -> master) Revert \"nothing\"\n> 7153607b11e0 nothing\n> 51753222dd9a whizzbang\n> a388f78d9013 baz\n> /tmp$ git status\n> On branch master\n> Revert currently in progress.\n> (run \"git revert --continue\" to continue)\n> (use \"git revert --skip\" to skip this patch)\n> (use \"git revert --abort\" to cancel the revert operation)\n> \n> nothing to commit, working tree clean\n> /tmp$ git revert --continue\n> ## EDITOR pops up, as expected\n> [master b8dd23f61d07] Revert \"whizzbang\"\n> 1 file changed, 1 deletion(-)\n> On branch master\n> Revert currently in progress.\n> \n> nothing to commit, working tree clean\n> ## Unexpected state; both reverts happened, but revert is still in progress?\n> /tmp$ git log --oneline\n> b8dd23f61d07 (HEAD -> master) Revert \"whizzbang\"\n> 586469974ec2 Revert \"nothing\"\n> 7153607b11e0 nothing\n> 51753222dd9a whizzbang\n> a388f78d9013 baz\n> /tmp$ git status\n> On branch master\n> Revert currently in progress.\n> (run \"git revert --continue\" to continue)\n> (use \"git revert --skip\" to skip this patch)\n> (use \"git revert --abort\" to cancel the revert operation)\n> \n> nothing to commit, working tree clean\n> ## OK...I'll run it one more time.\n> /tmp$ git revert --continue\n> /tmp$ git status\n> On branch master\n> nothing to commit, working tree clean\n> ## *Now* I'm done\n> /tmp$ git log --oneline\n> b8dd23f61d07 (HEAD -> master) Revert \"whizzbang\"\n> 586469974ec2 Revert \"nothing\"\n> 7153607b11e0 nothing\n> 51753222dd9a whizzbang\n> a388f78d9013 baz\n"},{"id":"386900","messageId":"e7c01e0f-8466-c2c5-b53a-a93f941dfb1c@gmail.com","threadId":"52319","inReplyTo":"20191123003454.GA23183@szeder.dev","subject":"Re: git 2.24: git revert requires extra '--continue'?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-23T09:53:51Z","receivedAt":"2019-11-23T09:54:56Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Brian & Gábor\n\nThanks to Brian for reporting this and Gábor for bisecting and creating \nthe tests.\n\nOn 23/11/2019 00:34, SZEDER Gábor wrote:\n> On Fri, Nov 22, 2019 at 03:10:20PM -0800, Brian Norris wrote:\n>> Hi! I'm using git 2.24 (or, a variant of that packaged my distro -- I\n>> can try to build my own if this is deemed not reproducible), and I\n>> feel like I've been seeing a regression here:\n>>\n>> Previously, when reverting multiple commits (with the default --edit\n>> behavior), as soon as I'm done editing the first revert commit\n>> message, the second revert commit pops up an editor, and I'm on my\n>> way. Now, it seems to require an extra 'git revert --continue' prompt\n>> in between, yet it doesn't actually recommend that. This is highly\n>> confusing, and seemingly unnecessary.\n> \n> Thanks for the report, this is indeed a regression in v2.24.0: it\n> bisects down to a47ba3c777 (rebase -i: check for updated todo after\n> squash and reword, 2019-08-19) [Cc'ing Phillip]. It's not specific to\n> 'git revert', but with a slight variation affects 'git cherry-pick' as\n> well.\n\nThanks, I suspect it's missing an 'if (is_rebase_i(opts))' I'll take a \nlook later and come up with a fix\n\nBest Wishes\n\nPhillip\n\n>> An annotated transcript provided below.\n> \n> I transcribed your transcript into tests that can be run in our test\n> framework and demonstrate this regression:\n> \n> --- >8 ---\n> \n> #!/bin/sh\n> \n> test_description='test'\n> \n> . ./test-lib.sh\n> \n> test_expect_success \"Brian's revert regression\" '\n> \ttest_create_repo revert &&\n> \t(\n> \t\tcd revert &&\n> \n> \t\techo 1 >file &&\n> \t\tgit add file &&\n> \t\tgit commit -m first &&\n> \t\techo 2 >file &&\n> \t\tgit commit -am second &&\n> \t\techo 3 >file &&\n> \t\tgit commit -am third &&\n> \n> \t\tgit checkout -b branch &&\n> \n> \t\tgit revert --edit HEAD HEAD^ &&\n> \n> \t\techo 1 >expect &&\n> \t\ttest_cmp expect file\n> \t)\n> '\n> \n> test_expect_success \"a variant of Brian's regression for cherry-pick\" '\n> \ttest_create_repo cherry-pick &&\n> \t(\n> \t\tcd cherry-pick &&\n> \n> \t\techo 1 >file &&\n> \t\tgit add file &&\n> \t\tgit commit -m first &&\n> \t\techo 2 >file &&\n> \t\tgit commit -am second &&\n> \t\techo 3 >file &&\n> \t\tgit commit -am third &&\n> \n> \t\tgit checkout -b branch HEAD^^ &&\n> \n> \t\tgit cherry-pick --edit master^ master &&\n> \n> \t\techo 3 >expect &&\n> \t\ttest_cmp expect file\n> \t)\n> '\n> \n> test_done\n> \n> --- >8 ---\n> \n> They both succeed on a47ba3c777's parent, but fail on a47ba3c777 when\n> the 'git revert' or 'git cherry-pick' commands return with exit code 1\n> after reverting/cherry-picking the first commit instead of processing\n> the second commit:\n> \n> + git revert --edit HEAD HEAD^\n> [branch 88ea48c] Revert \"third\"\n> Author: A U Thor \n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> On branch branch\n> Revert currently in progress.\n> \n> nothing to commit, working tree clean\n> error: last command exited with $?=1\n> not ok 1 - Brian's revert regression\n> \n> \n> \n> + git cherry-pick --edit master^ master\n> [branch 2cb3f74] second\n> Author: A U Thor \n> Date: Sat Nov 23 00:17:32 2019 +0000\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> On branch branch\n> Cherry-pick currently in progress.\n> \n> nothing to commit, working tree clean\n> The previous cherry-pick is now empty, possibly due to conflict\n> resolution.\n> If you wish to commit it anyway, use:\n> \n> git commit --allow-empty\n> \n> If you wish to skip this commit, use:\n> \n> git reset\n> \n> Then \"git cherry-pick --continue\" will resume cherry-picking\n> the remaining commits.\n> error: last command exited with $?=1\n> not ok 2 - a variant of Brian's regression for cherry-pick\n> \n> \n> These test should probably be squeezed into\n> 't3508-cherry-pick-many-commits.sh', but Friday has just turned into\n> Saturday around here, so that's enough from me for now.\n> \n> \n>> Note that none of this happens with --no-edit; my reverts happen\n>> smoothly, with no extra need for --continue.\n>>\n>> Regards,\n>> Brian\n>>\n>> $ mkdir tmp\n>> $ cd tmp\n>> /tmp$ git init\n>> Initialized empty Git repository in [...]/tmp/.git/\n>> /tmp$ touch foo\n>> /tmp$ git add foo\n>> /tmp$ echo bar >> foo\n>> /tmp$ git commit -am baz\n>> [master (root-commit) a388f78d9013] baz\n>> 1 file changed, 1 insertion(+)\n>> create mode 100644 foo\n>> /tmp$ echo pow >> foo\n>> /tmp$ git commit -am whizzbang\n>> [master 51753222dd9a] whizzbang\n>> 1 file changed, 1 insertion(+)\n>> /tmp$ echo pop >> foo\n>> /tmp$ git commit -am nothing\n>> [master 7153607b11e0] nothing\n>> 1 file changed, 1 insertion(+)\n>> /tmp$ git revert HEAD HEAD^\n>> ## EDITOR pops up, as expected\n>> [master 586469974ec2] Revert \"nothing\"\n>> 1 file changed, 1 deletion(-)\n>> On branch master\n>> Revert currently in progress.\n>>\n>> nothing to commit, working tree clean\n>> ## Unexpected, confusing pause? No prompt to '--continue'\n>> /tmp$ git log --oneline\n>> 586469974ec2 (HEAD -> master) Revert \"nothing\"\n>> 7153607b11e0 nothing\n>> 51753222dd9a whizzbang\n>> a388f78d9013 baz\n>> /tmp$ git status\n>> On branch master\n>> Revert currently in progress.\n>> (run \"git revert --continue\" to continue)\n>> (use \"git revert --skip\" to skip this patch)\n>> (use \"git revert --abort\" to cancel the revert operation)\n>>\n>> nothing to commit, working tree clean\n>> /tmp$ git revert --continue\n>> ## EDITOR pops up, as expected\n>> [master b8dd23f61d07] Revert \"whizzbang\"\n>> 1 file changed, 1 deletion(-)\n>> On branch master\n>> Revert currently in progress.\n>>\n>> nothing to commit, working tree clean\n>> ## Unexpected state; both reverts happened, but revert is still in progress?\n>> /tmp$ git log --oneline\n>> b8dd23f61d07 (HEAD -> master) Revert \"whizzbang\"\n>> 586469974ec2 Revert \"nothing\"\n>> 7153607b11e0 nothing\n>> 51753222dd9a whizzbang\n>> a388f78d9013 baz\n>> /tmp$ git status\n>> On branch master\n>> Revert currently in progress.\n>> (run \"git revert --continue\" to continue)\n>> (use \"git revert --skip\" to skip this patch)\n>> (use \"git revert --abort\" to cancel the revert operation)\n>>\n>> nothing to commit, working tree clean\n>> ## OK...I'll run it one more time.\n>> /tmp$ git revert --continue\n>> /tmp$ git status\n>> On branch master\n>> nothing to commit, working tree clean\n>> ## *Now* I'm done\n>> /tmp$ git log --oneline\n>> b8dd23f61d07 (HEAD -> master) Revert \"whizzbang\"\n>> 586469974ec2 Revert \"nothing\"\n>> 7153607b11e0 nothing\n>> 51753222dd9a whizzbang\n>> a388f78d9013 baz\n"},{"id":"386908","messageId":"20191123172046.16359-1-szeder.dev@gmail.com","threadId":"52319","inReplyTo":"e7c01e0f-8466-c2c5-b53a-a93f941dfb1c@gmail.com","subject":"[PATCH] sequencer: don't re-read todo for revert and cherry-pick","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-23T17:20:46Z","receivedAt":"2019-11-23T17:21:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'git revert' or 'git cherry-pick --edit' is invoked with multiple\ncommits, then after editing the first commit message is finished both\nthese commands should continue with processing the second commit and\nlaunch another editor for its commit message, assuming there are\nno conflicts, of course.\n\nAlas, this inadvertently changed with commit a47ba3c777 (rebase -i:\ncheck for updated todo after squash and reword, 2019-08-19): after\nediting the first commit message is finished, both 'git revert' and\n'git cherry-pick --edit' exit with error, claiming that \"nothing to\ncommit, working tree clean\".\n\nThe reason for the changed behaviour is twofold:\n\n - Prior to a47ba3c777 the up-to-dateness of the todo list file was\n only checked after 'exec' instructions, and that commit moved\n those checks to the common code path. The intention was that this\n check should be performed after instructions spawning an editor\n ('squash' and 'reword') as well, so the ongoing 'rebase -i'\n notices when the user runs a 'git rebase --edit-todo' while\n squashing/rewording a commit message.\n\n However, as it happened that check is now performed even after\n 'revert' and 'pick' instructions when they involved editing the\n commit message. And 'revert' by default while 'pick' optionally\n (with 'git cherry-pick --edit') involves editing the commit\n message.\n\n - When invoking 'git revert' or 'git cherry-pick --edit' with\n multiple commits they don't read a todo list file but assemble the\n todo list in memory, thus the associated stat data used to check\n whether the file has been updated is all zeroed out initially.\n\n Then the sequencer writes all instructions (including the very\n first) to the todo file, executes the first 'revert/pick'\n instruction, and after the user finished editing the commit\n message the changes of a47ba3c777 kick in, and it checks whether\n the todo file has been modified. The initial all-zero stat data\n obviously differs from the todo file's current stat data, so the\n sequencer concludes that the file has been modified. Technically\n it is not wrong, of course, because the file just has been written\n indeed by the sequencer itself, though the file's contents still\n match what the sequencer was invoked with in the beginning.\n Consequently, after re-reading the todo file the sequencer\n executes the same first instruction _again_, thus ending up in\n that \"nothing to commit\" situation.\n\nThe todo list was never meant to be edited during multi-commit 'git\nrevert' or 'cherry-pick' operations, so perform that \"has the todo\nfile been modified\" check only when the sequencer was invoked as part\nof an interactive rebase.\n\nReported-by: Brian Norris \nSigned-off-by: SZEDER Gábor \n---\n\nOn Sat, Nov 23, 2019 at 09:53:51AM +0000, Phillip Wood wrote:\n> Thanks, I suspect it's missing an 'if (is_rebase_i(opts))' I'll take a look\n> later and come up with a fix\n\nThat missing condition was my hunch yesterday evening as well, and\nwhile it did fix my tests and didn't break anything else, it took some\ntime to wrap my head around some of the subtleties that are going on\nin the sequencer. That's why the commit message ended up this long as\nit did.\n\nIn the end I decided to add the new tests to\n't3429-rebase-edit-todo.sh', because, though these new tests don't\nactually check 'rebase', that is the one test script focusing on\n(re-)reading the todo file in particular.\n\n sequencer.c | 2 +-\n t/t3429-rebase-edit-todo.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 2adcf5a639..3b05d0277d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3791,7 +3791,7 @@ static int pick_commits(struct repository *r,\n \t\t\t\t\t\t\titem->commit,\n \t\t\t\t\t\t\targ, item->arg_len,\n \t\t\t\t\t\t\topts, res, 0);\n-\t\t} else if (check_todo && !res) {\n+\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n \t\t\tstruct stat st;\n \n \t\t\tif (stat(get_todo_path(opts), &st)) {\ndiff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\nindex 8739cb60a7..1679f2563d 100755\n--- a/t/t3429-rebase-edit-todo.sh\n+++ b/t/t3429-rebase-edit-todo.sh\n@@ -52,4 +52,34 @@ test_expect_success 'todo is re-read after reword and squash' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 're-reading todo doesnt interfere with revert --edit' '\n+\tgit reset --hard third &&\n+\n+\tgit revert --edit third second &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tRevert \"second\"\n+\tRevert \"third\"\n+\tthird\n+\tsecond\n+\tfirst\n+\tEOF\n+\tgit log --format=\"%s\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 're-reading todo doesnt interfere with cherry-pick --edit' '\n+\tgit reset --hard first &&\n+\n+\tgit cherry-pick --edit second third &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tthird\n+\tsecond\n+\tfirst\n+\tEOF\n+\tgit log --format=\"%s\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.24.0.532.ge18579ded8\n\n"},{"id":"386924","messageId":"nycvar.QRO.7.76.6.1911232214080.31080@tvgsbejvaqbjf.bet","threadId":"52319","inReplyTo":"20191123172046.16359-1-szeder.dev@gmail.com","subject":"Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-23T21:14:54Z","receivedAt":"2019-11-23T21:15:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 23 Nov 2019, SZEDER Gábor wrote:\n\n> When 'git revert' or 'git cherry-pick --edit' is invoked with multiple\n> commits, then after editing the first commit message is finished both\n> these commands should continue with processing the second commit and\n> launch another editor for its commit message, assuming there are\n> no conflicts, of course.\n>\n> Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:\n> check for updated todo after squash and reword, 2019-08-19): after\n> editing the first commit message is finished, both 'git revert' and\n> 'git cherry-pick --edit' exit with error, claiming that \"nothing to\n> commit, working tree clean\".\n>\n> The reason for the changed behaviour is twofold:\n>\n> - Prior to a47ba3c777 the up-to-dateness of the todo list file was\n> only checked after 'exec' instructions, and that commit moved\n> those checks to the common code path. The intention was that this\n> check should be performed after instructions spawning an editor\n> ('squash' and 'reword') as well, so the ongoing 'rebase -i'\n> notices when the user runs a 'git rebase --edit-todo' while\n> squashing/rewording a commit message.\n>\n> However, as it happened that check is now performed even after\n> 'revert' and 'pick' instructions when they involved editing the\n> commit message. And 'revert' by default while 'pick' optionally\n> (with 'git cherry-pick --edit') involves editing the commit\n> message.\n>\n> - When invoking 'git revert' or 'git cherry-pick --edit' with\n> multiple commits they don't read a todo list file but assemble the\n> todo list in memory, thus the associated stat data used to check\n> whether the file has been updated is all zeroed out initially.\n>\n> Then the sequencer writes all instructions (including the very\n> first) to the todo file, executes the first 'revert/pick'\n> instruction, and after the user finished editing the commit\n> message the changes of a47ba3c777 kick in, and it checks whether\n> the todo file has been modified. The initial all-zero stat data\n> obviously differs from the todo file's current stat data, so the\n> sequencer concludes that the file has been modified. Technically\n> it is not wrong, of course, because the file just has been written\n> indeed by the sequencer itself, though the file's contents still\n> match what the sequencer was invoked with in the beginning.\n> Consequently, after re-reading the todo file the sequencer\n> executes the same first instruction _again_, thus ending up in\n> that \"nothing to commit\" situation.\n>\n> The todo list was never meant to be edited during multi-commit 'git\n> revert' or 'cherry-pick' operations, so perform that \"has the todo\n> file been modified\" check only when the sequencer was invoked as part\n> of an interactive rebase.\n>\n> Reported-by: Brian Norris \n> Signed-off-by: SZEDER Gábor \n> ---\n\nThe patch and the commit message (thank you so much for the detail!) look\nvery good to me.\n\n> On Sat, Nov 23, 2019 at 09:53:51AM +0000, Phillip Wood wrote:\n> > Thanks, I suspect it's missing an 'if (is_rebase_i(opts))' I'll take a look\n> > later and come up with a fix\n>\n> That missing condition was my hunch yesterday evening as well, and\n> while it did fix my tests and didn't break anything else, it took some\n> time to wrap my head around some of the subtleties that are going on\n> in the sequencer. That's why the commit message ended up this long as\n> it did.\n>\n> In the end I decided to add the new tests to\n> 't3429-rebase-edit-todo.sh', because, though these new tests don't\n> actually check 'rebase', that is the one test script focusing on\n> (re-)reading the todo file in particular.\n\nYup, makes sense.\n\nThanks,\nDscho\n\n>\n> sequencer.c | 2 +-\n> t/t3429-rebase-edit-todo.sh | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 31 insertions(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 2adcf5a639..3b05d0277d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3791,7 +3791,7 @@ static int pick_commits(struct repository *r,\n> \t\t\t\t\t\t\titem->commit,\n> \t\t\t\t\t\t\targ, item->arg_len,\n> \t\t\t\t\t\t\topts, res, 0);\n> -\t\t} else if (check_todo && !res) {\n> +\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n> \t\t\tstruct stat st;\n>\n> \t\t\tif (stat(get_todo_path(opts), &st)) {\n> diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\n> index 8739cb60a7..1679f2563d 100755\n> --- a/t/t3429-rebase-edit-todo.sh\n> +++ b/t/t3429-rebase-edit-todo.sh\n> @@ -52,4 +52,34 @@ test_expect_success 'todo is re-read after reword and squash' '\n> \ttest_cmp expected actual\n> '\n>\n> +test_expect_success 're-reading todo doesnt interfere with revert --edit' '\n> +\tgit reset --hard third &&\n> +\n> +\tgit revert --edit third second &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\tRevert \"second\"\n> +\tRevert \"third\"\n> +\tthird\n> +\tsecond\n> +\tfirst\n> +\tEOF\n> +\tgit log --format=\"%s\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 're-reading todo doesnt interfere with cherry-pick --edit' '\n> +\tgit reset --hard first &&\n> +\n> +\tgit cherry-pick --edit second third &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\tthird\n> +\tsecond\n> +\tfirst\n> +\tEOF\n> +\tgit log --format=\"%s\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.24.0.532.ge18579ded8\n>\n>\n"},{"id":"386928","messageId":"xmqqk17p280y.fsf@gitster-ct.c.googlers.com","threadId":"52319","inReplyTo":"20191123172046.16359-1-szeder.dev@gmail.com","subject":"Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-24T04:49:33Z","receivedAt":"2019-11-24T04:51:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> When 'git revert' or 'git cherry-pick --edit' is invoked with multiple\n> commits, then after editing the first commit message is finished both\n\n\"commits, then after editing the first commit message, both of\" I\nwould say.\n\n> these commands should continue with processing the second commit and\n> launch another editor for its commit message, assuming there are\n> no conflicts, of course.\n>\n> Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:\n> check for updated todo after squash and reword, 2019-08-19): after\n> editing the first commit message is finished, both 'git revert' and\n> 'git cherry-pick --edit' exit with error, claiming that \"nothing to\n> commit, working tree clean\".\n> ...\n> - When invoking 'git revert' or 'git cherry-pick --edit' with\n> multiple commits they don't read a todo list file but assemble the\n> todo list in memory, thus the associated stat data used to check\n> whether the file has been updated is all zeroed out initially.\n>\n> Then the sequencer writes all instructions (including the very\n> first) to the todo file, executes the first 'revert/pick'\n> instruction, and after the user finished editing the commit\n> message the changes of a47ba3c777 kick in, and it checks whether\n> the todo file has been modified. The initial all-zero stat data\n> obviously differs from the todo file's current stat data, so the\n> sequencer concludes that the file has been modified. Technically\n> it is not wrong, of course, because the file just has been written\n> indeed by the sequencer itself, though the file's contents still\n> match what the sequencer was invoked with in the beginning.\n> Consequently, after re-reading the todo file the sequencer\n> executes the same first instruction _again_, thus ending up in\n> that \"nothing to commit\" situation.\n\nHmph, that makes it sound as if the right fix is to re-read after\nwriting the first version of the todo file out, so that the stat\ndata matches reality and tells us that it has never been modified?\n\n> The todo list was never meant to be edited during multi-commit 'git\n> revert' or 'cherry-pick' operations, so perform that \"has the todo\n> file been modified\" check only when the sequencer was invoked as part\n> of an interactive rebase.\n\nOK. That is a valid fix, I think, but the explanation given in the\nsecond bullet point gives a wrong impression that it is merely a\nworkaround (iow, we are assuming that the user would behave, instead\nof making sure we can detect when the user touches the list), when\nit is *not*.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 2adcf5a639..3b05d0277d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3791,7 +3791,7 @@ static int pick_commits(struct repository *r,\n> \t\t\t\t\t\t\titem->commit,\n> \t\t\t\t\t\t\targ, item->arg_len,\n> \t\t\t\t\t\t\topts, res, 0);\n> -\t\t} else if (check_todo && !res) {\n> +\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n\nIt is a bit sad that setting of check_todo is not something a single\nhelper function can decide, so that this is_rebase_i(opts) can be\ntaken into account when that helper function (the logical place\nwould be do_pick_commit()) decides to set (or not set) check_todo.\n\nUnfortunately, that is not sufficient, I suspect. Why did a47ba3c7\n(\"rebase -i: check for updated todo after squash and reword\",\n2019-08-19) decide to flip check_todo on when running TODO_EXEC?\n\n"},{"id":"386935","messageId":"8c21662f-6548-a46e-9c87-eb364355cb78@gmail.com","threadId":"52319","inReplyTo":"xmqqk17p280y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-24T10:44:10Z","receivedAt":"2019-11-24T10:44:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/11/2019 04:49, Junio C Hamano wrote:\n\nThanks for working on this Gábor\n\n> SZEDER Gábor writes:\n> \n>> When 'git revert' or 'git cherry-pick --edit' is invoked with multiple\n>> commits, then after editing the first commit message is finished both\n> \n> \"commits, then after editing the first commit message, both of\" I\n> would say.\n> \n>> these commands should continue with processing the second commit and\n>> launch another editor for its commit message, assuming there are\n>> no conflicts, of course.\n>>\n>> Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:\n>> check for updated todo after squash and reword, 2019-08-19): after\n>> editing the first commit message is finished, both 'git revert' and\n>> 'git cherry-pick --edit' exit with error, claiming that \"nothing to\n>> commit, working tree clean\".\n>> ...\n>> - When invoking 'git revert' or 'git cherry-pick --edit' with\n>> multiple commits they don't read a todo list file but assemble the\n>> todo list in memory, thus the associated stat data used to check\n>> whether the file has been updated is all zeroed out initially.\n>>\n>> Then the sequencer writes all instructions (including the very\n>> first) to the todo file, executes the first 'revert/pick'\n>> instruction, and after the user finished editing the commit\n>> message the changes of a47ba3c777 kick in, and it checks whether\n>> the todo file has been modified. The initial all-zero stat data\n>> obviously differs from the todo file's current stat data, so the\n>> sequencer concludes that the file has been modified. Technically\n>> it is not wrong, of course, because the file just has been written\n>> indeed by the sequencer itself, though the file's contents still\n>> match what the sequencer was invoked with in the beginning.\n>> Consequently, after re-reading the todo file the sequencer\n>> executes the same first instruction _again_, thus ending up in\n>> that \"nothing to commit\" situation.\n> \n> Hmph, that makes it sound as if the right fix is to re-read after\n> writing the first version of the todo file out, so that the stat\n> data matches reality and tells us that it has never been modified?\n\nI think we should update the stat data after we write the todo list. \nag/sequencer-todo-updates changes the rebase startup sequence to avoid \nthe initial read that populates the stat data. There's a subtle \ndifference in the todo list handling between rabese and \ncherry-pick/revert. The list written by rebase does not include the \ncommit currently being picked but it does for cherry-pick/revert. This \nmeans that rebase is not subject to this bug in \nag/sequencer-todo-updates as although it re-reads the todo list because \nthe stat data is zero the list does not contain the commit we've just \npicked.\n\n> \n>> The todo list was never meant to be edited during multi-commit 'git\n>> revert' or 'cherry-pick' operations, so perform that \"has the todo\n>> file been modified\" check only when the sequencer was invoked as part\n>> of an interactive rebase.\n> \n> OK. That is a valid fix, I think, but the explanation given in the\n> second bullet point gives a wrong impression that it is merely a\n> workaround (iow, we are assuming that the user would behave, instead\n> of making sure we can detect when the user touches the list), when\n> it is *not*.\n> \n\nI'd be happier not to set check_todo in the first place unless we're \nrebasing (though one could argue that Gábor's version is more future-proof)\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 2adcf5a639..3b05d0277d 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -3791,7 +3791,7 @@ static int pick_commits(struct repository *r,\n>> \t\t\t\t\t\t\titem->commit,\n>> \t\t\t\t\t\t\targ, item->arg_len,\n>> \t\t\t\t\t\t\topts, res, 0);\n>> -\t\t} else if (check_todo && !res) {\n>> +\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n> \n> It is a bit sad that setting of check_todo is not something a single\n> helper function can decide, so that this is_rebase_i(opts) can be\n> taken into account when that helper function (the logical place\n> would be do_pick_commit()) decides to set (or not set) check_todo.\n> \n> Unfortunately, that is not sufficient, I suspect. Why did a47ba3c7\n> (\"rebase -i: check for updated todo after squash and reword\",\n> 2019-08-19) decide to flip check_todo on when running TODO_EXEC?\n\nI'm not sure what you mean by this\n\nThis is what I had before I saw Gábor's patch (the tests are pretty \nsimilar but I think we should check that the messages of all the picks \nare actually edited with --edit - that does not seem to be checked by \nthe current tests) Note this does not include updating the stat data \nafter writing the todo list yet as I've only just thought about that.\nGábor are you happy to take this forward?\n\nBest Wishes\n\nPhillip\n\n\n--- 8> ---\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f2abe2a366..b363b45366 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1999,7 +1999,7 @@ static int do_pick_commit(struct repository *r,\n res = do_commit(r, msg_file, author, opts, flags);\n else\n res = error(_(\"unable to parse commit author\"));\n- *check_todo = !!(flags & EDIT_MSG);\n+ *check_todo = is_rebase_i(opts) && !!(flags & EDIT_MSG);\n if (!res && reword) {\n fast_forward_edit:\n res = run_git_commit(r, NULL, opts, EDIT_MSG |\ndiff --git a/t/t3508-cherry-pick-many-commits.sh \nb/t/t3508-cherry-pick-many-commits.sh\nindex b457333e18..fd9d626779 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -40,6 +40,31 @@ test_expect_success 'cherry-pick first..fourth works' '\n check_head_differs_from fourth\n '\n\n+test_expect_success 'cherry-pick --edit first..fourth works' '\n+ git checkout -f master &&\n+ git reset --hard first &&\n+ test_tick &&\n+ GIT_EDITOR=\"echo edited >>\" git cherry-pick --edit first..fourth &&\n+ git log --pretty=format:%B -3 >actual &&\n+ cat >expect <<-\\EOF &&\n+ fourth\n+\n+ edited\n+\n+ third\n+\n+ edited\n+\n+ second\n+\n+ edited\n+ EOF\n+ test_cmp expect actual &&\n+ git diff --quiet other &&\n+ git diff --quiet HEAD other &&\n+ check_head_differs_from fourth\n+'\n+\n test_expect_success 'cherry-pick three one two works' '\n git checkout -f first &&\n test_commit one &&\n@@ -153,6 +178,37 @@ test_expect_success 'revert first..fourth works' '\n git diff --quiet HEAD first\n '\n\n+test_expect_success 'revert --edit first..fourth works' '\n+ git checkout -f master &&\n+ git reset --hard fourth &&\n+ test_tick &&\n+ GIT_EDITOR=\"echo edited >>\" git revert --edit first..fourth &&\n+ git log --pretty=format:%B -3 >actual &&\n+ cat >expect <<-EOF &&\n+ Revert \"second\"\n+\n+ This reverts commit $(git rev-parse second).\n+\n+ edited\n+\n+ Revert \"third\"\n+\n+ This reverts commit $(git rev-parse third).\n+\n+ edited\n'\n\n+test_expect_success 'revert --edit first..fourth works' '\n+ git checkout -f master &&\n+ git reset --hard fourth &&\n+ test_tick &&\n+ GIT_EDITOR=\"echo edited >>\" git revert --edit first..fourth &&\n+ git log --pretty=format:%B -3 >actual &&\n+ cat >expect <<-EOF &&\n+ Revert \"second\"\n+\n+ This reverts commit $(git rev-parse second).\n+\n+ edited\n+\n+ Revert \"third\"\n+\n+ This reverts commit $(git rev-parse third).\n+\n+ edited\n+\n+ Revert \"fourth\"\n+\n+ This reverts commit $(git rev-parse fourth).\n+\n+ edited\n+ EOF\n+ test_cmp expect actual &&\n+ git diff --quiet first &&\n+ git diff --cached --quiet first &&\n+ git diff --quiet HEAD first\n+'\n+\n test_expect_success 'revert ^first fourth works' '\n git checkout -f master &&\n git reset --hard fourth &&\n\n\n\n"},{"id":"386957","messageId":"20191124211021.GB23183@szeder.dev","threadId":"52319","inReplyTo":"8c21662f-6548-a46e-9c87-eb364355cb78@gmail.com","subject":"[PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-24T21:10:21Z","receivedAt":"2019-11-24T21:10:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Nov 24, 2019 at 10:44:10AM +0000, Phillip Wood wrote:\n> On 24/11/2019 04:49, Junio C Hamano wrote:\n> \n> Thanks for working on this Gábor\n> \n> >SZEDER Gábor writes:\n> >\n> >>When 'git revert' or 'git cherry-pick --edit' is invoked with multiple\n> >>commits, then after editing the first commit message is finished both\n> >\n> >\"commits, then after editing the first commit message, both of\" I\n> >would say.\n> >\n> >>these commands should continue with processing the second commit and\n> >>launch another editor for its commit message, assuming there are\n> >>no conflicts, of course.\n> >>\n> >>Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:\n> >>check for updated todo after squash and reword, 2019-08-19): after\n> >>editing the first commit message is finished, both 'git revert' and\n> >>'git cherry-pick --edit' exit with error, claiming that \"nothing to\n> >>commit, working tree clean\".\n> >>...\n> >> - When invoking 'git revert' or 'git cherry-pick --edit' with\n> >> multiple commits they don't read a todo list file but assemble the\n> >> todo list in memory, thus the associated stat data used to check\n> >> whether the file has been updated is all zeroed out initially.\n> >>\n> >> Then the sequencer writes all instructions (including the very\n> >> first) to the todo file, executes the first 'revert/pick'\n> >> instruction, and after the user finished editing the commit\n> >> message the changes of a47ba3c777 kick in, and it checks whether\n> >> the todo file has been modified. The initial all-zero stat data\n> >> obviously differs from the todo file's current stat data, so the\n> >> sequencer concludes that the file has been modified. Technically\n> >> it is not wrong, of course, because the file just has been written\n> >> indeed by the sequencer itself, though the file's contents still\n> >> match what the sequencer was invoked with in the beginning.\n> >> Consequently, after re-reading the todo file the sequencer\n> >> executes the same first instruction _again_, thus ending up in\n> >> that \"nothing to commit\" situation.\n> >\n> >Hmph, that makes it sound as if the right fix is to re-read after\n> >writing the first version of the todo file out, so that the stat\n> >data matches reality and tells us that it has never been modified?\n> \n> I think we should update the stat data after we write the todo list.\n\nWell, yes and no.\n\nNo, because we are dealing with regression in v2.24.0 here, so the\nsimpler the fix the better it is for maint. I don't think a fix can\nget any simpler than my patch, with or without the suggestions from\nPhillip.\n\nYes, we should definitely consider updating the stat data after the\nsequencer writes the todo list, or any other options with which the\nsequencer could notice a modified todo list file with less subtlety.\nAlas, there is a big can of worms in that direction, see the patch\nbelow, and we have to be very careful going that way, so I think it's\nonly viable in the long term, but less suitable as a regression fix\nfor maint.\n\n(Hrm, perhaps I spent too many words on the all zeroed out stat data,\nand managed to sidetrack you a bit...)\n\n --- >8 ---\n\nSubject: [PATCH] t3429: try to protect against a potential racy todo file\n problem\n\nDuring an interactive rebase session the user can run basically any\ncommands with an 'exec' instruction, including commands that modify\nthe todo list of the ongoing rebase. Similarly, the user might choose\nto modify the todo list while editing a commit message for a 'reword'\nor a 'squash' instruction. The ongoing interactive rebase process\nshould be able to notice the modified todo file and re-read it to\nfollow the updated instructions. This has been working well for the\n'exec' instruction for some time, but for 'reword' and 'squash' it has\nbeen fixed only recently in a47ba3c777 (rebase -i: check for updated\ntodo after squash and reword, 2019-08-19).\n\nTo notice a changed todo file the sequencer stores the file's stat\ndata in its 'struct todo_list' instance, and compares it with the\nfile's current stat data after 'reword', 'squash' and 'exec'\ninstructions. If the two stat data doesn't match, it re-reads the\ntodo file.\n\nSounds simple, but there are some subtleties going on here:\n\n - The 'struct todo_list' holds the stat data from the time when the\n todo file was last read.\n\n - This stat data in 'struct todo_list' is not updated when the\n sequencer itself writes the todo file.\n\n - Before executing each instruction during an interactive rebase,\n the sequencer always updates the todo file by removing the\n just-about-to-be-executed instruction. This changes the file's\n size and inode [1].\n\nConsequently, when the sequencer looks at the stat data after a\n'reword', 'squash' or 'exec' instruction, it most likely finds that\nthey differ, even when the user didn't modify the todo list at all!\nThis is not an issue in practice, it just wastes a few cycles on\nre-reading the todo list that matches what the sequencer already has\nin memory anyway.\n\nHowever, an unsuspecting Git developer might try to \"fix\" it by simply\nupdating the stat data each time the sequencer writes the todo list\nfor an interactive rebase. On first sight it looks quite sensible and\nstraightforward, but we have to be very careful when doing that,\nbecause potential racy problems lie that way.\n\nIt is possible to overwrite the todo list file without modifying\neither its inode or size, and if such an overwrite were to happen in\nthe same second when the file was last read (our stat data has one\nsecond granularity by default), then the actual stat data on the file\nsystem would match the stat data that the sequencer has in memory.\nConsequently, such a modification to the todo list file would go\nunnoticed.\n\nAdd a test to 't3429-rebase-edit-todo.sh' that modifies the todo list\nduring an interactive rebase in a way that the file's stat data very\nlikely doesn't change. Since we have no control over ctime repeat the\ntest a few times, just like we do in the racy git and racy split index\ntests (t0010 and t1701). This test succeeds right now because of all\nthe above, but it does fail most of the time if the stat data is\nnaively updated when writing the todo file [2]. Hopefully this test\nwill help prevent us from introducing such racy problems if we ever\nchange how the sequencer looks out for modified todo list files.\n\nThis new test only checks what it's supposed to when it doesn't change\nthe size of the todo file while modifying it, thus it critically\ndepends on the todo file format. And the todo file format did change\nin the past: early on it stored abbreviated commit object ids, but\nsince 75c6976655 (rebase -i: fix short SHA-1 collision, 2013-08-23) it\nstores full object ids. Perhaps it will change in the future as well,\nso be extra defensive about this, and check that the file size before\nand after the edit actually match, and complain with a \"bug in the\ntest script\" if they don't.\n\n[1] The todo file is written using the lockfile API, i.e. first to the\n lockfile, which is then moved to place, so the new file can't\n possibly have the same inode as the file it replaces. Note,\n however, that the file system might reuse inodes, so it is\n possible that the new todo file ends up with the same inode as is\n recorded in the 'struct todo_list' from the last time the file was\n read.\n\n[2] To trigger failure in the new test updating the stat data when\n writing the todo file like the diff below does. No other test in\n our test suite seems to fail with this change applied.\n\n diff --git a/sequencer.c b/sequencer.c\n index 3b05d0277d..b2acb7d536 100644\n --- a/sequencer.c\n +++ b/sequencer.c\n @@ -2730,6 +2730,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)\n \tstruct lock_file todo_lock = LOCK_INIT;\n \tconst char *todo_path = get_todo_path(opts);\n \tint next = todo_list->current, offset, fd;\n +\tstruct stat st;\n\n \t/*\n \t * rebase -i writes \"git-rebase-todo\" without the currently executing\n @@ -2748,6 +2749,10 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)\n \tif (commit_lock_file(&todo_lock) < 0)\n \t\treturn error(_(\"failed to finalize '%s'\"), todo_path);\n\n +\tif (stat(todo_path, &st) < 0)\n +\t\treturn error(_(\"could not stat '%s'\"), todo_path);\n +\tfill_stat_data(&todo_list->stat, &st);\n +\n \tif (is_rebase_i(opts) && next > 0) {\n \t\tconst char *done = rebase_path_done();\n \t\tint fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);\n\nSigned-off-by: SZEDER Gábor \n---\n t/t3429-rebase-edit-todo.sh | 59 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 59 insertions(+)\n\ndiff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\nindex 8739cb60a7..c80c139b47 100755\n--- a/t/t3429-rebase-edit-todo.sh\n+++ b/t/t3429-rebase-edit-todo.sh\n@@ -52,4 +52,63 @@ test_expect_success 'todo is re-read after reword and squash' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'setup for racy tests' '\n+\twrite_script sequence-editor <<-EOS &&\n+\t\tcat >.git/rebase-merge/git-rebase-todo <<-\\EOF\n+\t\t\tr $(git rev-parse second^0) second\n+\t\t\tr $(git rev-parse third^0) third\n+\t\tEOF\n+\tEOS\n+\n+\twrite_script commit-editor <<-\\EOS &&\n+\t\tread first_line <\"$1\" &&\n+\t\techo \"$first_line - edited\" >\"$1\" &&\n+\n+\t\ttodo=.git/rebase-merge/git-rebase-todo &&\n+\n+\t\tif test \"$first_line\" = second\n+\t\tthen\n+\t\t\told_size=$(wc -c <\"$todo\") &&\n+\t\t\t# Replace the \"reword third\" line with\n+\t\t\t# a different instruction of the same line length.\n+\t\t\t# Overwrite the file in-place, so the inode stays\n+\t\t\t# the same as well.\n+\t\t\tcat >\"$todo\" <<-EOF &&\n+\t\t\t\texec echo 0123456789012345678901234 >exec-cmd-was-run\n+\t\t\tEOF\n+\t\t\tnew_size=$(wc -c <\"$todo\") &&\n+\n+\t\t\tif test $old_size -ne $new_size\n+\t\t\tthen\n+\t\t\t\techo >&2 \"error: bug in the test script: the size of the todo list must not change\"\n+\t\t\t\texit 1\n+\t\t\tfi\n+\t\tfi\n+\tEOS\n+\n+\tcat >expect <<-\\EOF\n+\tsecond - edited\n+\tfirst\n+\tEOF\n+'\n+\n+# This test can give false success if your machine is sufficiently\n+# slow or all trials happened to happen on second boundaries.\n+\n+for trial in 0 1 2 3 4\n+do\n+\ttest_expect_success \"racy todo re-read #$trial\" '\n+\t\tgit reset --hard third &&\n+\t\trm -rf exec-cmd-was-run &&\n+\n+\t\tGIT_SEQUENCE_EDITOR=./sequence-editor \\\n+\t\tGIT_EDITOR=./commit-editor \\\n+\t\tgit rebase -i HEAD^^ &&\n+\n+\t\ttest_path_is_file exec-cmd-was-run &&\n+\t\tgit log --format=%s >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_done\n-- \n2.24.0.532.ge18579ded8\n\n"},{"id":"386959","messageId":"xmqqo8x0zros.fsf@gitster-ct.c.googlers.com","threadId":"52319","inReplyTo":"8c21662f-6548-a46e-9c87-eb364355cb78@gmail.com","subject":"Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-25T01:10:43Z","receivedAt":"2019-11-25T01:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> -\t\t} else if (check_todo && !res) {\n>>> +\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n>>\n>> It is a bit sad that setting of check_todo is not something a single\n>> helper function can decide, so that this is_rebase_i(opts) can be\n>> taken into account when that helper function (the logical place\n>> would be do_pick_commit()) decides to set (or not set) check_todo.\n>>\n>> Unfortunately, that is not sufficient, I suspect. Why did a47ba3c7\n>> (\"rebase -i: check for updated todo after squash and reword\",\n>> 2019-08-19) decide to flip check_todo on when running TODO_EXEC?\n>\n> I'm not sure what you mean by this\n>\n> This is what I had before I saw Gábor's patch (the tests are pretty\n> similar but I think we should check that the messages of all the picks\n> are actually edited with --edit - that does not seem to be checked by\n> the current tests)...\n\nI first thought that unsetting *check_todo in do_pick_commit(), when\n!is_rebase_i(), was a clean solution. But sadly it is *not* a godo\nequivalent to Gábor's patch, because check_todo can be set to true\nwithout looking at is_rebase_i() in pick_commits() [*1*]. To ignore\nthat setting where the variable's value is used, the hunk we see\nabove in the beginning of this message is necessary.\n\nThat was what I meant. I think the \"This is what I had before\"\npatch matches my \"I first thought\" version, so we were on the same\npage and both wrong ;-)\n\nThanks.\n\n\n[Footnote]\n\n*1* I still do not know why a47ba3c7 (\"rebase -i: check for updated\ntodo after squash and reword\", 2019-08-19) sets check_todo to true\nwithout looking at is_rebase_i(). If this unconditonal setting in\nTODO_EXEC did not exist, I think your \"This is what I had before\"\npatch would have been equivalent to Gábor's patch.\n\n"},{"id":"386960","messageId":"xmqqk17ozqvy.fsf@gitster-ct.c.googlers.com","threadId":"52319","inReplyTo":"20191124211021.GB23183@szeder.dev","subject":"Re: [PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-25T01:28:01Z","receivedAt":"2019-11-25T01:28:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Sun, Nov 24, 2019 at 10:44:10AM +0000, Phillip Wood wrote:\n>> On 24/11/2019 04:49, Junio C Hamano wrote:\n>> ...\n>> >Hmph, that makes it sound as if the right fix is to re-read after\n>> >writing the first version of the todo file out, so that the stat\n>> >data matches reality and tells us that it has never been modified?\n>> \n>> I think we should update the stat data after we write the todo list.\n>\n> Well, yes and no.\n>\n> No, because we are dealing with regression in v2.24.0 here, so the\n> simpler the fix the better it is for maint. I don't think a fix can\n> get any simpler than my patch, with or without the suggestions from\n> Phillip.\n\nOf course, the simplest \"fix\" for regression is to revert the\noffending one, and anything else is a band-aid ;-). The question is\nwhich band-aid is the least risky and which one takes us the closest\nto the real solution. I tend to agree that forcing to skip checking\nno matter what the variable \"check_todo\" says unless is_rebase_i()\nqualifies as the band-aid that is the least risky.\n\n> Yes, we should definitely consider updating the stat data after the\n> sequencer writes the todo list, or any other options with which the\n> sequencer could notice a modified todo list file with less subtlety.\n> Alas, there is a big can of worms in that direction, see the patch\n> below, and we have to be very careful going that way, so I think it's\n> only viable in the long term, but less suitable as a regression fix\n> for maint.\n\nYes, I agree that it is much less suitable than even reverting the\noffending one outright.\n\n> (Hrm, perhaps I spent too many words on the all zeroed out stat data,\n> and managed to sidetrack you a bit...)\n\nNo, I do not think so. Thinking about what we need to do in the\nlonger term, while coming up with a shorter term fix, is a necessary\nstep of gaining confidence in the latter.\n\nAgain, thanks both for thinking about this issue.\n"},{"id":"386968","messageId":"20191125031005.GC23183@szeder.dev","threadId":"52319","inReplyTo":"20191124211021.GB23183@szeder.dev","subject":"Re: [PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-25T03:10:05Z","receivedAt":"2019-11-25T03:10:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Nov 24, 2019 at 10:10:21PM +0100, SZEDER Gábor wrote:\n> To notice a changed todo file the sequencer stores the file's stat\n> data in its 'struct todo_list' instance, and compares it with the\n> file's current stat data after 'reword', 'squash' and 'exec'\n> instructions. If the two stat data doesn't match, it re-reads the\n> todo file.\n> \n> Sounds simple, but there are some subtleties going on here:\n> \n> - The 'struct todo_list' holds the stat data from the time when the\n> todo file was last read.\n> \n> - This stat data in 'struct todo_list' is not updated when the\n> sequencer itself writes the todo file.\n> \n> - Before executing each instruction during an interactive rebase,\n> the sequencer always updates the todo file by removing the\n> just-about-to-be-executed instruction. This changes the file's\n> size and inode [1].\n> \n> Consequently, when the sequencer looks at the stat data after a\n> 'reword', 'squash' or 'exec' instruction, it most likely finds that\n> they differ, even when the user didn't modify the todo list at all!\n> This is not an issue in practice, it just wastes a few cycles on\n> re-reading the todo list that matches what the sequencer already has\n> in memory anyway.\n> \n> However, an unsuspecting Git developer might try to \"fix\" it by simply\n> updating the stat data each time the sequencer writes the todo list\n> for an interactive rebase. On first sight it looks quite sensible and\n> straightforward, but we have to be very careful when doing that,\n> because potential racy problems lie that way.\n> \n> It is possible to overwrite the todo list file without modifying\n> either its inode or size, and if such an overwrite were to happen in\n> the same second when the file was last read (our stat data has one\n> second granularity by default), then the actual stat data on the file\n> system would match the stat data that the sequencer has in memory.\n> Consequently, such a modification to the todo list file would go\n> unnoticed.\n\n> [1] The todo file is written using the lockfile API, i.e. first to the\n> lockfile, which is then moved to place, so the new file can't\n> possibly have the same inode as the file it replaces. Note,\n> however, that the file system might reuse inodes, so it is\n> possible that the new todo file ends up with the same inode as is\n> recorded in the 'struct todo_list' from the last time the file was\n> read.\n\nUnfortunately, we already do have an issue here when the sequencer can\noverlook a modified todo list, but triggering it needs the lucky\n\"alignment\" of inodes as well. I can trigger it fairly reliably with\nthe test below, but forcing the inode match makes it kind of gross and\nLinux-only.\n\n https://travis-ci.org/szeder/git/jobs/616460522#L1470\n\n\n --- >8 ---\n\ndiff --git a/t/t9999-rebase-racy-todo-reread.sh b/t/t9999-rebase-racy-todo-reread.sh\nnew file mode 100755\nindex 0000000000..437ebd55e0\n--- /dev/null\n+++ b/t/t9999-rebase-racy-todo-reread.sh\n@@ -0,0 +1,87 @@\n+#!/bin/sh\n+\n+test_description='racy edit todo reread problem'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit first_ &&\n+\ttest_commit second &&\n+\ttest_commit third_ &&\n+\ttest_commit fourth &&\n+\ttest_commit fifth_ &&\n+\ttest_commit sixth_ &&\n+\n+\twrite_script sequence-editor <<-\\EOS &&\n+\t\ttodo=.git/rebase-merge/git-rebase-todo &&\n+\t\tcat >\"$todo\" <<-EOF\n+\t\t\treword $(git rev-parse second^0) second\n+\t\t\treword $(git rev-parse third_^0) third_\n+\t\t\treword $(git rev-parse fourth^0) fourth\n+\t\tEOF\n+\tEOS\n+\n+\twrite_script commit-editor <<-\\EOS &&\n+\t\tread first_line <\"$1\" &&\n+\t\techo \"$first_line - edited\" >\"$1\" &&\n+\n+\t\ttodo=.git/rebase-merge/git-rebase-todo &&\n+\n+\t\tif test \"$first_line\" = second\n+\t\tthen\n+\t\t\tstat --format=%i \"$todo\" >expected-ino\n+\t\telif test \"$first_line\" = third_\n+\t\tthen\n+\t\t\tino=$(cat expected-ino) &&\n+\t\t\tfile=$(find . -inum $ino) &&\n+\t\t\tif test -n \"$file\"\n+\t\t\tthen\n+\t\t\t\techo &&\n+\t\t\t\techo \"Trying to free inode $ino by moving \\\"$file\\\" out of the way\" &&\n+\t\t\t\tcp -av \"$file\" \"$file\".tmp &&\n+\t\t\t\trm -fv \"$file\"\n+\t\t\tfi &&\n+\n+\t\t\tcat >\"$todo\".tmp <<-EOF &&\n+\t\t\treword $(git rev-parse fifth_^0) fifth_\n+\t\t\treword $(git rev-parse sixth_^0) sixth_\n+\t\t\tEOF\n+\t\t\tmv -v \"$todo\".tmp \"$todo\" &&\n+\n+\t\t\tif test \"$ino\" -eq $(stat --format=%i \"$todo\")\n+\t\t\tthen\n+\t\t\t\techo \"Yay! The todo list did get inode $ino, just what the sequencer is expecting!\"\n+\t\t\tfi &&\n+\n+\t\t\tif test -n \"$file\"\n+\t\t\tthen\n+\t\t\t\tmv -v \"$file\".tmp \"$file\"\n+\t\t\tfi\n+\t\tfi\n+\tEOS\n+\n+\tcat >expect <<-\\EOF\n+\tsixth_ - edited\n+\tfifth_ - edited\n+\tthird_ - edited\n+\tsecond - edited\n+\tfirst_\n+\tEOF\n+'\n+\n+for trial in 0 1 2 3 4\n+do\n+\ttest_expect_success \"demonstrate racy todo re-read problem #$trial\" '\n+\t\tgit reset --hard fourth &&\n+\t\t>expected-ino && # placeholder\n+\n+\t\tGIT_SEQUENCE_EDITOR=./sequence-editor \\\n+\t\tGIT_EDITOR=./commit-editor \\\n+\t\tgit rebase -i HEAD^^^ &&\n+\n+\t\tgit log --format=%s >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+test_done\n"},{"id":"386980","messageId":"8881749f-195e-7f24-6f38-3b0a53ef8e1e@gmail.com","threadId":"52319","inReplyTo":"xmqqo8x0zros.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-25T10:47:27Z","receivedAt":"2019-11-25T10:47:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 25/11/2019 01:10, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>>> -\t\t} else if (check_todo && !res) {\n>>>> +\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n>>>\n>>> It is a bit sad that setting of check_todo is not something a single\n>>> helper function can decide, so that this is_rebase_i(opts) can be\n>>> taken into account when that helper function (the logical place\n>>> would be do_pick_commit()) decides to set (or not set) check_todo.\n>>>\n>>> Unfortunately, that is not sufficient, I suspect. Why did a47ba3c7\n>>> (\"rebase -i: check for updated todo after squash and reword\",\n>>> 2019-08-19) decide to flip check_todo on when running TODO_EXEC?\n>>\n>> I'm not sure what you mean by this\n>>\n>> This is what I had before I saw Gábor's patch (the tests are pretty\n>> similar but I think we should check that the messages of all the picks\n>> are actually edited with --edit - that does not seem to be checked by\n>> the current tests)...\n> \n> I first thought that unsetting *check_todo in do_pick_commit(), when\n> !is_rebase_i(), was a clean solution. But sadly it is *not* a godo\n> equivalent to Gábor's patch, because check_todo can be set to true\n> without looking at is_rebase_i() in pick_commits() [*1*]. \n\nThat only happens if we're running a rebase as exec commands are not \nused by cherry-pick and revert so the unconditional setting that remains \nafter my suggested fix should be safe. Prior to a47ba3c7 (\"rebase -i: \ncheck for updated todo after squash and reword\", 2019-08-19) we always \nchecked for an updated todo list after processing an exec command \nwithout checking is_rebase_i()\n\nBest Wishes\n\nPhillip\n\n> To ignore\n> that setting where the variable's value is used, the hunk we see\n> above in the beginning of this message is necessary.\n> \n> That was what I meant. I think the \"This is what I had before\"\n> patch matches my \"I first thought\" version, so we were on the same\n> page and both wrong ;-)\n> \n> Thanks.\n> \n> \n> [Footnote]\n> \n> *1* I still do not know why a47ba3c7 (\"rebase -i: check for updated\n> todo after squash and reword\", 2019-08-19) sets check_todo to true\n> without looking at is_rebase_i(). If this unconditonal setting in\n> TODO_EXEC did not exist, I think your \"This is what I had before\"\n> patch would have been equivalent to Gábor's patch.\n\n"},{"id":"386984","messageId":"20191125131833.GD23183@szeder.dev","threadId":"52319","inReplyTo":"20191124211021.GB23183@szeder.dev","subject":"Re: [PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-25T13:18:33Z","receivedAt":"2019-11-25T13:18:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Nov 24, 2019 at 10:10:21PM +0100, SZEDER Gábor wrote:\n> To notice a changed todo file the sequencer stores the file's stat\n> data in its 'struct todo_list' instance, and compares it with the\n> file's current stat data after 'reword', 'squash' and 'exec'\n> instructions. If the two stat data doesn't match, it re-reads the\n> todo file.\n> \n> Sounds simple, but there are some subtleties going on here:\n> \n> - The 'struct todo_list' holds the stat data from the time when the\n> todo file was last read.\n> \n> - This stat data in 'struct todo_list' is not updated when the\n> sequencer itself writes the todo file.\n> \n> - Before executing each instruction during an interactive rebase,\n> the sequencer always updates the todo file by removing the\n> just-about-to-be-executed instruction. This changes the file's\n> size and inode [1].\n> \n> Consequently, when the sequencer looks at the stat data after a\n> 'reword', 'squash' or 'exec' instruction, it most likely finds that\n> they differ, even when the user didn't modify the todo list at all!\n> This is not an issue in practice, it just wastes a few cycles on\n> re-reading the todo list that matches what the sequencer already has\n> in memory anyway.\n\nIt can be much more than just a few cycles, because the total number\nof parsed instructions from all the todo file re-reads can go\nquadratic with the number of rebased commits.\n\nThe simple test below runs 'git rebase -i -x' on 1000 commits, which\ntakes over 14seconds to run. If it doesn't re-read the todo file at\nall (I simply deleted the whole condition block checking the stat data\nand re-reading) it runs for only ~2.5secs.\n\nJust another angle to consider...\n\n\n --- >8 ---\n\ntest_expect_success 'test' '\n\tnum_commits=1000 &&\n\ttest_commit_bulk --filename=file $num_commits &&\n\n\t/usr/bin/time -f \"Elapsed time: %e\" \\\n\t\tgit rebase -i --root -x true 2>out &&\n\n\tgrep \"Executing: true\" out >actual &&\n\ttest_line_count = $num_commits actual &&\n\n\t# show the elapsed time\n\ttail -n2 out\n'\n\n --- >8 ---\n\n"},{"id":"386997","messageId":"5a43a071-a3c2-770e-bca4-3e73aff96e48@gmail.com","threadId":"52319","inReplyTo":"20191125131833.GD23183@szeder.dev","subject":"Re: [PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-25T14:43:07Z","receivedAt":"2019-11-25T14:43:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/11/2019 13:18, SZEDER Gábor wrote:\n> On Sun, Nov 24, 2019 at 10:10:21PM +0100, SZEDER Gábor wrote:\n>> To notice a changed todo file the sequencer stores the file's stat\n>> data in its 'struct todo_list' instance, and compares it with the\n>> file's current stat data after 'reword', 'squash' and 'exec'\n>> instructions. If the two stat data doesn't match, it re-reads the\n>> todo file.\n>>\n>> Sounds simple, but there are some subtleties going on here:\n>>\n>> - The 'struct todo_list' holds the stat data from the time when the\n>> todo file was last read.\n>>\n>> - This stat data in 'struct todo_list' is not updated when the\n>> sequencer itself writes the todo file.\n>>\n>> - Before executing each instruction during an interactive rebase,\n>> the sequencer always updates the todo file by removing the\n>> just-about-to-be-executed instruction. This changes the file's\n>> size and inode [1].\n>>\n>> Consequently, when the sequencer looks at the stat data after a\n>> 'reword', 'squash' or 'exec' instruction, it most likely finds that\n>> they differ, even when the user didn't modify the todo list at all!\n>> This is not an issue in practice, it just wastes a few cycles on\n>> re-reading the todo list that matches what the sequencer already has\n>> in memory anyway.\n> \n> It can be much more than just a few cycles, because the total number\n> of parsed instructions from all the todo file re-reads can go\n> quadratic with the number of rebased commits.\n> \n> The simple test below runs 'git rebase -i -x' on 1000 commits, which\n> takes over 14seconds to run. If it doesn't re-read the todo file at\n> all (I simply deleted the whole condition block checking the stat data\n> and re-reading) it runs for only ~2.5secs.\n> \n> Just another angle to consider...\n\nI know dscho was keen to avoid re-parsing the list all the time [1] \npresumably because of the quadratic behavior. (He also assumed most \npeople were using ns stat times [2] but that appears not to be the case) \nCould we just compare the text of the todo list on disk to whats in \ntodo->buf.buf (with an appropriate offset)? That would avoid parsing the \ntext and looking up all the commits with get_oid()\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://public-inbox.org/git/alpine.DEB.2.20.1703021617510.3767@virtualbox/\n[2] \nhttps://public-inbox.org/git/alpine.DEB.2.20.1704131526500.2135@virtualbox/\n\n> \n> --- >8 ---\n> \n> test_expect_success 'test' '\n> \tnum_commits=1000 &&\n> \ttest_commit_bulk --filename=file $num_commits &&\n> \n> \t/usr/bin/time -f \"Elapsed time: %e\" \\\n> \t\tgit rebase -i --root -x true 2>out &&\n> \n> \tgrep \"Executing: true\" out >actual &&\n> \ttest_line_count = $num_commits actual &&\n> \n> \t# show the elapsed time\n> \ttail -n2 out\n> '\n> \n> --- >8 ---\n> \n"},{"id":"386999","messageId":"20191125151517.GE23183@szeder.dev","threadId":"52319","inReplyTo":"5a43a071-a3c2-770e-bca4-3e73aff96e48@gmail.com","subject":"Re: [PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-25T15:15:17Z","receivedAt":"2019-11-25T15:15:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 25, 2019 at 02:43:07PM +0000, Phillip Wood wrote:\n> On 25/11/2019 13:18, SZEDER Gábor wrote:\n> >On Sun, Nov 24, 2019 at 10:10:21PM +0100, SZEDER Gábor wrote:\n> >>To notice a changed todo file the sequencer stores the file's stat\n> >>data in its 'struct todo_list' instance, and compares it with the\n> >>file's current stat data after 'reword', 'squash' and 'exec'\n> >>instructions. If the two stat data doesn't match, it re-reads the\n> >>todo file.\n> >>\n> >>Sounds simple, but there are some subtleties going on here:\n> >>\n> >> - The 'struct todo_list' holds the stat data from the time when the\n> >> todo file was last read.\n> >>\n> >> - This stat data in 'struct todo_list' is not updated when the\n> >> sequencer itself writes the todo file.\n> >>\n> >> - Before executing each instruction during an interactive rebase,\n> >> the sequencer always updates the todo file by removing the\n> >> just-about-to-be-executed instruction. This changes the file's\n> >> size and inode [1].\n> >>\n> >>Consequently, when the sequencer looks at the stat data after a\n> >>'reword', 'squash' or 'exec' instruction, it most likely finds that\n> >>they differ, even when the user didn't modify the todo list at all!\n> >>This is not an issue in practice, it just wastes a few cycles on\n> >>re-reading the todo list that matches what the sequencer already has\n> >>in memory anyway.\n> >\n> >It can be much more than just a few cycles, because the total number\n> >of parsed instructions from all the todo file re-reads can go\n> >quadratic with the number of rebased commits.\n> >\n> >The simple test below runs 'git rebase -i -x' on 1000 commits, which\n> >takes over 14seconds to run. If it doesn't re-read the todo file at\n> >all (I simply deleted the whole condition block checking the stat data\n> >and re-reading) it runs for only ~2.5secs.\n> >\n> >Just another angle to consider...\n> \n> I know dscho was keen to avoid re-parsing the list all the time [1]\n> presumably because of the quadratic behavior. (He also assumed most people\n> were using ns stat times [2] but that appears not to be the case)\n\nNanosecond file timestamp comparisons are only enabled by the USE_NSEC\nmacro, which is only defined if the USE_NSEC Makefile knob is enabled,\nbut that is not enabled by default.\n\nThen there is the related NO_NSEC Makefile knob:\n\n # Define NO_NSEC if your \"struct stat\" does not have \"st_ctim.tv_nsec\"\n # available. This automatically turns USE_NSEC off.\n\nAs Dscho mentioned in [2], we do disable nanosecond file timestamp\ncomparisons in 'config.mak.uname' on a handful of platforms by setting\nNO_NSEC. This, however, does not mean that nanosec timestamps are\nenabled on other platforms by default.\n\n> Could we\n> just compare the text of the todo list on disk to whats in todo->buf.buf\n> (with an appropriate offset)? That would avoid parsing the text and looking\n> up all the commits with get_oid()\n\nComparing the contents without parsing is still quadratic in the size\nof the todo list, though I suppose with a much lower constant factor\nthan actually parsing it.\n\n> [1]\n> https://public-inbox.org/git/alpine.DEB.2.20.1703021617510.3767@virtualbox/\n> [2]\n> https://public-inbox.org/git/alpine.DEB.2.20.1704131526500.2135@virtualbox/\n> \n> >\n> > --- >8 ---\n> >\n> >test_expect_success 'test' '\n> >\tnum_commits=1000 &&\n> >\ttest_commit_bulk --filename=file $num_commits &&\n> >\n> >\t/usr/bin/time -f \"Elapsed time: %e\" \\\n> >\t\tgit rebase -i --root -x true 2>out &&\n> >\n> >\tgrep \"Executing: true\" out >actual &&\n> >\ttest_line_count = $num_commits actual &&\n> >\n> >\t# show the elapsed time\n> >\ttail -n2 out\n> >'\n> >\n> > --- >8 ---\n> >\n"},{"id":"387008","messageId":"1d94202c-4ab9-8437-0d73-80820ecb6789@gmail.com","threadId":"52319","inReplyTo":"20191125151517.GE23183@szeder.dev","subject":"Re: [PATCH] t3429: try to protect against a potential racy todo file problem","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-25T16:40:46Z","receivedAt":"2019-11-25T16:40:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/11/2019 15:15, SZEDER Gábor wrote:\n> On Mon, Nov 25, 2019 at 02:43:07PM +0000, Phillip Wood wrote:\n>> On 25/11/2019 13:18, SZEDER Gábor wrote:\n>>> On Sun, Nov 24, 2019 at 10:10:21PM +0100, SZEDER Gábor wrote:\n>>>> To notice a changed todo file the sequencer stores the file's stat\n>>>> data in its 'struct todo_list' instance, and compares it with the\n>>>> file's current stat data after 'reword', 'squash' and 'exec'\n>>>> instructions. If the two stat data doesn't match, it re-reads the\n>>>> todo file.\n>>>>\n>>>> Sounds simple, but there are some subtleties going on here:\n>>>>\n>>>> - The 'struct todo_list' holds the stat data from the time when the\n>>>> todo file was last read.\n>>>>\n>>>> - This stat data in 'struct todo_list' is not updated when the\n>>>> sequencer itself writes the todo file.\n>>>>\n>>>> - Before executing each instruction during an interactive rebase,\n>>>> the sequencer always updates the todo file by removing the\n>>>> just-about-to-be-executed instruction. This changes the file's\n>>>> size and inode [1].\n>>>>\n>>>> Consequently, when the sequencer looks at the stat data after a\n>>>> 'reword', 'squash' or 'exec' instruction, it most likely finds that\n>>>> they differ, even when the user didn't modify the todo list at all!\n>>>> This is not an issue in practice, it just wastes a few cycles on\n>>>> re-reading the todo list that matches what the sequencer already has\n>>>> in memory anyway.\n>>>\n>>> It can be much more than just a few cycles, because the total number\n>>> of parsed instructions from all the todo file re-reads can go\n>>> quadratic with the number of rebased commits.\n>>>\n>>> The simple test below runs 'git rebase -i -x' on 1000 commits, which\n>>> takes over 14seconds to run. If it doesn't re-read the todo file at\n>>> all (I simply deleted the whole condition block checking the stat data\n>>> and re-reading) it runs for only ~2.5secs.\n>>>\n>>> Just another angle to consider...\n>>\n>> I know dscho was keen to avoid re-parsing the list all the time [1]\n>> presumably because of the quadratic behavior. (He also assumed most people\n>> were using ns stat times [2] but that appears not to be the case)\n> \n> Nanosecond file timestamp comparisons are only enabled by the USE_NSEC\n> macro, which is only defined if the USE_NSEC Makefile knob is enabled,\n> but that is not enabled by default.\n> \n> Then there is the related NO_NSEC Makefile knob:\n> \n> # Define NO_NSEC if your \"struct stat\" does not have \"st_ctim.tv_nsec\"\n> # available. This automatically turns USE_NSEC off.\n> \n> As Dscho mentioned in [2], we do disable nanosecond file timestamp\n> comparisons in 'config.mak.uname' on a handful of platforms by setting\n> NO_NSEC. This, however, does not mean that nanosec timestamps are\n> enabled on other platforms by default.\n> \n>> Could we\n>> just compare the text of the todo list on disk to whats in todo->buf.buf\n>> (with an appropriate offset)? That would avoid parsing the text and looking\n>> up all the commits with get_oid()\n> \n> Comparing the contents without parsing is still quadratic in the size\n> of the todo list, though I suppose with a much lower constant factor\n> than actually parsing it.\n\nThe patch below (assuming thunderbird doesn't mangle it) reduces the \ntime to run your bulk commit test from 30s to 7s, if I delete the \ncondition block which checks the stat data it takes 4.7s on my (somewhat \nancient) laptop. So there is a cost to the string comparison approach \nbut it's much less that the full todo list parsing.\n\nBest Wishes\n\nPhillip\n\n--- >8 ---\ndiff --git a/sequencer.c b/sequencer.c\nindex 8952cfa89b..a3efdae0a5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3909,12 +3909,17 @@ static int pick_commits(struct repository *r,\n arg, item->arg_len,\n opts, res, 0);\n } else if (check_todo && !res) {\n- struct stat st;\n-\n- if (stat(get_todo_path(opts), &st)) {\n- res = error_errno(_(\"could not stat '%s'\"),\n+ int offset;\n+ struct strbuf buf = STRBUF_INIT;\n+ if (strbuf_read_file(&buf, get_todo_path(opts), \n8096) < 0)\n+ res = error_errno(_(\"could not read '%s'\"),\n get_todo_path(opts));\n- } else if (match_stat_data(&todo_list->stat, &st)) {\n+\n+ offset = get_item_line_offset(todo_list,\n+ todo_list->current \n+ 1);\n+ if (buf.len != todo_list->buf.len - offset ||\n+ memcmp(buf.buf, todo_list->buf.buf + offset, \nbuf.len)) {\n+ fputs(\"re-reading todo list\\n\", stderr);\n /* Reread the todo file if it has \nchanged. */\n todo_list_release(todo_list);\n if (read_populate_todo(r, todo_list, opts))\n\n"}]}