t3424: new rebase testcase documenting a stat-dirty-like failure
diff mbox series

Message ID pull.712.git.git.1581959751454.gitgitgadget@gmail.com
State New
Headers show
Series
  • t3424: new rebase testcase documenting a stat-dirty-like failure
Related show

Commit Message

Johannes Schindelin via GitGitGadget Feb. 17, 2020, 5:15 p.m. UTC
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


base-commit: e68e29171cc2d6968902e0654b5687fbe1ccb903

Comments

Elijah Newren Feb. 17, 2020, 7:12 p.m. UTC | #1
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
Johannes Schindelin Feb. 18, 2020, 2:03 p.m. UTC | #2
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
>
Phillip Wood Feb. 18, 2020, 3:05 p.m. UTC | #3
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
Elijah Newren Feb. 18, 2020, 3:55 p.m. UTC | #4
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.
Elijah Newren Feb. 18, 2020, 3:59 p.m. UTC | #5
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.
Phillip Wood Feb. 19, 2020, 11:01 a.m. UTC | #6
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.
>
Elijah Newren Feb. 19, 2020, 4 p.m. UTC | #7
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.
Junio C Hamano Feb. 19, 2020, 4:38 p.m. UTC | #8
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 ;-)
Phillip Wood Feb. 19, 2020, 7:33 p.m. UTC | #9
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

Patch
diff mbox series

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