trapexit / mergerfs-tools

Optional tools to help manage data in a mergerfs pool
ISC License
385 stars 44 forks source link

Fix logic error in hash_file(), merge both hash functions #72

Closed 7eggert closed 5 years ago

7eggert commented 5 years ago

I moved count to above the loop, also I assign the amount being read to the count (instead of the amount that was requested to be read). Finally I skip reading a block if total is reached.

I think it's good enough.

trapexit commented 5 years ago

I see the logic error but the count is before the loop and I don't want these combined because the intent is to enhance the function to not simply read the beginning but I wanted something that worked for the time being.

Total shouldn't be -1. It should be max int.

7eggert commented 5 years ago

Please ignore the second commit here, it's because git did not switch branch on creating a new one

trapexit commented 5 years ago

Please don't mix logically separate commits into one PR.

I'd rather not normalize those functions. Clarity is lost IMO. Additional conditional logic and literal bools without meaning.

7eggert commented 5 years ago

It can't be max int as python 3 does not have a limit and the sys.maxint may be less than the max. file size.

https://stackoverflow.com/questions/7604966/maximum-and-minimum-values-for-ints

I didn't intend to commit the function merger here. I did git branch ~; git commit without checkout

trapexit commented 5 years ago

I'm not talking about max int for python itself but for off_t. It's going to be 64bit.

7eggert commented 5 years ago

I did some research now, but I didn't find a way to get an upper limit on file sizes in python. sys.maxint / sys.maxsize may be 2 GB on systems with 64 bit file offsets.

I'd not like to manually introduce a fixed limit. Using the special value -1 still seems to be the best thing to me. ¢¢

Anyway, if you'll replace the _1MB function soon, I see no need to pull these changes, but there'd be no harm either as now both file_hash and ~_1MB do work correctly.