Skip to content

Conversation

royjacobson
Copy link
Contributor

The loop had several problems - it's using step to access the strings, but the bitfields is not contiguous (there's a jump between SCRIPTING and FT_SEARCH), and it has two off-by-ones (KEYSPACE and JSON are never checked for).

@royjacobson royjacobson requested a review from kostasrim August 24, 2023 09:55
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

Great catch!

@royjacobson royjacobson requested a review from kostasrim August 24, 2023 11:27
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

Nice!

"KEYSPACE", "READ", "WRITE", "SET", "SORTED_SET", "LIST", "HASH",
"STRING", "BITMAP", "HYPERLOG", "GEO", "STREAM", "PUBSUB", "ADMIN",
"FAST", "SLOW", "BLOCKING", "DANGEROUS", "CONNECTION", "TRANSACTION", "SCRIPTING",
"_RESERVED", "_RESERVED", "_RESERVED", "_RESERVED", "_RESERVED", "_RESERVED", "_RESERVED",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should guard against +@_RESERVED but I can handle this case separately since I will make some changes anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's not in the hash table it's fine, no?

@royjacobson royjacobson merged commit 5042cb2 into main Aug 24, 2023
@royjacobson royjacobson deleted the fix_acl_tostring branch August 24, 2023 13:51
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