replay: fix failed assertion when directory renames are turned off #1540
Conversation
Yep, I'll try to take a look at it this next week; thanks for providing these details and the testcase.
I'm pretty sure you are right about the |
|
Thank you @newren!!! |
I did have a look into it, and it was a little bit of fun (as in: interesting). However, I failed to wrap my head around the full details of the rename detection cache. So I figured I'd better ask you for help, seeing as I did not make any progress clearing enough head space to dig deeper. |
My apologies; I obviously didn't get to this before going on vacation, but I haven't forgotten it. I might get some time to look at this next week... |
|
Thank you @newren ! |
|
Hey @newren here's an idea: how about pairing on this (e.g. via Zoom)? |
|
Pairing may work, if we can find a time we're both available. I had about 15 minutes to look at this today (obviously insufficient, but at least I finally had some time) and am hoping for more time tomorrow morning. But I've also got a 800 mile drive ahead of me tomorrow and a few other things. Sigh... |
I'll ping you via mail. |
There is a bug in the way renames are cached that rears its head when directory renames are turned off. This patch comes with a demonstration of this bug which will fail with the following message unless the rename cache is explicitly reset in `merge_check_renames_reusable()` when `merge.directoryRenames=false`: merge-ort.c:2909: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed. Aborted It is quite a curious bug: the same test case will succeed, without any assertion, if instead run with `merge.directoryRenames=true`. Further, the assertion does not manifest while replaying the first commit, it manifests while replaying the _second_ commit of the commit range. But it does _not_ manifest when the second commit is replayed individually. This would indicate that there is an incomplete rename cache left-over from the first replayed commit which is being reused for the second commit, and if directory rename detection is enabled, the missing paths are somehow regenerated. This was a fun, intense hunt. The solution to the riddle is that indeed, there was a stale cached rename information. The fix is easy: explicitly record the diff pair as a potential rename. Helped-by; Elijah Newren <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
replay-vs-directory-renames
branch
from
a57873d to
f4c7994
Compare
|
At long last, I found the time to look further into a suggestion @newren gave me in a PM, and it does indeed fix the bug, in a better way than my original work-around. I also changed the target branch to Git's default branch because Junio deleted the |
dscho
changed the title
| test_expect_success 'merge.directoryRenames=false' ' | ||
| # create a test case that stress-tests the rename caching | ||
| git switch -c rename-onto && | ||
| mkdir -p to-rename && | ||
| test_commit to-rename/move && | ||
| mkdir -p renamed-directory && | ||
| git mv to-rename/move* renamed-directory/ && | ||
| test_tick && | ||
| git commit -m renamed-directory && | ||
| git switch -c rename-from HEAD^ && | ||
| test_commit to-rename/add-a-file && | ||
| echo modified >to-rename/add-a-file.t && | ||
| test_tick && | ||
| git commit -m modified to-rename/add-a-file.t && | ||
| git -c merge.directoryRenames=false replay \ | ||
| --onto rename-onto rename-onto..rename-from | ||
| ' | ||
|
|
There was a problem hiding this comment.
This test case is identical to the one over here.
|
Superseded by #1875 |
2 participants
@newren maybe I can ask you to look over this first, before I send it off to the Git mailing list? I am almost 100% certain that this work-around is not only heavy-handed but also incorrect. There's got to be a better way to handle this situation. After all, the rename detection cache was designed by you specifically to accelerate long-running rebases.
I also suspect that the underlying bug has little to do with
merge.directoryRenames=falseeven if it might very well be a lot easier to trigger using that flag.Here are the details:
There is a bug in the way renames are cached that rears its head when directory renames are turned off.
This patch comes with a demonstration of this bug which will fail with the following message unless the rename cache is explicitly reset in
merge_check_renames_reusable()whenmerge.directoryRenames=false:It is quite a curious bug: the same test case will succeed, without any assertion, if instead run with
merge.directoryRenames=true.Further, the assertion does not manifest while replaying the first commit, it manifests while replaying the second commit of the commit range. But it does not manifest when the second commit is replayed individually.
This would indicate that there is an incomplete rename cache left-over from the first replayed commit which is being reused for the second commit, and if directory rename detection is enabled, the missing paths are somehow regenerated.
This will be a fun, intense hunt.
However, to use my time efficiently, I will first verify that the band-aid works to simply throw away the rename cache when running with directory rename detection turned off, and if it works, will run with it for the time being in order to unblock the ongoing experiment to replace libgit2-based PR rebases with
git replay-based ones.