Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix name ordering when names only differ in length #926

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geky
Copy link
Member

@geky geky commented Jan 17, 2024

Wild this hasn't been caught until now.

Because the exact ordering of the comparison in lfs_bd_cmp is a bit ambiguous, lfs_dir_find_match returned the wrong result when filenames were equal, and only differed in length.

For example:

  • cmp("a", "aa") should be LFS_CMP_LT
  • cmp("aaa", "aa") should be LFS_CMP_GT

We're quite lucky that none of the littlefs internals currently depend on the sorted order, otherwise we'd probably be stuck with this weird ordering for backwards compatibility reasons...

Fixed, and added some test cases over directory ordering to prevent regression in the future.

Found by @andriyndev
Related issue #923

Wild this hasn't been caught until now.

Because the exact ordering of the comparison in lfs_bd_cmp is a bit
ambiguous, lfs_dir_find_match returned the wrong result when filenames
were equal, and only differed in length.

For example:

  - cmp("a", "aa") should be LFS_CMP_LT
  - cmp("aaa", "aa") should be LFS_CMP_GT

We're quite lucky that none of the littlefs internals currently depend
on the sorted order, otherwise we'd probably be stuck with this weird
ordering for backwards compatibility reasons...

Fixed, and added some test cases over directory ordering to prevent
regression in the future.

Found by andriyndev
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 16828 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)
Code Stack Structs Coverage
Default 16828 B (+0.0%) 1448 B (+0.0%) 800 B (+0.0%) Lines 2357/2533 lines (+0.0%)
Readonly 6130 B (+0.0%) 448 B (+0.0%) 800 B (+0.0%) Branches 1202/1528 branches (+0.0%)
Threadsafe 17696 B (+0.0%) 1448 B (+0.0%) 808 B (+0.0%) Benchmarks
Multiversion 16892 B (+0.0%) 1448 B (+0.0%) 804 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18508 B (+0.0%) 1752 B (+0.0%) 804 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17484 B (+0.0%) 1440 B (+0.0%) 800 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky geky added this to the v2.9 milestone Jan 17, 2024
@geky geky added needs minor version new functionality only allowed in minor versions needs test all fixes need test coverage to prevent regression and removed next minor labels Jan 19, 2024
@geky geky removed this from the v2.9 milestone Jan 19, 2024
@geky geky marked this pull request as draft January 19, 2024 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs minor version new functionality only allowed in minor versions needs test all fixes need test coverage to prevent regression on-disk minor postponed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants