ignore: fix determining whether a shorter pattern negates another - #5173
Merged
Conversation
When computing whether we need to store a negative pattern, we iterate through all previously known patterns and check whether the negative pattern undoes any of the previous ones. In doing so we call `wildmatch` and check it's return for any negative error values. If there was a negative return, we will abort and bubble up that error to the caller. In fact, this check for negative values stems from the time where we still used `fnmatch` instead of `wildmatch`. For `fnmatch`, negative values indicate a "real" error, while for `wildmatch` a negative value may be returned if the matching was prematurely aborted. A premature abort may for example also happen if the pattern matches a prefix of the haystack if the pattern is shorter. Returning an error in that case is the wrong thing to do. Fix the code to compare for equality with `WM_MATCH`, only. Negative values returned by `wildmatch` are perfectly fine and thus should be ignored. Add a test that verifies we do not see the error.
ethomson
approved these changes
Jul 20, 2019
|
Hi everyone, somebody knows when this fix will be released? |
Member
Author
|
We do not yet have any schedule for the next releases. Given that it has been nearly three months since doing the last bugfix release I'd be up to prepare one towards the end of this month. |
Member
Author
|
With that being said, though, this only fixes an issue that has been introduced with the conversion to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When computing whether we need to store a negative pattern, we iterate
through all previously known patterns and check whether the negative
pattern undoes any of the previous ones. In doing so we call
wildmatchand check it's return for any negative error values. If there was a
negative return, we will abort and bubble up that error to the caller.
In fact, this check for negative values stems from the time where we
still used
fnmatchinstead ofwildmatch. Forfnmatch, negativevalues indicate a "real" error, while for
wildmatcha negative valuemay be returned if the matching was prematurely aborted. A premature
abort may for example also happen if the pattern matches a prefix of the
haystack if the pattern is shorter. Returning an error in that case is
the wrong thing to do.
Fix the code to compare for equality with
WM_MATCH, only. Negativevalues returned by
wildmatchare perfectly fine and thus should beignored. Add a test that verifies we do not see the error.
I've skimmed through the tree to see whether there's any other places where we fail to use
wildmatchcorrectly. The only place that struck me was when matching pathspecs, but I failed to come up with any breaking pattern. I think this one is fine, asWM_ABORT_*is only returned if the text is a prefix of the pattern. And as we're swallowing all negative values up the callchain we should be good.Fixes #5166