Fix various rename corner cases #1943
Conversation
In commit 919df31 (Collect merge-related tests to t64xx, 2020-08-10), merge related tests were moved from t60xx to t64xx. Some comments in merge-ort relating to some tricky code referenced specific testcases within certain testfiles for additional information, but referred to their historical testfile names; update the testfile names to mention their modern location. Signed-off-by: Elijah Newren <[email protected]>
check_for_directory_rename() had a weirdly coded check for whether a strmap contained a certain key. Replace the temporary variable and call to strmap_get_entry() with the more natural strmap_contains() call. Signed-off-by: Elijah Newren <[email protected]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@3904e22. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@9888a97. |
|
This patch series was integrated into seen via git@b0cd9b0. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Comments? source: <[email protected]> |
|
This patch series was integrated into seen via git@996ae55. |
|
This patch series was integrated into seen via git@bcdf9ae. |
|
This patch series was integrated into seen via git@2b1b913. |
|
This patch series was integrated into seen via git@9bf31d0. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Comments? source: <[email protected]> |
|
This patch series was integrated into seen via git@94ff0f2. |
|
This patch series was integrated into seen via git@d90b430. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Comments? source: <[email protected]> |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Jul 22, 2025 at 03:23:05PM +0000, Elijah Newren via GitGitGadget wrote:
> At GitHub, we've got a real-world repository that has been triggering
> failures of the form:
>
> git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.
>
>
> Digging in, this particular corner case requires multiple things to trigger:
> (1) a rename/delete of one file, and (2) a directory rename modifying an
> unrelated rename such that this unrelated rename's target becomes the source
> of the rename/delete from (1).
>
> Unfortunately, looking around, it's not the only bug in the area. Plus, some
> of our testcases in tangential situations were not checked closely enough or
> were weird or buggy in various ways. Adding to the challenge was the fact
> that the relevant renames optimization was sometimes triggering making
> renames look like delete-and-add, and overlooking this meant I sometimes
> wasn't triggering what I thought I was triggering.
>
> The combination of challenges sometimes made me think my fixes were breaking
> things when sometimes I was just unaware of other bugs. I went in circles a
> few times and took a rather non-linear path to finding and fixing these
> issues. While I think I've turned it into a nice linear progression of
> patches, I might be a bit too deep in the mud and it might not be as linear
> or clear as I think. Let me know and I'll try to clarify anything needed.
I had a read through this whole series and it looks plausible to me. I
won't claim to fully understand it though given that I don't have a lot
of experience with the whole merge machinery.
Patrick |
|
This patch series was integrated into seen via git@d57bdb0. |
|
This patch series was integrated into seen via git@0972ba1. |
|
This patch series was integrated into seen via git@5e25ac3. |
|
This patch series was integrated into seen via git@3fd440e. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Expecting a reroll? cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@d245864. |
newren
force-pushed
the
fix-rename-corner-cases
branch
from
7238c8c to
c2eef5e
Compare
|
This patch series was integrated into seen via git@066334c. |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@480bbb7. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Looking good. source: <[email protected]> |
|
This patch series was integrated into seen via git@ebd65fe. |
|
This patch series was integrated into seen via git@9e2b32e. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Will merge to 'next'? source: <[email protected]> |
|
This patch series was integrated into seen via git@1cb887e. |
|
This patch series was integrated into seen via git@f331c9d. |
|
This patch series was integrated into seen via git@9fb5b44. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Will merge to 'next'? source: <[email protected]> |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Will cook in 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@3a3a1ee. |
|
This patch series was integrated into next via git@02536ed. |
|
This patch series was integrated into seen via git@95ed8d3. |
|
This patch series was integrated into seen via git@247153d. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Will cook in 'next'. source: <[email protected]> |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Will cook in 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@2c3d3b1. |
|
This patch series was integrated into seen via git@c2e47c7. |
|
There was a status update in the "Cooking" section about the branch Various bugs about rename handling in "ort" merge strategy have been fixed. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@d1123cd. |
|
This patch series was integrated into master via git@d1123cd. |
|
This patch series was integrated into next via git@d1123cd. |
|
Closed via d1123cd. |
Changes since v2:
Changes since v1; many thanks to Patrick for reviewing v1 in detail:
Original cover letter:
At GitHub, we've got a real-world repository that has been triggering failures of the form:
Digging in, this particular corner case requires multiple things to trigger: (1) a rename/delete of one file, and (2) a directory rename modifying an unrelated rename such that this unrelated rename's target becomes the source of the rename/delete from (1).
Unfortunately, looking around, it's not the only bug in the area. Plus, some of our testcases in tangential situations were not checked closely enough or were weird or buggy in various ways. Adding to the challenge was the fact that the relevant renames optimization was sometimes triggering making renames look like delete-and-add, and overlooking this meant I sometimes wasn't triggering what I thought I was triggering.
The combination of challenges sometimes made me think my fixes were breaking things when sometimes I was just unaware of other bugs. I went in circles a few times and took a rather non-linear path to finding and fixing these issues. While I think I've turned it into a nice linear progression of patches, I might be a bit too deep in the mud and it might not be as linear or clear as I think. Let me know and I'll try to clarify anything needed.
cc: Patrick Steinhardt [email protected]
cc: Elijah Newren [email protected]