{"thread":{"id":"52828","subject":"[PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","startedAt":"2020-02-17T17:15:56Z","lastAt":"2020-02-19T21:39:34Z","messageCount":20,"participants":["Elijah Newren via GitGitGadget","Elijah Newren","Johannes Schindelin","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"391925","messageId":"pull.712.git.git.1581959751454.gitgitgadget@gmail.com","threadId":"52828","inReplyTo":null,"subject":"[PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T17:15:51Z","receivedAt":"2020-02-17T17:15:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nA user discovered a case where they had a stack of 20 simple commits to\nrebase, and the rebase would succeed in picking the first commit and\nthen error out with a pair of \"Could not execute the todo command\" and\n\"Your local changes to the following files would be overwritten by\nmerge\" messages.\n\nTheir steps actually made use of the -i flag, but I switched it over to\n-m to make it simpler to trigger the bug.  With that flag, it bisects\nback to commit 68aa495b590d (rebase: implement --merge via the\ninteractive machinery, 2018-12-11), but that's misleading.  If you\nchange the -m flag to --keep-empty, then the problem persists and will\nbisect back to 356ee4659bb5 (sequencer: try to commit without forking\n'git commit', 2017-11-24)\n\nAfter playing with the testcase for a bit, I discovered that added\n--exec \"sleep 1\" to the command line makes the rebase succeed, making me\nsuspect there is some kind of discard and reloading of caches that lead\nus to believe that something is stat dirty, but I didn't succeed in\ndigging any further than that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    t3424: new rebase testcase documenting a stat-dirty-like failure\n    \n    A user discovered a case where they had a stack of 20 simple commits to\n    rebase, and the rebase would succeed in picking the first commit and\n    then error out with a pair of \"Could not execute the todo command\" and\n    \"Your local changes to the following files would be overwritten by\n    merge\" messages.\n    \n    Their steps actually made use of the -i flag, but I switched it over to\n    -m to make it simpler to trigger the bug. With that flag, it bisects\n    back to commit 68aa495b590d (rebase: implement --merge via the\n    interactive machinery, 2018-12-11), but that's misleading. If you change\n    the -m flag to --keep-empty, then the problem persists and will bisect\n    back to 356ee4659bb5 (sequencer: try to commit without forking 'git\n    commit', 2017-11-24)\n    \n    After playing with the testcase for a bit, I discovered that added\n    --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n    suspect there is some kind of discard and reloading of caches that lead\n    us to believe that something is stat dirty, but I didn't succeed in\n    digging any further than that.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v1\nPull-Request: https://github.com/git/git/pull/712\n\n t/t3424-rebase-across-mode-change.sh | 52 ++++++++++++++++++++++++++++\n 1 file changed, 52 insertions(+)\n create mode 100755 t/t3424-rebase-across-mode-change.sh\n\ndiff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\nnew file mode 100755\nindex 00000000000..4d2eb1dd7c6\n--- /dev/null\n+++ b/t/t3424-rebase-across-mode-change.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+test_description='git rebase across mode change'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\trm -rf ../stupid &&\n+\tgit init ../stupid &&\n+\tcd ../stupid &&\n+\tmkdir DS &&\n+\t>DS/whatever &&\n+\tgit add DS &&\n+\tgit commit -m base &&\n+\n+\tgit branch side1 &&\n+\tgit branch side2 &&\n+\n+\tgit checkout side1 &&\n+\tgit rm -rf DS &&\n+\tln -s unrelated DS &&\n+\tgit add DS &&\n+\tgit commit -m side1 &&\n+\n+\tgit checkout side2 &&\n+\t>unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m commit1 &&\n+\n+\techo >>unrelated &&\n+\tgit commit -am commit2\n+'\n+\n+test_expect_success 'rebase changes with the apply backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b apply-backend side2 &&\n+\tgit rebase side1\n+'\n+\n+test_expect_failure 'rebase changes with the merge backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-backend side2 &&\n+\tgit rebase -m side1\n+'\n+\n+test_expect_success 'rebase changes with the merge backend with a delay' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-delay-backend side2 &&\n+\tgit rebase -m --exec \"sleep 1\" side1\n+'\n+\n+test_done\n\nbase-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\n-- \ngitgitgadget\n"},{"id":"391949","messageId":"CABPp-BEtnmzDp0E4=0y9eEMKQ89FcrsK9h-1Mqcd2FDV_EBohw@mail.gmail.com","threadId":"52828","inReplyTo":"pull.712.git.git.1581959751454.gitgitgadget@gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-17T19:12:50Z","receivedAt":"2020-02-17T19:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"I forgot to add some cc's in GitGitGadget before submitting; adding now...\n\nOn Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Elijah Newren <newren@gmail.com>\n>\n> A user discovered a case where they had a stack of 20 simple commits to\n> rebase, and the rebase would succeed in picking the first commit and\n> then error out with a pair of \"Could not execute the todo command\" and\n> \"Your local changes to the following files would be overwritten by\n> merge\" messages.\n>\n> Their steps actually made use of the -i flag, but I switched it over to\n> -m to make it simpler to trigger the bug.  With that flag, it bisects\n> back to commit 68aa495b590d (rebase: implement --merge via the\n> interactive machinery, 2018-12-11), but that's misleading.  If you\n> change the -m flag to --keep-empty, then the problem persists and will\n> bisect back to 356ee4659bb5 (sequencer: try to commit without forking\n> 'git commit', 2017-11-24)\n>\n> After playing with the testcase for a bit, I discovered that added\n> --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n> suspect there is some kind of discard and reloading of caches that lead\n> us to believe that something is stat dirty, but I didn't succeed in\n> digging any further than that.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>     t3424: new rebase testcase documenting a stat-dirty-like failure\n>\n>     A user discovered a case where they had a stack of 20 simple commits to\n>     rebase, and the rebase would succeed in picking the first commit and\n>     then error out with a pair of \"Could not execute the todo command\" and\n>     \"Your local changes to the following files would be overwritten by\n>     merge\" messages.\n>\n>     Their steps actually made use of the -i flag, but I switched it over to\n>     -m to make it simpler to trigger the bug. With that flag, it bisects\n>     back to commit 68aa495b590d (rebase: implement --merge via the\n>     interactive machinery, 2018-12-11), but that's misleading. If you change\n>     the -m flag to --keep-empty, then the problem persists and will bisect\n>     back to 356ee4659bb5 (sequencer: try to commit without forking 'git\n>     commit', 2017-11-24)\n>\n>     After playing with the testcase for a bit, I discovered that added\n>     --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n>     suspect there is some kind of discard and reloading of caches that lead\n>     us to believe that something is stat dirty, but I didn't succeed in\n>     digging any further than that.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v1\n> Pull-Request: https://github.com/git/git/pull/712\n>\n>  t/t3424-rebase-across-mode-change.sh | 52 ++++++++++++++++++++++++++++\n>  1 file changed, 52 insertions(+)\n>  create mode 100755 t/t3424-rebase-across-mode-change.sh\n>\n> diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\n> new file mode 100755\n> index 00000000000..4d2eb1dd7c6\n> --- /dev/null\n> +++ b/t/t3424-rebase-across-mode-change.sh\n> @@ -0,0 +1,52 @@\n> +#!/bin/sh\n> +\n> +test_description='git rebase across mode change'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +       rm -rf ../stupid &&\n> +       git init ../stupid &&\n> +       cd ../stupid &&\n> +       mkdir DS &&\n> +       >DS/whatever &&\n> +       git add DS &&\n> +       git commit -m base &&\n> +\n> +       git branch side1 &&\n> +       git branch side2 &&\n> +\n> +       git checkout side1 &&\n> +       git rm -rf DS &&\n> +       ln -s unrelated DS &&\n> +       git add DS &&\n> +       git commit -m side1 &&\n> +\n> +       git checkout side2 &&\n> +       >unrelated &&\n> +       git add unrelated &&\n> +       git commit -m commit1 &&\n> +\n> +       echo >>unrelated &&\n> +       git commit -am commit2\n> +'\n> +\n> +test_expect_success 'rebase changes with the apply backend' '\n> +       test_when_finished \"git rebase --abort || true\" &&\n> +       git checkout -b apply-backend side2 &&\n> +       git rebase side1\n> +'\n> +\n> +test_expect_failure 'rebase changes with the merge backend' '\n> +       test_when_finished \"git rebase --abort || true\" &&\n> +       git checkout -b merge-backend side2 &&\n> +       git rebase -m side1\n> +'\n> +\n> +test_expect_success 'rebase changes with the merge backend with a delay' '\n> +       test_when_finished \"git rebase --abort || true\" &&\n> +       git checkout -b merge-delay-backend side2 &&\n> +       git rebase -m --exec \"sleep 1\" side1\n> +'\n> +\n> +test_done\n>\n> base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\n> --\n> gitgitgadget\n"},{"id":"391985","messageId":"nycvar.QRO.7.76.6.2002181501240.46@tvgsbejvaqbjf.bet","threadId":"52828","inReplyTo":"pull.712.git.git.1581959751454.gitgitgadget@gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-18T14:03:00Z","receivedAt":"2020-02-18T14:03:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nI am awfully short on time these days, so just a very quick observation:\n\nOn Mon, 17 Feb 2020, Elijah Newren via GitGitGadget wrote:\n\n> diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\n> new file mode 100755\n> index 00000000000..4d2eb1dd7c6\n> --- /dev/null\n> +++ b/t/t3424-rebase-across-mode-change.sh\n> @@ -0,0 +1,52 @@\n> +#!/bin/sh\n> +\n> +test_description='git rebase across mode change'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\trm -rf ../stupid &&\n> +\tgit init ../stupid &&\n> +\tcd ../stupid &&\n> +\tmkdir DS &&\n> +\t>DS/whatever &&\n> +\tgit add DS &&\n> +\tgit commit -m base &&\n> +\n> +\tgit branch side1 &&\n> +\tgit branch side2 &&\n> +\n> +\tgit checkout side1 &&\n> +\tgit rm -rf DS &&\n> +\tln -s unrelated DS &&\n\nThis requires symbolic links. Therefore it won't work on Windows, and will\nat least need the `SYMLINKS` prereq. Maybe there is a way, though, to\nchange the test so it does not require a symbolic link here?\n\nThanks,\nDscho\n\n> +\tgit add DS &&\n> +\tgit commit -m side1 &&\n> +\n> +\tgit checkout side2 &&\n> +\t>unrelated &&\n> +\tgit add unrelated &&\n> +\tgit commit -m commit1 &&\n> +\n> +\techo >>unrelated &&\n> +\tgit commit -am commit2\n> +'\n> +\n> +test_expect_success 'rebase changes with the apply backend' '\n> +\ttest_when_finished \"git rebase --abort || true\" &&\n> +\tgit checkout -b apply-backend side2 &&\n> +\tgit rebase side1\n> +'\n> +\n> +test_expect_failure 'rebase changes with the merge backend' '\n> +\ttest_when_finished \"git rebase --abort || true\" &&\n> +\tgit checkout -b merge-backend side2 &&\n> +\tgit rebase -m side1\n> +'\n> +\n> +test_expect_success 'rebase changes with the merge backend with a delay' '\n> +\ttest_when_finished \"git rebase --abort || true\" &&\n> +\tgit checkout -b merge-delay-backend side2 &&\n> +\tgit rebase -m --exec \"sleep 1\" side1\n> +'\n> +\n> +test_done\n>\n> base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\n> --\n> gitgitgadget\n>\n"},{"id":"391988","messageId":"ed8dc52c-db50-f6fa-9583-8ad4af23d327@gmail.com","threadId":"52828","inReplyTo":"CABPp-BEtnmzDp0E4=0y9eEMKQ89FcrsK9h-1Mqcd2FDV_EBohw@mail.gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-02-18T15:05:39Z","receivedAt":"2020-02-18T15:05:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 17/02/2020 19:12, Elijah Newren wrote:\n> I forgot to add some cc's in GitGitGadget before submitting; adding now...\n> \n> On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Elijah Newren <newren@gmail.com>\n>>\n>> A user discovered a case where they had a stack of 20 simple commits to\n>> rebase, and the rebase would succeed in picking the first commit and\n>> then error out with a pair of \"Could not execute the todo command\" and\n>> \"Your local changes to the following files would be overwritten by\n>> merge\" messages.\n>>\n>> Their steps actually made use of the -i flag, but I switched it over to\n>> -m to make it simpler to trigger the bug.  With that flag, it bisects\n>> back to commit 68aa495b590d (rebase: implement --merge via the\n>> interactive machinery, 2018-12-11), but that's misleading.  If you\n>> change the -m flag to --keep-empty, then the problem persists and will\n>> bisect back to 356ee4659bb5 (sequencer: try to commit without forking\n>> 'git commit', 2017-11-24)\n>>\n>> After playing with the testcase for a bit, I discovered that added\n>> --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n>> suspect there is some kind of discard and reloading of caches that lead\n>> us to believe that something is stat dirty, but I didn't succeed in\n>> digging any further than that.\n\nIntriguing - it's strange that it errors out picking commit2, not \ncommit1 I'll try and have a look at it. There seem to be some leftover \nbits at the start of the test that want removing see below\n\n>> Signed-off-by: Elijah Newren <newren@gmail.com>\n>> ---\n>>      t3424: new rebase testcase documenting a stat-dirty-like failure\n>>\n>>      A user discovered a case where they had a stack of 20 simple commits to\n>>      rebase, and the rebase would succeed in picking the first commit and\n>>      then error out with a pair of \"Could not execute the todo command\" and\n>>      \"Your local changes to the following files would be overwritten by\n>>      merge\" messages.\n>>\n>>      Their steps actually made use of the -i flag, but I switched it over to\n>>      -m to make it simpler to trigger the bug. With that flag, it bisects\n>>      back to commit 68aa495b590d (rebase: implement --merge via the\n>>      interactive machinery, 2018-12-11), but that's misleading. If you change\n>>      the -m flag to --keep-empty, then the problem persists and will bisect\n>>      back to 356ee4659bb5 (sequencer: try to commit without forking 'git\n>>      commit', 2017-11-24)\n>>\n>>      After playing with the testcase for a bit, I discovered that added\n>>      --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n>>      suspect there is some kind of discard and reloading of caches that lead\n>>      us to believe that something is stat dirty, but I didn't succeed in\n>>      digging any further than that.\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v1\n>> Pull-Request: https://github.com/git/git/pull/712\n>>\n>>   t/t3424-rebase-across-mode-change.sh | 52 ++++++++++++++++++++++++++++\n>>   1 file changed, 52 insertions(+)\n>>   create mode 100755 t/t3424-rebase-across-mode-change.sh\n>>\n>> diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\n>> new file mode 100755\n>> index 00000000000..4d2eb1dd7c6\n>> --- /dev/null\n>> +++ b/t/t3424-rebase-across-mode-change.sh\n>> @@ -0,0 +1,52 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='git rebase across mode change'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup' '\n>> +       rm -rf ../stupid &&\n>> +       git init ../stupid &&\n>> +       cd ../stupid &&\n\nI think these 3 lines must be left over from you trying this out before \nmaking it a test\n\nBest Wishes\n\nPhillip\n\n>> +       mkdir DS &&\n>> +       >DS/whatever &&\n>> +       git add DS &&\n>> +       git commit -m base &&\n>> +\n>> +       git branch side1 &&\n>> +       git branch side2 &&\n>> +\n>> +       git checkout side1 &&\n>> +       git rm -rf DS &&\n>> +       ln -s unrelated DS &&\n>> +       git add DS &&\n>> +       git commit -m side1 &&\n>> +\n>> +       git checkout side2 &&\n>> +       >unrelated &&\n>> +       git add unrelated &&\n>> +       git commit -m commit1 &&\n>> +\n>> +       echo >>unrelated &&\n>> +       git commit -am commit2\n>> +'\n>> +\n>> +test_expect_success 'rebase changes with the apply backend' '\n>> +       test_when_finished \"git rebase --abort || true\" &&\n>> +       git checkout -b apply-backend side2 &&\n>> +       git rebase side1\n>> +'\n>> +\n>> +test_expect_failure 'rebase changes with the merge backend' '\n>> +       test_when_finished \"git rebase --abort || true\" &&\n>> +       git checkout -b merge-backend side2 &&\n>> +       git rebase -m side1\n>> +'\n>> +\n>> +test_expect_success 'rebase changes with the merge backend with a delay' '\n>> +       test_when_finished \"git rebase --abort || true\" &&\n>> +       git checkout -b merge-delay-backend side2 &&\n>> +       git rebase -m --exec \"sleep 1\" side1\n>> +'\n>> +\n>> +test_done\n>>\n>> base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\n>> --\n>> gitgitgadget\n"},{"id":"391989","messageId":"CABPp-BFz63D9Mm69T9tOQpmQdJuhKbA3_U_Su4dV2XSkZxhTfg@mail.gmail.com","threadId":"52828","inReplyTo":"nycvar.QRO.7.76.6.2002181501240.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-18T15:55:59Z","receivedAt":"2020-02-18T15:56:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 18, 2020 at 6:03 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Elijah,\n>\n> I am awfully short on time these days, so just a very quick observation:\n>\n> On Mon, 17 Feb 2020, Elijah Newren via GitGitGadget wrote:\n>\n> > diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\n> > new file mode 100755\n> > index 00000000000..4d2eb1dd7c6\n> > --- /dev/null\n> > +++ b/t/t3424-rebase-across-mode-change.sh\n> > @@ -0,0 +1,52 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='git rebase across mode change'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup' '\n> > +     rm -rf ../stupid &&\n> > +     git init ../stupid &&\n> > +     cd ../stupid &&\n> > +     mkdir DS &&\n> > +     >DS/whatever &&\n> > +     git add DS &&\n> > +     git commit -m base &&\n> > +\n> > +     git branch side1 &&\n> > +     git branch side2 &&\n> > +\n> > +     git checkout side1 &&\n> > +     git rm -rf DS &&\n> > +     ln -s unrelated DS &&\n>\n> This requires symbolic links. Therefore it won't work on Windows, and will\n> at least need the `SYMLINKS` prereq. Maybe there is a way, though, to\n> change the test so it does not require a symbolic link here?\n\nAh, yes, I will fix it up.  Thanks.\n"},{"id":"391990","messageId":"CABPp-BHBv+_HkExM1q6WAZZyMhR=UPNQZDhE8jFSQFNoCtgytg@mail.gmail.com","threadId":"52828","inReplyTo":"ed8dc52c-db50-f6fa-9583-8ad4af23d327@gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-18T15:59:48Z","receivedAt":"2020-02-18T16:00:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 18, 2020 at 7:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> Hi Elijah\n>\n> On 17/02/2020 19:12, Elijah Newren wrote:\n> > I forgot to add some cc's in GitGitGadget before submitting; adding now...\n> >\n> > On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> >>\n> >> From: Elijah Newren <newren@gmail.com>\n> >>\n> >> A user discovered a case where they had a stack of 20 simple commits to\n> >> rebase, and the rebase would succeed in picking the first commit and\n> >> then error out with a pair of \"Could not execute the todo command\" and\n> >> \"Your local changes to the following files would be overwritten by\n> >> merge\" messages.\n> >>\n> >> Their steps actually made use of the -i flag, but I switched it over to\n> >> -m to make it simpler to trigger the bug.  With that flag, it bisects\n> >> back to commit 68aa495b590d (rebase: implement --merge via the\n> >> interactive machinery, 2018-12-11), but that's misleading.  If you\n> >> change the -m flag to --keep-empty, then the problem persists and will\n> >> bisect back to 356ee4659bb5 (sequencer: try to commit without forking\n> >> 'git commit', 2017-11-24)\n> >>\n> >> After playing with the testcase for a bit, I discovered that added\n> >> --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n> >> suspect there is some kind of discard and reloading of caches that lead\n> >> us to believe that something is stat dirty, but I didn't succeed in\n> >> digging any further than that.\n>\n> Intriguing - it's strange that it errors out picking commit2, not\n> commit1 I'll try and have a look at it. There seem to be some leftover\n> bits at the start of the test that want removing see below\n\nI know, right?  It's kinda weird.\n\n> >> +test_expect_success 'setup' '\n> >> +       rm -rf ../stupid &&\n> >> +       git init ../stupid &&\n> >> +       cd ../stupid &&\n>\n> I think these 3 lines must be left over from you trying this out before\n> making it a test\n\nWhoops, yes you are exactly right.  I'll remove them.\n"},{"id":"392017","messageId":"pull.712.v2.git.git.1582059331257.gitgitgadget@gmail.com","threadId":"52828","inReplyTo":"pull.712.git.git.1581959751454.gitgitgadget@gmail.com","subject":"[PATCH v2] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-18T20:55:31Z","receivedAt":"2020-02-18T20:55:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nA user discovered a case where they had a stack of 20 simple commits to\nrebase, and the rebase would succeed in picking the first commit and\nthen error out with a pair of \"Could not execute the todo command\" and\n\"Your local changes to the following files would be overwritten by\nmerge\" messages.\n\nTheir steps actually made use of the -i flag, but I switched it over to\n-m to make it simpler to trigger the bug.  With that flag, it bisects\nback to commit 68aa495b590d (rebase: implement --merge via the\ninteractive machinery, 2018-12-11), but that's misleading.  If you\nchange the -m flag to --keep-empty, then the problem persists and will\nbisect back to 356ee4659bb5 (sequencer: try to commit without forking\n'git commit', 2017-11-24)\n\nAfter playing with the testcase for a bit, I discovered that added\n--exec \"sleep 1\" to the command line makes the rebase succeed, making me\nsuspect there is some kind of discard and reloading of caches that lead\nus to believe that something is stat dirty, but I didn't succeed in\ndigging any further than that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    t3424: new rebase testcase documenting a stat-dirty-like failure\n    \n    Remove extraneous lines and make use of test_ln_s_add to make the test\n    work on Windows\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v2\nPull-Request: https://github.com/git/git/pull/712\n\nRange-diff vs v1:\n\n 1:  ca79ca3ed8a ! 1:  09085292596 t3424: new rebase testcase documenting a stat-dirty-like failure\n     @@ -36,9 +36,6 @@\n      +. ./test-lib.sh\n      +\n      +test_expect_success 'setup' '\n     -+\trm -rf ../stupid &&\n     -+\tgit init ../stupid &&\n     -+\tcd ../stupid &&\n      +\tmkdir DS &&\n      +\t>DS/whatever &&\n      +\tgit add DS &&\n     @@ -49,8 +46,7 @@\n      +\n      +\tgit checkout side1 &&\n      +\tgit rm -rf DS &&\n     -+\tln -s unrelated DS &&\n     -+\tgit add DS &&\n     ++\ttest_ln_s_add unrelated DS &&\n      +\tgit commit -m side1 &&\n      +\n      +\tgit checkout side2 &&\n\n\n t/t3424-rebase-across-mode-change.sh | 48 ++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n create mode 100755 t/t3424-rebase-across-mode-change.sh\n\ndiff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\nnew file mode 100755\nindex 00000000000..f11fc35c3ee\n--- /dev/null\n+++ b/t/t3424-rebase-across-mode-change.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='git rebase across mode change'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir DS &&\n+\t>DS/whatever &&\n+\tgit add DS &&\n+\tgit commit -m base &&\n+\n+\tgit branch side1 &&\n+\tgit branch side2 &&\n+\n+\tgit checkout side1 &&\n+\tgit rm -rf DS &&\n+\ttest_ln_s_add unrelated DS &&\n+\tgit commit -m side1 &&\n+\n+\tgit checkout side2 &&\n+\t>unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m commit1 &&\n+\n+\techo >>unrelated &&\n+\tgit commit -am commit2\n+'\n+\n+test_expect_success 'rebase changes with the apply backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b apply-backend side2 &&\n+\tgit rebase side1\n+'\n+\n+test_expect_failure 'rebase changes with the merge backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-backend side2 &&\n+\tgit rebase -m side1\n+'\n+\n+test_expect_success 'rebase changes with the merge backend with a delay' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-delay-backend side2 &&\n+\tgit rebase -m --exec \"sleep 1\" side1\n+'\n+\n+test_done\n\nbase-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\n-- \ngitgitgadget\n"},{"id":"392025","messageId":"xmqqh7znins2.fsf@gitster-ct.c.googlers.com","threadId":"52828","inReplyTo":"pull.712.v2.git.git.1582059331257.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-18T21:33:17Z","receivedAt":"2020-02-18T21:33:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  t/t3424-rebase-across-mode-change.sh | 48 ++++++++++++++++++++++++++++\n\nt3424 is already taken by en/rebase-backend topic that has been\ncooking for some time by now.  It seems 342? are all taken and the\nnext available may be t3433 perhaps (please double check with 'pu')?\n"},{"id":"392028","messageId":"CABPp-BFAa9zwYD0c-2FWXgswDoduvQ=+GiksLC2Ym2E3WOqFsQ@mail.gmail.com","threadId":"52828","inReplyTo":"xmqqh7znins2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-18T22:01:32Z","receivedAt":"2020-02-18T22:01:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 18, 2020 at 1:33 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> >  t/t3424-rebase-across-mode-change.sh | 48 ++++++++++++++++++++++++++++\n>\n> t3424 is already taken by en/rebase-backend topic that has been\n> cooking for some time by now.  It seems 342? are all taken and the\n> next available may be t3433 perhaps (please double check with 'pu')?\n\nWhoops, sorry about that.  Will fix up.\n"},{"id":"392029","messageId":"pull.712.v3.git.git.1582064105813.gitgitgadget@gmail.com","threadId":"52828","inReplyTo":"pull.712.v2.git.git.1582059331257.gitgitgadget@gmail.com","subject":"[PATCH v3] t3433: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-18T22:15:05Z","receivedAt":"2020-02-18T22:15:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nA user discovered a case where they had a stack of 20 simple commits to\nrebase, and the rebase would succeed in picking the first commit and\nthen error out with a pair of \"Could not execute the todo command\" and\n\"Your local changes to the following files would be overwritten by\nmerge\" messages.\n\nTheir steps actually made use of the -i flag, but I switched it over to\n-m to make it simpler to trigger the bug.  With that flag, it bisects\nback to commit 68aa495b590d (rebase: implement --merge via the\ninteractive machinery, 2018-12-11), but that's misleading.  If you\nchange the -m flag to --keep-empty, then the problem persists and will\nbisect back to 356ee4659bb5 (sequencer: try to commit without forking\n'git commit', 2017-11-24)\n\nAfter playing with the testcase for a bit, I discovered that added\n--exec \"sleep 1\" to the command line makes the rebase succeed, making me\nsuspect there is some kind of discard and reloading of caches that lead\nus to believe that something is stat dirty, but I didn't succeed in\ndigging any further than that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    t3433: new rebase testcase documenting a stat-dirty-like failure\n    \n    Rename to t3433 to avoid having two t3424 tests.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v3\nPull-Request: https://github.com/git/git/pull/712\n\nRange-diff vs v2:\n\n 1:  09085292596 ! 1:  270591cd3be t3424: new rebase testcase documenting a stat-dirty-like failure\n     @@ -1,6 +1,6 @@\n      Author: Elijah Newren <newren@gmail.com>\n      \n     -    t3424: new rebase testcase documenting a stat-dirty-like failure\n     +    t3433: new rebase testcase documenting a stat-dirty-like failure\n      \n          A user discovered a case where they had a stack of 20 simple commits to\n          rebase, and the rebase would succeed in picking the first commit and\n     @@ -24,10 +24,10 @@\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n     - diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh\n     + diff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh\n       new file mode 100755\n       --- /dev/null\n     - +++ b/t/t3424-rebase-across-mode-change.sh\n     + +++ b/t/t3433-rebase-across-mode-change.sh\n      @@\n      +#!/bin/sh\n      +\n\n\n t/t3433-rebase-across-mode-change.sh | 48 ++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n create mode 100755 t/t3433-rebase-across-mode-change.sh\n\ndiff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh\nnew file mode 100755\nindex 00000000000..f11fc35c3ee\n--- /dev/null\n+++ b/t/t3433-rebase-across-mode-change.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='git rebase across mode change'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir DS &&\n+\t>DS/whatever &&\n+\tgit add DS &&\n+\tgit commit -m base &&\n+\n+\tgit branch side1 &&\n+\tgit branch side2 &&\n+\n+\tgit checkout side1 &&\n+\tgit rm -rf DS &&\n+\ttest_ln_s_add unrelated DS &&\n+\tgit commit -m side1 &&\n+\n+\tgit checkout side2 &&\n+\t>unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m commit1 &&\n+\n+\techo >>unrelated &&\n+\tgit commit -am commit2\n+'\n+\n+test_expect_success 'rebase changes with the apply backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b apply-backend side2 &&\n+\tgit rebase side1\n+'\n+\n+test_expect_failure 'rebase changes with the merge backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-backend side2 &&\n+\tgit rebase -m side1\n+'\n+\n+test_expect_success 'rebase changes with the merge backend with a delay' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-delay-backend side2 &&\n+\tgit rebase -m --exec \"sleep 1\" side1\n+'\n+\n+test_done\n\nbase-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\n-- \ngitgitgadget\n"},{"id":"392056","messageId":"deae766d-f552-2e30-fb49-e7e187ee984b@gmail.com","threadId":"52828","inReplyTo":"CABPp-BHBv+_HkExM1q6WAZZyMhR=UPNQZDhE8jFSQFNoCtgytg@mail.gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-02-19T11:01:49Z","receivedAt":"2020-02-19T11:01:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 18/02/2020 15:59, Elijah Newren wrote:\n> On Tue, Feb 18, 2020 at 7:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>\n>> Hi Elijah\n>>\n>> On 17/02/2020 19:12, Elijah Newren wrote:\n>>> I forgot to add some cc's in GitGitGadget before submitting; adding now...\n>>>\n>>> On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget\n>>> <gitgitgadget@gmail.com> wrote:\n>>>>\n>>>> From: Elijah Newren <newren@gmail.com>\n>>>>\n>>>> A user discovered a case where they had a stack of 20 simple commits to\n>>>> rebase, and the rebase would succeed in picking the first commit and\n>>>> then error out with a pair of \"Could not execute the todo command\" and\n>>>> \"Your local changes to the following files would be overwritten by\n>>>> merge\" messages.\n>>>>\n>>>> Their steps actually made use of the -i flag, but I switched it over to\n>>>> -m to make it simpler to trigger the bug.  With that flag, it bisects\n>>>> back to commit 68aa495b590d (rebase: implement --merge via the\n>>>> interactive machinery, 2018-12-11), but that's misleading.  If you\n>>>> change the -m flag to --keep-empty, then the problem persists and will\n>>>> bisect back to 356ee4659bb5 (sequencer: try to commit without forking\n>>>> 'git commit', 2017-11-24)\n>>>>\n>>>> After playing with the testcase for a bit, I discovered that added\n>>>> --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n>>>> suspect there is some kind of discard and reloading of caches that lead\n>>>> us to believe that something is stat dirty, but I didn't succeed in\n>>>> digging any further than that.\n\nI think `--exec true` would be better as it makes it clear that it's not \na timing issue. I've changed do_recursive_merge() to print the mtime and \nmode of \"DS\" before and after the merge which gives\n\nHEAD is now at abd8fe3 side1\nRebasing (1/2) # picking commit1\nDS mtime, mode before merge 1582109854, 120000\nDS mtime, mode after merge 0, 120000\nRebasing (2/2) # picking commit2\nDS mtime, mode before merge 0, 120000\nerror: Your local changes to the following files would be overwritten by \nmerge:\n\tDS\n\nSo it looks like the problem is that when we pick commit1 we don't \nupdate the index entry for DS properly in merge_trees()\n\nBest Wishes\n\nPhillip\n\n>> Intriguing - it's strange that it errors out picking commit2, not\n>> commit1 I'll try and have a look at it. There seem to be some leftover\n>> bits at the start of the test that want removing see below\n> \n> I know, right?  It's kinda weird.\n> \n>>>> +test_expect_success 'setup' '\n>>>> +       rm -rf ../stupid &&\n>>>> +       git init ../stupid &&\n>>>> +       cd ../stupid &&\n>>\n>> I think these 3 lines must be left over from you trying this out before\n>> making it a test\n> \n> Whoops, yes you are exactly right.  I'll remove them.\n> \n"},{"id":"392062","messageId":"CABPp-BFij++-6P2ht1EacGXaX4vA_CuBQEfz6M9w9CadXHC8Jw@mail.gmail.com","threadId":"52828","inReplyTo":"deae766d-f552-2e30-fb49-e7e187ee984b@gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-19T16:00:45Z","receivedAt":"2020-02-19T16:01:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Phillip,\n\nOn Wed, Feb 19, 2020 at 3:01 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> Hi Elijah\n>\n> On 18/02/2020 15:59, Elijah Newren wrote:\n> > On Tue, Feb 18, 2020 at 7:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> >>\n> >> Hi Elijah\n> >>\n> >> On 17/02/2020 19:12, Elijah Newren wrote:\n> >>> I forgot to add some cc's in GitGitGadget before submitting; adding now...\n> >>>\n> >>> On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget\n> >>> <gitgitgadget@gmail.com> wrote:\n> >>>>\n> >>>> From: Elijah Newren <newren@gmail.com>\n> >>>>\n> >>>> A user discovered a case where they had a stack of 20 simple commits to\n> >>>> rebase, and the rebase would succeed in picking the first commit and\n> >>>> then error out with a pair of \"Could not execute the todo command\" and\n> >>>> \"Your local changes to the following files would be overwritten by\n> >>>> merge\" messages.\n> >>>>\n> >>>> Their steps actually made use of the -i flag, but I switched it over to\n> >>>> -m to make it simpler to trigger the bug.  With that flag, it bisects\n> >>>> back to commit 68aa495b590d (rebase: implement --merge via the\n> >>>> interactive machinery, 2018-12-11), but that's misleading.  If you\n> >>>> change the -m flag to --keep-empty, then the problem persists and will\n> >>>> bisect back to 356ee4659bb5 (sequencer: try to commit without forking\n> >>>> 'git commit', 2017-11-24)\n> >>>>\n> >>>> After playing with the testcase for a bit, I discovered that added\n> >>>> --exec \"sleep 1\" to the command line makes the rebase succeed, making me\n> >>>> suspect there is some kind of discard and reloading of caches that lead\n> >>>> us to believe that something is stat dirty, but I didn't succeed in\n> >>>> digging any further than that.\n>\n> I think `--exec true` would be better as it makes it clear that it's not\n> a timing issue. I've changed do_recursive_merge() to print the mtime and\n> mode of \"DS\" before and after the merge which gives\n>\n> HEAD is now at abd8fe3 side1\n> Rebasing (1/2) # picking commit1\n> DS mtime, mode before merge 1582109854, 120000\n> DS mtime, mode after merge 0, 120000\n> Rebasing (2/2) # picking commit2\n> DS mtime, mode before merge 0, 120000\n> error: Your local changes to the following files would be overwritten by\n> merge:\n>         DS\n>\n> So it looks like the problem is that when we pick commit1 we don't\n> update the index entry for DS properly in merge_trees()\n>\n> Best Wishes\n>\n> Phillip\n\nOh, indeed, so this was my bug.  Thanks for jumping in and\ninvestigating; I probably should have found that lead but I just\ndidn't.  Anyway, with your extra information I dug around for a bit\nand I think I found the fix.  I'll post it soon.\n"},{"id":"392071","messageId":"xmqqblpuh6ru.fsf@gitster-ct.c.googlers.com","threadId":"52828","inReplyTo":"CABPp-BFij++-6P2ht1EacGXaX4vA_CuBQEfz6M9w9CadXHC8Jw@mail.gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T16:38:13Z","receivedAt":"2020-02-19T16:38:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> I think `--exec true` would be better as it makes it clear that it's not\n>> a timing issue. I've changed do_recursive_merge() to print the mtime and\n>> mode of \"DS\" before and after the merge which gives\n>>\n>> HEAD is now at abd8fe3 side1\n>> Rebasing (1/2) # picking commit1\n>> DS mtime, mode before merge 1582109854, 120000\n>> DS mtime, mode after merge 0, 120000\n>> Rebasing (2/2) # picking commit2\n>> DS mtime, mode before merge 0, 120000\n>> error: Your local changes to the following files would be overwritten by\n>> merge:\n>>         DS\n>>\n>> So it looks like the problem is that when we pick commit1 we don't\n>> update the index entry for DS properly in merge_trees()\n>>\n>> Best Wishes\n>>\n>> Phillip\n>\n> Oh, indeed, so this was my bug.  Thanks for jumping in and\n> investigating; I probably should have found that lead but I just\n> didn't.  Anyway, with your extra information I dug around for a bit\n> and I think I found the fix.  I'll post it soon.\n\nNice.  I recall that somebody said that no bug is deep enough to\nhide from us when we have sufficient number of eyeballs ;-)\n\n"},{"id":"392076","messageId":"pull.712.v4.git.git.1582131847.gitgitgadget@gmail.com","threadId":"52828","inReplyTo":"pull.712.v3.git.git.1582064105813.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] t3433: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-19T17:04:05Z","receivedAt":"2020-02-19T17:04:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v3:\n\n * add another patch which fixes the bug. Thanks to Phillip for doing some\n   digging to find the bug wasn't in rebase but in git_merge_trees().\n\nElijah Newren (2):\n  t3433: new rebase testcase documenting a stat-dirty-like failure\n  merge-recursive: fix the refresh logic in update_file_flags\n\n merge-recursive.c                    |  7 ++--\n t/t3433-rebase-across-mode-change.sh | 48 ++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 2 deletions(-)\n create mode 100755 t/t3433-rebase-across-mode-change.sh\n\n\nbase-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v4\nPull-Request: https://github.com/git/git/pull/712\n\nRange-diff vs v3:\n\n 1:  270591cd3be = 1:  270591cd3be t3433: new rebase testcase documenting a stat-dirty-like failure\n -:  ----------- > 2:  ba297fd67bb merge-recursive: fix the refresh logic in update_file_flags\n\n-- \ngitgitgadget\n"},{"id":"392077","messageId":"ba297fd67bb98bd06408241030cf42f410d5d366.1582131847.git.gitgitgadget@gmail.com","threadId":"52828","inReplyTo":"pull.712.v4.git.git.1582131847.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] merge-recursive: fix the refresh logic in update_file_flags","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-19T17:04:07Z","receivedAt":"2020-02-19T17:04:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIf we need to delete a higher stage entry in the index to place the file\nat stage 0, then we'll lose that file's stat information.  In such\nsituations we may still be able to detect that the file on disk is the\nversion we want (as noted by our comment in the code:\n  /* do not overwrite file if already present */\n), but we do still need to update the mtime since we are creating a new\ncache_entry for that file.  Update the logic used to determine whether\nwe refresh a file's mtime.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c                    | 7 +++++--\n t/t3433-rebase-across-mode-change.sh | 2 +-\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex aee1769a7ac..e6f943c5ccc 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -998,10 +998,13 @@ static int update_file_flags(struct merge_options *opt,\n \t\tfree(buf);\n \t}\n update_index:\n-\tif (!ret && update_cache)\n-\t\tif (add_cacheinfo(opt, contents, path, 0, update_wd,\n+\tif (!ret && update_cache) {\n+\t\tint refresh = (!opt->priv->call_depth &&\n+\t\t\t       contents->mode != S_IFGITLINK);\n+\t\tif (add_cacheinfo(opt, contents, path, 0, refresh,\n \t\t\t\t  ADD_CACHE_OK_TO_ADD))\n \t\t\treturn -1;\n+\t}\n \treturn ret;\n }\n \ndiff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh\nindex f11fc35c3ee..05df964670f 100755\n--- a/t/t3433-rebase-across-mode-change.sh\n+++ b/t/t3433-rebase-across-mode-change.sh\n@@ -33,7 +33,7 @@ test_expect_success 'rebase changes with the apply backend' '\n \tgit rebase side1\n '\n \n-test_expect_failure 'rebase changes with the merge backend' '\n+test_expect_success 'rebase changes with the merge backend' '\n \ttest_when_finished \"git rebase --abort || true\" &&\n \tgit checkout -b merge-backend side2 &&\n \tgit rebase -m side1\n-- \ngitgitgadget\n"},{"id":"392078","messageId":"270591cd3be783fd294039281c969a48ee718806.1582131847.git.gitgitgadget@gmail.com","threadId":"52828","inReplyTo":"pull.712.v4.git.git.1582131847.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] t3433: new rebase testcase documenting a stat-dirty-like failure","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-19T17:04:06Z","receivedAt":"2020-02-19T17:04:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nA user discovered a case where they had a stack of 20 simple commits to\nrebase, and the rebase would succeed in picking the first commit and\nthen error out with a pair of \"Could not execute the todo command\" and\n\"Your local changes to the following files would be overwritten by\nmerge\" messages.\n\nTheir steps actually made use of the -i flag, but I switched it over to\n-m to make it simpler to trigger the bug.  With that flag, it bisects\nback to commit 68aa495b590d (rebase: implement --merge via the\ninteractive machinery, 2018-12-11), but that's misleading.  If you\nchange the -m flag to --keep-empty, then the problem persists and will\nbisect back to 356ee4659bb5 (sequencer: try to commit without forking\n'git commit', 2017-11-24)\n\nAfter playing with the testcase for a bit, I discovered that added\n--exec \"sleep 1\" to the command line makes the rebase succeed, making me\nsuspect there is some kind of discard and reloading of caches that lead\nus to believe that something is stat dirty, but I didn't succeed in\ndigging any further than that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t3433-rebase-across-mode-change.sh | 48 ++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n create mode 100755 t/t3433-rebase-across-mode-change.sh\n\ndiff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh\nnew file mode 100755\nindex 00000000000..f11fc35c3ee\n--- /dev/null\n+++ b/t/t3433-rebase-across-mode-change.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='git rebase across mode change'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir DS &&\n+\t>DS/whatever &&\n+\tgit add DS &&\n+\tgit commit -m base &&\n+\n+\tgit branch side1 &&\n+\tgit branch side2 &&\n+\n+\tgit checkout side1 &&\n+\tgit rm -rf DS &&\n+\ttest_ln_s_add unrelated DS &&\n+\tgit commit -m side1 &&\n+\n+\tgit checkout side2 &&\n+\t>unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m commit1 &&\n+\n+\techo >>unrelated &&\n+\tgit commit -am commit2\n+'\n+\n+test_expect_success 'rebase changes with the apply backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b apply-backend side2 &&\n+\tgit rebase side1\n+'\n+\n+test_expect_failure 'rebase changes with the merge backend' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-backend side2 &&\n+\tgit rebase -m side1\n+'\n+\n+test_expect_success 'rebase changes with the merge backend with a delay' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -b merge-delay-backend side2 &&\n+\tgit rebase -m --exec \"sleep 1\" side1\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"392090","messageId":"xmqq4kvmfmjy.fsf@gitster-ct.c.googlers.com","threadId":"52828","inReplyTo":"ba297fd67bb98bd06408241030cf42f410d5d366.1582131847.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] merge-recursive: fix the refresh logic in update_file_flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T18:40:17Z","receivedAt":"2020-02-19T18:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> If we need to delete a higher stage entry in the index to place the file\n> at stage 0, then we'll lose that file's stat information.  In such\n> situations we may still be able to detect that the file on disk is the\n> version we want (as noted by our comment in the code:\n>   /* do not overwrite file if already present */\n> ), but we do still need to update the mtime since we are creating a new\n> cache_entry for that file.  Update the logic used to determine whether\n> we refresh a file's mtime.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  merge-recursive.c                    | 7 +++++--\n>  t/t3433-rebase-across-mode-change.sh | 2 +-\n>  2 files changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index aee1769a7ac..e6f943c5ccc 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -998,10 +998,13 @@ static int update_file_flags(struct merge_options *opt,\n>  \t\tfree(buf);\n>  \t}\n>  update_index:\n> -\tif (!ret && update_cache)\n> -\t\tif (add_cacheinfo(opt, contents, path, 0, update_wd,\n> +\tif (!ret && update_cache) {\n> +\t\tint refresh = (!opt->priv->call_depth &&\n> +\t\t\t       contents->mode != S_IFGITLINK);\n> +\t\tif (add_cacheinfo(opt, contents, path, 0, refresh,\n>  \t\t\t\t  ADD_CACHE_OK_TO_ADD))\n>  \t\t\treturn -1;\n\nHmph, !.call_depth would avoid resetting update_wd to 0, so the only\ndifference this patch makes is when the caller of this helper passed\n(update_wd == 0) during the outermost merge.  We did not tell\nadd_cacheinfo() to refresh, and refresh_cache_entry() was not\ncalled.  But the new code forces refresh to happen for normal\nentries.  The proposed log message explains that a refresh is needed\nfor a new cache entry, but if I am reading the code correctly, this\nfunction is called with !update_wd from two places, one of which is\nthe \"Adding %s\" /* do not overwrite ... */ the log message mentions.\n\nBut the other one?  When both sides added identically, we do have an\nup-to-date result on our side already, so shouldn't we avoid forcing\nupdate_wd in that case?\n\nI do not think passing refresh==1 in that case will produce an\nincorrect result, but doesn't it force an unnecessary refreshing?\n\nPuzzled.\n\n> +\t}\n>  \treturn ret;\n>  }\n>  \n> diff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh\n> index f11fc35c3ee..05df964670f 100755\n> --- a/t/t3433-rebase-across-mode-change.sh\n> +++ b/t/t3433-rebase-across-mode-change.sh\n> @@ -33,7 +33,7 @@ test_expect_success 'rebase changes with the apply backend' '\n>  \tgit rebase side1\n>  '\n>  \n> -test_expect_failure 'rebase changes with the merge backend' '\n> +test_expect_success 'rebase changes with the merge backend' '\n>  \ttest_when_finished \"git rebase --abort || true\" &&\n>  \tgit checkout -b merge-backend side2 &&\n>  \tgit rebase -m side1\n"},{"id":"392096","messageId":"CABPp-BFH2qgM2oR-6g-3RgPegpq4yZujizxCb=_Ax0g2WFOYYw@mail.gmail.com","threadId":"52828","inReplyTo":"xmqq4kvmfmjy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 2/2] merge-recursive: fix the refresh logic in update_file_flags","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-19T19:32:03Z","receivedAt":"2020-02-19T19:32:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 19, 2020 at 10:40 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > If we need to delete a higher stage entry in the index to place the file\n> > at stage 0, then we'll lose that file's stat information.  In such\n> > situations we may still be able to detect that the file on disk is the\n> > version we want (as noted by our comment in the code:\n> >   /* do not overwrite file if already present */\n> > ), but we do still need to update the mtime since we are creating a new\n> > cache_entry for that file.  Update the logic used to determine whether\n> > we refresh a file's mtime.\n> >\n> > Signed-off-by: Elijah Newren <newren@gmail.com>\n> > ---\n> >  merge-recursive.c                    | 7 +++++--\n> >  t/t3433-rebase-across-mode-change.sh | 2 +-\n> >  2 files changed, 6 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/merge-recursive.c b/merge-recursive.c\n> > index aee1769a7ac..e6f943c5ccc 100644\n> > --- a/merge-recursive.c\n> > +++ b/merge-recursive.c\n> > @@ -998,10 +998,13 @@ static int update_file_flags(struct merge_options *opt,\n> >               free(buf);\n> >       }\n> >  update_index:\n> > -     if (!ret && update_cache)\n> > -             if (add_cacheinfo(opt, contents, path, 0, update_wd,\n> > +     if (!ret && update_cache) {\n> > +             int refresh = (!opt->priv->call_depth &&\n> > +                            contents->mode != S_IFGITLINK);\n> > +             if (add_cacheinfo(opt, contents, path, 0, refresh,\n> >                                 ADD_CACHE_OK_TO_ADD))\n> >                       return -1;\n>\n> Hmph, !.call_depth would avoid resetting update_wd to 0, so the only\n> difference this patch makes is when the caller of this helper passed\n> (update_wd == 0) during the outermost merge.  We did not tell\n> add_cacheinfo() to refresh, and refresh_cache_entry() was not\n> called.  But the new code forces refresh to happen for normal\n> entries.  The proposed log message explains that a refresh is needed\n> for a new cache entry, but if I am reading the code correctly, this\n> function is called with !update_wd from two places, one of which is\n> the \"Adding %s\" /* do not overwrite ... */ the log message mentions.\n>\n> But the other one?  When both sides added identically, we do have an\n> up-to-date result on our side already, so shouldn't we avoid forcing\n> update_wd in that case?\n\nThis change doesn't force update_wd (write out a new file, also\nimplies refreshing is needed), this only forces refreshing (check\nstat-related fields of existing file).\n\n> I do not think passing refresh==1 in that case will produce an\n> incorrect result, but doesn't it force an unnecessary refreshing?\n>\n> Puzzled.\n\nIt does force a refreshing, and it is a necessary one based on\nmerge-recursive's design.  You can verify by putting an \"exit 1\" right\nafter \"git merge side\" in t6022.37 (\"avoid unnecessary update,\nrename/add-dest\").  If you do that, then cd into the test results\ndirectory, you'll see the following:\n\n$ git diff-index --name-only HEAD\nnewfile\n$ git update-index --refresh\n$ git diff-index --name-only HEAD\n$\n\nAfter my patch, newfile won't be stat dirty.\n\n\nAs for why it's needed, let's dig into the code case you are\nhighlighting; it's code for a rename/add conflict case:\n\n            } else if ((dst_other.mode == ren1->pair->two->mode) &&\n                   oideq(&dst_other.oid, &ren1->pair->two->oid)) {\n                /*\n                 * Added file on the other side identical to\n                 * the file being renamed: clean merge.\n                 * Also, there is no need to overwrite the\n                 * file already in the working copy, so call\n                 * update_file_flags() instead of\n                 * update_file().\n                 */\n                if (update_file_flags(opt,\n                              ren1->pair->two,\n                              ren1_dst,\n                              1, /* update_cache */\n                              0  /* update_wd    */))\n                    clean_merge = -1;\n\n\nNote that the fact that this was a rename/add conflict means that\nunpack_trees() will create an index with two unstaged entries for the\ngiven file, while leaving the file alone on disk.  When this section\nof code calls update_file_flags, it skips over the bits about updating\nthe working tree (since update_wd is 0), then calls add_cacheinfo with\nrefresh=1 (was refresh=0 before my patch).  add_cacheinfo() will then\ncall make_cache_entry() and add_index_entry(), which will end up\nreplacing the two unstaged entries in the index with the new stage 0\nentry, but the new stage 0 entry will not have all 0 ce_stat_data.\nOnly if refresh=1 will it then call refresh_cache_entry().\n\nSo, this was a bug all along for BOTH cases, we just didn't notice before.\n\n\n(And if you are complaining that we had stat information that we could\nhave used if it hadn't been lost based on the design of\nmerge-recursive, then yes you are correct.  If my current design for\nmerge-ort works out, then it'll avoid this extra unnecessary stat.\nBut that's not easy to achieve with the\ncall-unpack-trees-then-fix-it-up design.)\n\n\nElijah\n"},{"id":"392097","messageId":"73f32ad3-204e-09a8-d424-5a3534358676@gmail.com","threadId":"52828","inReplyTo":"CABPp-BFij++-6P2ht1EacGXaX4vA_CuBQEfz6M9w9CadXHC8Jw@mail.gmail.com","subject":"Re: [PATCH] t3424: new rebase testcase documenting a stat-dirty-like failure","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-02-19T19:33:15Z","receivedAt":"2020-02-19T19:33:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 19/02/2020 16:00, Elijah Newren wrote:\n> Hi Phillip,\n> \n>>\n>> HEAD is now at abd8fe3 side1\n>> Rebasing (1/2) # picking commit1\n>> DS mtime, mode before merge 1582109854, 120000\n>> DS mtime, mode after merge 0, 120000\n>> Rebasing (2/2) # picking commit2\n>> DS mtime, mode before merge 0, 120000\n>> error: Your local changes to the following files would be overwritten by\n>> merge:\n>>          DS\n>>\n>> So it looks like the problem is that when we pick commit1 we don't\n>> update the index entry for DS properly in merge_trees()\n>>\n>> Best Wishes\n>>\n>> Phillip\n> \n> Oh, indeed, so this was my bug.  Thanks for jumping in and\n> investigating; I probably should have found that lead but I just\n> didn't.  Anyway, with your extra information I dug around for a bit\n> and I think I found the fix.  I'll post it soon.\n\nI'm glad that was helpful, thanks for fixing the bug\n\nPhillip\n"},{"id":"392119","messageId":"xmqq36b6dzox.fsf@gitster-ct.c.googlers.com","threadId":"52828","inReplyTo":"CABPp-BFH2qgM2oR-6g-3RgPegpq4yZujizxCb=_Ax0g2WFOYYw@mail.gmail.com","subject":"Re: [PATCH v4 2/2] merge-recursive: fix the refresh logic in update_file_flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T21:39:26Z","receivedAt":"2020-02-19T21:39:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> Hmph, !.call_depth would avoid resetting update_wd to 0, so the only\n>> difference this patch makes is when the caller of this helper passed\n>> (update_wd == 0) during the outermost merge.  We did not tell\n>> add_cacheinfo() to refresh, and refresh_cache_entry() was not\n>> called.  But the new code forces refresh to happen for normal\n>> entries.  The proposed log message explains that a refresh is needed\n>> for a new cache entry, but if I am reading the code correctly, this\n>> function is called with !update_wd from two places, one of which is\n>> the \"Adding %s\" /* do not overwrite ... */ the log message mentions.\n>>\n>> But the other one?  When both sides added identically, we do have an\n>> up-to-date result on our side already, so shouldn't we avoid forcing\n>> update_wd in that case?\n>\n> This change doesn't force update_wd (write out a new file, also\n> implies refreshing is needed), this only forces refreshing (check\n> stat-related fields of existing file).\n>\n>> I do not think passing refresh==1 in that case will produce an\n>> incorrect result, but doesn't it force an unnecessary refreshing?\n>>\n>> Puzzled.\n>\n> It does force a refreshing, and it is a necessary one based on\n> merge-recursive's design.  You can verify by putting an \"exit 1\" right\n> ...\n> So, this was a bug all along for BOTH cases, we just didn't notice before.\n\nAh, thanks.  It was just me getting a wrong impression from the\nproposed log message that only the other one needed refresh; if both\nsides need a refresh, then the change is absolutely correct.\n\nThanks for clearing my puzzlement.  Will queue.\n\n"}]}