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:bug in bitpos function for the clear bit mode #337

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

Diskein
Copy link
Contributor

@Diskein Diskein commented Oct 9, 2024

Hey! Thanks for the library! 🚀

I've recently been testing dragonfly's BITPOS implementation with fakeredis and stumbled upon some bugs in fakeredis's implementation, so I've decided to fix it. The misbehaving is related to the clear bit mode i.e. when BITPOS command looking for 0 bit.

https://redis.io/docs/latest/commands/bitpos/

If we look for clear bits (the bit argument is 0) and the string only contains bits set to 1, the function returns the first bit not part of the string on the right. So if the string is three bytes set to the value 0xff the command BITPOS key 0 will return 24, since up to bit 23 all the bits are 1.

The function considers the right of the string as padded with zeros if you look for clear bits and specify no range or the start argument only.

However, this behavior changes if you are looking for clear bits and specify a range with both start and end. If a clear bit isn't found in the specified range, the function returns -1 as the user specified a clear range and there are no 0 bits in that range.

This PR fixes this, so for the clear bit mode:

  • fakeredis returns the number of bits instead of -1 if it doesn't find 0.
  • It returns 0 if the key doesn't exist.

- fakeredis returns the number of bits instead of -1 if it doesn't find 0.
- It returns 0 if the key doesn't exist.
@Diskein Diskein requested a review from cunla as a code owner October 9, 2024 21:35
@cunla
Copy link
Owner

cunla commented Oct 10, 2024

Hi, thank you for the fix.
Please run flake8 and black on the code.
Also, it is not clear to me why you created positive_range instead of fixing fix_range?

@cunla cunla merged commit 858bdd5 into cunla:master Oct 10, 2024
51 of 52 checks passed
@cunla
Copy link
Owner

cunla commented Oct 10, 2024

Hi, I simplified the changes a bit.
Thanks again for your contribution!

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