Skip to content

Conversation

@BobLd
Copy link
Collaborator

@BobLd BobLd commented Nov 13, 2025

The regression was "introduced" in 5a6b3970 but removing the code now makes the Issue874 test fail. As a result it appears this commit just surfaced another issue (cc @ricflams)

@EliotJones do you remember why you introduced custom order logic when not doing brute force? I'm now using the same logic for both paths and it looks like it works.This was originally part of commit 0afe021a

@EliotJones
Copy link
Member

EliotJones commented Nov 13, 2025

It has been a while since I wrote the code so I don't remember the context but in general the Xref streams and tables form a linked list (that you can only navigate in reverse by /Prev) so the custom ordering logic respects the order of that linked list rather than the file order.

This might be the case where an incremental update to a file touches an existing object and the update logic writes the new Xref before (in file order) the existing file's Xref. Does this happen in reality? I'm not sure.

Ultimately I don't think we need to respect the linked list in most scenarios so file order is probably a safer default.

@BobLd
Copy link
Collaborator Author

BobLd commented Nov 13, 2025

@EliotJones thanks a lot for getting back to me - understood. I'll merge as is and let's see if in breaks any downstream unit tests

@BobLd BobLd merged commit f4e7db5 into UglyToad:master Nov 13, 2025
2 checks passed
@BobLd BobLd deleted the issues/1208 branch November 13, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants