Read a directory's own json before those of its subdirectories - #59
Open
TheLastDarkthorne wants to merge 1 commit into
Open
Read a directory's own json before those of its subdirectories#59TheLastDarkthorne wants to merge 1 commit into
TheLastDarkthorne wants to merge 1 commit into
Conversation
createItems() adds items in the order findFiles() returns them, and mergeDown() keeps the position of the first occurrence. A group declared in its parent's json therefore only holds its declared position if that json is read first. eachFileRecurse returns files in filesystem order, so a subdirectory's json is often read before its parent's. The group is then positioned by the order its directory happened to be visited rather than where it was declared, silently reordering siblings: subgroups get hoisted ahead of leaf items and sorted by name. Ordering matters because Mudlet evaluates triggers in tree order, so this changes which trigger sees a line first. Converting an existing package, 19 of 381 groups came out reordered, including one whose first child was an anti-illusion group that ended up in the middle of the list. Sort the discovered files shallowest-first, breaking ties by path. The tie break matters as much as the depth: eachFileRecurse returns directory entries in whatever order the platform gives - alphabetical on NTFS, effectively arbitrary on ext4 - and a group that no parent json declares takes its position from read order. Without a total order the same sources build a differently ordered package on a different machine, which is how this was found: a package that verified clean on Windows came out reordered when the same commit was built on a Linux CI runner.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
findFiles()returns each json in filesystem order,createItems()addsitems in that same order, and
mergeDown()keeps the position of whicheveroccurrence it merges first. So a group only holds the position declared in
its parent's json if that json happens to be read before its subdirectories'
jsons.
eachFileRecursefrequently reads a subdirectory's json first, andthe group is then positioned by directory-visit order instead of by its
declared position - subgroups get hoisted ahead of leaf items and end up
sorted by name.
This matters because Mudlet evaluates triggers in tree order, so a reordered
group can change which trigger sees a line first. Converting an existing
~2,500-trigger package with this bug present, 19 of 381 groups came out
reordered, including one whose first child was an anti-illusion group that
ended up in the middle of the list instead.
Fixes #44. Likely also the cause of #14, where folders come out sorted by
neither name nor creation date - directory-visit order looks exactly that
arbitrary from outside.
The fix
Sort
findFiles()'s result shallowest-first, breaking ties by path.The tie-break matters as much as the depth does.
eachFileRecursereturnsdirectory entries in whatever order the platform's filesystem gives -
alphabetical on NTFS, effectively arbitrary on ext4 - and a group that no
parent json declares takes its position from read order regardless. Without
a total order, the same sources build a differently ordered package on a
different machine. That is how the depth-only version of this fix was caught
as still-incomplete: a package that verified clean on Windows came out
reordered when the identical commit was built on a Linux CI runner. Breaking
ties by path removes the platform dependence entirely.
Verification
Caught and verified converting an existing ~2,500-trigger, 120-alias, 25-module
Mudlet package to a muddler project: comparing the built package against the
25 hand-authored originals object-by-object (structure, names, load order)
found 19 reordered groups before this fix and none after, on both a Windows
build and a from-scratch Linux CI build of the same commit.
Runnable reproduction
https://github.com/TheLastDarkthorne/mudlet-tooling-repros/tree/main/muddler/bug7-directory-read-order
Run against the stock 1.1.0 jar,
Outer's children come out asAlphathenBeta- alphabetical directory-scan order - instead of the declaredBetathen
Alpha.run_repros.pyin the same directory builds it against both astock and a patched jar and prints the resulting tree for both.