Skip to content

fix #1569 - improve monitor_regex #1595

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Apr 8, 2021
Merged

fix #1569 - improve monitor_regex #1595

merged 2 commits into from
Apr 8, 2021

Conversation

leibale
Copy link
Contributor

@leibale leibale commented Apr 8, 2021

No description provided.

@leibale leibale requested a review from gkorland April 8, 2021 17:16
@lgtm-com
Copy link

lgtm-com bot commented Apr 8, 2021

This pull request fixes 1 alert when merging 79d4b26 into 7e77de8 - view on LGTM.com

fixed alerts:

  • 1 for Inefficient regular expression

@lgtm-com
Copy link

lgtm-com bot commented Apr 8, 2021

This pull request fixes 1 alert when merging 8c5506d into 7e77de8 - view on LGTM.com

fixed alerts:

  • 1 for Inefficient regular expression

@leibale leibale merged commit 2d11b6d into master Apr 8, 2021
@leibale leibale deleted the GHSL-2021-026 branch April 8, 2021 22:04
Copy link

@OnlineCop OnlineCop left a comment

Choose a reason for hiding this comment

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

The original, ending in ( ".+?")+$, would fail to match:

1234567890.0 [1 2]  "test"

(There are two spaces between the closing square bracket and the double quote.)

The new, ending in .*"$, will match that string (or any other string, so long as the last character on the line is a double quote).

Should this text match, or fail to match?

@leibale
Copy link
Contributor Author

leibale commented Apr 28, 2021

@OnlineCop Basically you're right, but since this regex is used only to check if a message from Redis is a monitor message or a reply to a command, and the only message that starts with a decimal number is a reply from the monitor command, even the new regex is an overkill...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants