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

BigInts for all the things #23

Merged
merged 3 commits into from
Jul 27, 2022
Merged

BigInts for all the things #23

merged 3 commits into from
Jul 27, 2022

Conversation

atjn
Copy link
Collaborator

@atjn atjn commented Jul 27, 2022

This PR is two-fold:

Use BigInts for ino IDs

As reported in #21, using Numbers for ino IDs is dangerous, because real ino IDs are 64-bit and can overflow the 32-bit Numbers, which can make the program believe that two different files are the same file, and then only count the size of one of them.

This change is theoretically just a minor fix, but if anyone uses a custom filesystem, it now has to support BigInts, which it did not have to do previously. Therefore, I think we should release this as v4.0.0 to avoid possible breakage.

Fixes #21

Support folder size output as BigInt

This change is arguably less important, and yet much more complex. It allows the user to choose that the size of the folder is returned as a BigInt. This is only necessary if you are working with crazy large folders, but there can be other practical reasons that you'd want the output as a BigInt.
It's a pretty common feature in other node libraries, and is also modeled to work the same way as other libraries, such as fs.

@atjn
Copy link
Collaborator Author

atjn commented Jul 27, 2022

@alessioalex this is a major change, so I would like a thumbs up from you before I publish it :)

Copy link
Owner

@alessioalex alessioalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great man, thank you!

Feel free to merge and publish it!

@atjn atjn merged commit 427d014 into master Jul 27, 2022
@atjn atjn deleted the bigint branch July 27, 2022 13:37
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.

Number overflow
2 participants