| Message ID | pull.712.git.git.1581959751454.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t3424: new rebase testcase documenting a stat-dirty-like failure | expand |
I forgot to add some cc's in GitGitGadget before submitting; adding now... On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Elijah Newren <newren@gmail.com> > > A user discovered a case where they had a stack of 20 simple commits to > rebase, and the rebase would succeed in picking the first commit and > then error out with a pair of "Could not execute the todo command" and > "Your local changes to the following files would be overwritten by > merge" messages. > > Their steps actually made use of the -i flag, but I switched it over to > -m to make it simpler to trigger the bug. With that flag, it bisects > back to commit 68aa495b590d (rebase: implement --merge via the > interactive machinery, 2018-12-11), but that's misleading. If you > change the -m flag to --keep-empty, then the problem persists and will > bisect back to 356ee4659bb5 (sequencer: try to commit without forking > 'git commit', 2017-11-24) > > After playing with the testcase for a bit, I discovered that added > --exec "sleep 1" to the command line makes the rebase succeed, making me > suspect there is some kind of discard and reloading of caches that lead > us to believe that something is stat dirty, but I didn't succeed in > digging any further than that. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > t3424: new rebase testcase documenting a stat-dirty-like failure > > A user discovered a case where they had a stack of 20 simple commits to > rebase, and the rebase would succeed in picking the first commit and > then error out with a pair of "Could not execute the todo command" and > "Your local changes to the following files would be overwritten by > merge" messages. > > Their steps actually made use of the -i flag, but I switched it over to > -m to make it simpler to trigger the bug. With that flag, it bisects > back to commit 68aa495b590d (rebase: implement --merge via the > interactive machinery, 2018-12-11), but that's misleading. If you change > the -m flag to --keep-empty, then the problem persists and will bisect > back to 356ee4659bb5 (sequencer: try to commit without forking 'git > commit', 2017-11-24) > > After playing with the testcase for a bit, I discovered that added > --exec "sleep 1" to the command line makes the rebase succeed, making me > suspect there is some kind of discard and reloading of caches that lead > us to believe that something is stat dirty, but I didn't succeed in > digging any further than that. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v1 > Pull-Request: https://github.com/git/git/pull/712 > > t/t3424-rebase-across-mode-change.sh | 52 ++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > create mode 100755 t/t3424-rebase-across-mode-change.sh > > diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh > new file mode 100755 > index 00000000000..4d2eb1dd7c6 > --- /dev/null > +++ b/t/t3424-rebase-across-mode-change.sh > @@ -0,0 +1,52 @@ > +#!/bin/sh > + > +test_description='git rebase across mode change' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + rm -rf ../stupid && > + git init ../stupid && > + cd ../stupid && > + mkdir DS && > + >DS/whatever && > + git add DS && > + git commit -m base && > + > + git branch side1 && > + git branch side2 && > + > + git checkout side1 && > + git rm -rf DS && > + ln -s unrelated DS && > + git add DS && > + git commit -m side1 && > + > + git checkout side2 && > + >unrelated && > + git add unrelated && > + git commit -m commit1 && > + > + echo >>unrelated && > + git commit -am commit2 > +' > + > +test_expect_success 'rebase changes with the apply backend' ' > + test_when_finished "git rebase --abort || true" && > + git checkout -b apply-backend side2 && > + git rebase side1 > +' > + > +test_expect_failure 'rebase changes with the merge backend' ' > + test_when_finished "git rebase --abort || true" && > + git checkout -b merge-backend side2 && > + git rebase -m side1 > +' > + > +test_expect_success 'rebase changes with the merge backend with a delay' ' > + test_when_finished "git rebase --abort || true" && > + git checkout -b merge-delay-backend side2 && > + git rebase -m --exec "sleep 1" side1 > +' > + > +test_done > > base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903 > -- > gitgitgadget
Hi Elijah, I am awfully short on time these days, so just a very quick observation: On Mon, 17 Feb 2020, Elijah Newren via GitGitGadget wrote: > diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh > new file mode 100755 > index 00000000000..4d2eb1dd7c6 > --- /dev/null > +++ b/t/t3424-rebase-across-mode-change.sh > @@ -0,0 +1,52 @@ > +#!/bin/sh > + > +test_description='git rebase across mode change' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + rm -rf ../stupid && > + git init ../stupid && > + cd ../stupid && > + mkdir DS && > + >DS/whatever && > + git add DS && > + git commit -m base && > + > + git branch side1 && > + git branch side2 && > + > + git checkout side1 && > + git rm -rf DS && > + ln -s unrelated DS && This requires symbolic links. Therefore it won't work on Windows, and will at least need the `SYMLINKS` prereq. Maybe there is a way, though, to change the test so it does not require a symbolic link here? Thanks, Dscho > + git add DS && > + git commit -m side1 && > + > + git checkout side2 && > + >unrelated && > + git add unrelated && > + git commit -m commit1 && > + > + echo >>unrelated && > + git commit -am commit2 > +' > + > +test_expect_success 'rebase changes with the apply backend' ' > + test_when_finished "git rebase --abort || true" && > + git checkout -b apply-backend side2 && > + git rebase side1 > +' > + > +test_expect_failure 'rebase changes with the merge backend' ' > + test_when_finished "git rebase --abort || true" && > + git checkout -b merge-backend side2 && > + git rebase -m side1 > +' > + > +test_expect_success 'rebase changes with the merge backend with a delay' ' > + test_when_finished "git rebase --abort || true" && > + git checkout -b merge-delay-backend side2 && > + git rebase -m --exec "sleep 1" side1 > +' > + > +test_done > > base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903 > -- > gitgitgadget >
Hi Elijah On 17/02/2020 19:12, Elijah Newren wrote: > I forgot to add some cc's in GitGitGadget before submitting; adding now... > > On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Elijah Newren <newren@gmail.com> >> >> A user discovered a case where they had a stack of 20 simple commits to >> rebase, and the rebase would succeed in picking the first commit and >> then error out with a pair of "Could not execute the todo command" and >> "Your local changes to the following files would be overwritten by >> merge" messages. >> >> Their steps actually made use of the -i flag, but I switched it over to >> -m to make it simpler to trigger the bug. With that flag, it bisects >> back to commit 68aa495b590d (rebase: implement --merge via the >> interactive machinery, 2018-12-11), but that's misleading. If you >> change the -m flag to --keep-empty, then the problem persists and will >> bisect back to 356ee4659bb5 (sequencer: try to commit without forking >> 'git commit', 2017-11-24) >> >> After playing with the testcase for a bit, I discovered that added >> --exec "sleep 1" to the command line makes the rebase succeed, making me >> suspect there is some kind of discard and reloading of caches that lead >> us to believe that something is stat dirty, but I didn't succeed in >> digging any further than that. Intriguing - it's strange that it errors out picking commit2, not commit1 I'll try and have a look at it. There seem to be some leftover bits at the start of the test that want removing see below >> Signed-off-by: Elijah Newren <newren@gmail.com> >> --- >> t3424: new rebase testcase documenting a stat-dirty-like failure >> >> A user discovered a case where they had a stack of 20 simple commits to >> rebase, and the rebase would succeed in picking the first commit and >> then error out with a pair of "Could not execute the todo command" and >> "Your local changes to the following files would be overwritten by >> merge" messages. >> >> Their steps actually made use of the -i flag, but I switched it over to >> -m to make it simpler to trigger the bug. With that flag, it bisects >> back to commit 68aa495b590d (rebase: implement --merge via the >> interactive machinery, 2018-12-11), but that's misleading. If you change >> the -m flag to --keep-empty, then the problem persists and will bisect >> back to 356ee4659bb5 (sequencer: try to commit without forking 'git >> commit', 2017-11-24) >> >> After playing with the testcase for a bit, I discovered that added >> --exec "sleep 1" to the command line makes the rebase succeed, making me >> suspect there is some kind of discard and reloading of caches that lead >> us to believe that something is stat dirty, but I didn't succeed in >> digging any further than that. >> >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-712%2Fnewren%2Fdocument-rebase-failure-v1 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-712/newren/document-rebase-failure-v1 >> Pull-Request: https://github.com/git/git/pull/712 >> >> t/t3424-rebase-across-mode-change.sh | 52 ++++++++++++++++++++++++++++ >> 1 file changed, 52 insertions(+) >> create mode 100755 t/t3424-rebase-across-mode-change.sh >> >> diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh >> new file mode 100755 >> index 00000000000..4d2eb1dd7c6 >> --- /dev/null >> +++ b/t/t3424-rebase-across-mode-change.sh >> @@ -0,0 +1,52 @@ >> +#!/bin/sh >> + >> +test_description='git rebase across mode change' >> + >> +. ./test-lib.sh >> + >> +test_expect_success 'setup' ' >> + rm -rf ../stupid && >> + git init ../stupid && >> + cd ../stupid && I think these 3 lines must be left over from you trying this out before making it a test Best Wishes Phillip >> + mkdir DS && >> + >DS/whatever && >> + git add DS && >> + git commit -m base && >> + >> + git branch side1 && >> + git branch side2 && >> + >> + git checkout side1 && >> + git rm -rf DS && >> + ln -s unrelated DS && >> + git add DS && >> + git commit -m side1 && >> + >> + git checkout side2 && >> + >unrelated && >> + git add unrelated && >> + git commit -m commit1 && >> + >> + echo >>unrelated && >> + git commit -am commit2 >> +' >> + >> +test_expect_success 'rebase changes with the apply backend' ' >> + test_when_finished "git rebase --abort || true" && >> + git checkout -b apply-backend side2 && >> + git rebase side1 >> +' >> + >> +test_expect_failure 'rebase changes with the merge backend' ' >> + test_when_finished "git rebase --abort || true" && >> + git checkout -b merge-backend side2 && >> + git rebase -m side1 >> +' >> + >> +test_expect_success 'rebase changes with the merge backend with a delay' ' >> + test_when_finished "git rebase --abort || true" && >> + git checkout -b merge-delay-backend side2 && >> + git rebase -m --exec "sleep 1" side1 >> +' >> + >> +test_done >> >> base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903 >> -- >> gitgitgadget
On Tue, Feb 18, 2020 at 6:03 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Elijah, > > I am awfully short on time these days, so just a very quick observation: > > On Mon, 17 Feb 2020, Elijah Newren via GitGitGadget wrote: > > > diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh > > new file mode 100755 > > index 00000000000..4d2eb1dd7c6 > > --- /dev/null > > +++ b/t/t3424-rebase-across-mode-change.sh > > @@ -0,0 +1,52 @@ > > +#!/bin/sh > > + > > +test_description='git rebase across mode change' > > + > > +. ./test-lib.sh > > + > > +test_expect_success 'setup' ' > > + rm -rf ../stupid && > > + git init ../stupid && > > + cd ../stupid && > > + mkdir DS && > > + >DS/whatever && > > + git add DS && > > + git commit -m base && > > + > > + git branch side1 && > > + git branch side2 && > > + > > + git checkout side1 && > > + git rm -rf DS && > > + ln -s unrelated DS && > > This requires symbolic links. Therefore it won't work on Windows, and will > at least need the `SYMLINKS` prereq. Maybe there is a way, though, to > change the test so it does not require a symbolic link here? Ah, yes, I will fix it up. Thanks.
On Tue, Feb 18, 2020 at 7:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Elijah > > On 17/02/2020 19:12, Elijah Newren wrote: > > I forgot to add some cc's in GitGitGadget before submitting; adding now... > > > > On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > >> > >> From: Elijah Newren <newren@gmail.com> > >> > >> A user discovered a case where they had a stack of 20 simple commits to > >> rebase, and the rebase would succeed in picking the first commit and > >> then error out with a pair of "Could not execute the todo command" and > >> "Your local changes to the following files would be overwritten by > >> merge" messages. > >> > >> Their steps actually made use of the -i flag, but I switched it over to > >> -m to make it simpler to trigger the bug. With that flag, it bisects > >> back to commit 68aa495b590d (rebase: implement --merge via the > >> interactive machinery, 2018-12-11), but that's misleading. If you > >> change the -m flag to --keep-empty, then the problem persists and will > >> bisect back to 356ee4659bb5 (sequencer: try to commit without forking > >> 'git commit', 2017-11-24) > >> > >> After playing with the testcase for a bit, I discovered that added > >> --exec "sleep 1" to the command line makes the rebase succeed, making me > >> suspect there is some kind of discard and reloading of caches that lead > >> us to believe that something is stat dirty, but I didn't succeed in > >> digging any further than that. > > Intriguing - it's strange that it errors out picking commit2, not > commit1 I'll try and have a look at it. There seem to be some leftover > bits at the start of the test that want removing see below I know, right? It's kinda weird. > >> +test_expect_success 'setup' ' > >> + rm -rf ../stupid && > >> + git init ../stupid && > >> + cd ../stupid && > > I think these 3 lines must be left over from you trying this out before > making it a test Whoops, yes you are exactly right. I'll remove them.
Hi Elijah On 18/02/2020 15:59, Elijah Newren wrote: > On Tue, Feb 18, 2020 at 7:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Elijah >> >> On 17/02/2020 19:12, Elijah Newren wrote: >>> I forgot to add some cc's in GitGitGadget before submitting; adding now... >>> >>> On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget >>> <gitgitgadget@gmail.com> wrote: >>>> >>>> From: Elijah Newren <newren@gmail.com> >>>> >>>> A user discovered a case where they had a stack of 20 simple commits to >>>> rebase, and the rebase would succeed in picking the first commit and >>>> then error out with a pair of "Could not execute the todo command" and >>>> "Your local changes to the following files would be overwritten by >>>> merge" messages. >>>> >>>> Their steps actually made use of the -i flag, but I switched it over to >>>> -m to make it simpler to trigger the bug. With that flag, it bisects >>>> back to commit 68aa495b590d (rebase: implement --merge via the >>>> interactive machinery, 2018-12-11), but that's misleading. If you >>>> change the -m flag to --keep-empty, then the problem persists and will >>>> bisect back to 356ee4659bb5 (sequencer: try to commit without forking >>>> 'git commit', 2017-11-24) >>>> >>>> After playing with the testcase for a bit, I discovered that added >>>> --exec "sleep 1" to the command line makes the rebase succeed, making me >>>> suspect there is some kind of discard and reloading of caches that lead >>>> us to believe that something is stat dirty, but I didn't succeed in >>>> digging any further than that. I think `--exec true` would be better as it makes it clear that it's not a timing issue. I've changed do_recursive_merge() to print the mtime and mode of "DS" before and after the merge which gives HEAD is now at abd8fe3 side1 Rebasing (1/2) # picking commit1 DS mtime, mode before merge 1582109854, 120000 DS mtime, mode after merge 0, 120000 Rebasing (2/2) # picking commit2 DS mtime, mode before merge 0, 120000 error: Your local changes to the following files would be overwritten by merge: DS So it looks like the problem is that when we pick commit1 we don't update the index entry for DS properly in merge_trees() Best Wishes Phillip >> Intriguing - it's strange that it errors out picking commit2, not >> commit1 I'll try and have a look at it. There seem to be some leftover >> bits at the start of the test that want removing see below > > I know, right? It's kinda weird. > >>>> +test_expect_success 'setup' ' >>>> + rm -rf ../stupid && >>>> + git init ../stupid && >>>> + cd ../stupid && >> >> I think these 3 lines must be left over from you trying this out before >> making it a test > > Whoops, yes you are exactly right. I'll remove them. >
Hi Phillip, On Wed, Feb 19, 2020 at 3:01 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Elijah > > On 18/02/2020 15:59, Elijah Newren wrote: > > On Tue, Feb 18, 2020 at 7:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> > >> Hi Elijah > >> > >> On 17/02/2020 19:12, Elijah Newren wrote: > >>> I forgot to add some cc's in GitGitGadget before submitting; adding now... > >>> > >>> On Mon, Feb 17, 2020 at 9:15 AM Elijah Newren via GitGitGadget > >>> <gitgitgadget@gmail.com> wrote: > >>>> > >>>> From: Elijah Newren <newren@gmail.com> > >>>> > >>>> A user discovered a case where they had a stack of 20 simple commits to > >>>> rebase, and the rebase would succeed in picking the first commit and > >>>> then error out with a pair of "Could not execute the todo command" and > >>>> "Your local changes to the following files would be overwritten by > >>>> merge" messages. > >>>> > >>>> Their steps actually made use of the -i flag, but I switched it over to > >>>> -m to make it simpler to trigger the bug. With that flag, it bisects > >>>> back to commit 68aa495b590d (rebase: implement --merge via the > >>>> interactive machinery, 2018-12-11), but that's misleading. If you > >>>> change the -m flag to --keep-empty, then the problem persists and will > >>>> bisect back to 356ee4659bb5 (sequencer: try to commit without forking > >>>> 'git commit', 2017-11-24) > >>>> > >>>> After playing with the testcase for a bit, I discovered that added > >>>> --exec "sleep 1" to the command line makes the rebase succeed, making me > >>>> suspect there is some kind of discard and reloading of caches that lead > >>>> us to believe that something is stat dirty, but I didn't succeed in > >>>> digging any further than that. > > I think `--exec true` would be better as it makes it clear that it's not > a timing issue. I've changed do_recursive_merge() to print the mtime and > mode of "DS" before and after the merge which gives > > HEAD is now at abd8fe3 side1 > Rebasing (1/2) # picking commit1 > DS mtime, mode before merge 1582109854, 120000 > DS mtime, mode after merge 0, 120000 > Rebasing (2/2) # picking commit2 > DS mtime, mode before merge 0, 120000 > error: Your local changes to the following files would be overwritten by > merge: > DS > > So it looks like the problem is that when we pick commit1 we don't > update the index entry for DS properly in merge_trees() > > Best Wishes > > Phillip Oh, indeed, so this was my bug. Thanks for jumping in and investigating; I probably should have found that lead but I just didn't. Anyway, with your extra information I dug around for a bit and I think I found the fix. I'll post it soon.
Elijah Newren <newren@gmail.com> writes: >> I think `--exec true` would be better as it makes it clear that it's not >> a timing issue. I've changed do_recursive_merge() to print the mtime and >> mode of "DS" before and after the merge which gives >> >> HEAD is now at abd8fe3 side1 >> Rebasing (1/2) # picking commit1 >> DS mtime, mode before merge 1582109854, 120000 >> DS mtime, mode after merge 0, 120000 >> Rebasing (2/2) # picking commit2 >> DS mtime, mode before merge 0, 120000 >> error: Your local changes to the following files would be overwritten by >> merge: >> DS >> >> So it looks like the problem is that when we pick commit1 we don't >> update the index entry for DS properly in merge_trees() >> >> Best Wishes >> >> Phillip > > Oh, indeed, so this was my bug. Thanks for jumping in and > investigating; I probably should have found that lead but I just > didn't. Anyway, with your extra information I dug around for a bit > and I think I found the fix. I'll post it soon. Nice. I recall that somebody said that no bug is deep enough to hide from us when we have sufficient number of eyeballs ;-)
Hi Elijah On 19/02/2020 16:00, Elijah Newren wrote: > Hi Phillip, > >> >> HEAD is now at abd8fe3 side1 >> Rebasing (1/2) # picking commit1 >> DS mtime, mode before merge 1582109854, 120000 >> DS mtime, mode after merge 0, 120000 >> Rebasing (2/2) # picking commit2 >> DS mtime, mode before merge 0, 120000 >> error: Your local changes to the following files would be overwritten by >> merge: >> DS >> >> So it looks like the problem is that when we pick commit1 we don't >> update the index entry for DS properly in merge_trees() >> >> Best Wishes >> >> Phillip > > Oh, indeed, so this was my bug. Thanks for jumping in and > investigating; I probably should have found that lead but I just > didn't. Anyway, with your extra information I dug around for a bit > and I think I found the fix. I'll post it soon. I'm glad that was helpful, thanks for fixing the bug Phillip
diff --git a/t/t3424-rebase-across-mode-change.sh b/t/t3424-rebase-across-mode-change.sh new file mode 100755 index 00000000000..4d2eb1dd7c6 --- /dev/null +++ b/t/t3424-rebase-across-mode-change.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='git rebase across mode change' + +. ./test-lib.sh + +test_expect_success 'setup' ' + rm -rf ../stupid && + git init ../stupid && + cd ../stupid && + mkdir DS && + >DS/whatever && + git add DS && + git commit -m base && + + git branch side1 && + git branch side2 && + + git checkout side1 && + git rm -rf DS && + ln -s unrelated DS && + git add DS && + git commit -m side1 && + + git checkout side2 && + >unrelated && + git add unrelated && + git commit -m commit1 && + + echo >>unrelated && + git commit -am commit2 +' + +test_expect_success 'rebase changes with the apply backend' ' + test_when_finished "git rebase --abort || true" && + git checkout -b apply-backend side2 && + git rebase side1 +' + +test_expect_failure 'rebase changes with the merge backend' ' + test_when_finished "git rebase --abort || true" && + git checkout -b merge-backend side2 && + git rebase -m side1 +' + +test_expect_success 'rebase changes with the merge backend with a delay' ' + test_when_finished "git rebase --abort || true" && + git checkout -b merge-delay-backend side2 && + git rebase -m --exec "sleep 1" side1 +' + +test_done