[MSHARED-1077] Bugfix for classifier in pattern #26
Conversation
Parsing of pattern with classifier was busted, that prevented use cases like before rewrite, that was again busted due early return (not happening since rewrite).
cstamas
changed the title
Not only it's never used (as version seems alwats PosPattern), but is completely left out from methods like getPatternsAsString and hasMissedCriteries etc.
Rewrite pattern matcher to high level Java (and make it even more faster).
|
Aside of build passing above, @gnodet made a nice perf test as well (to prove that his rewrite was better than "old" one). So, this PR continues along (hence, copies the existing code from master as GN prefixed) and now perf tests have 3 classes testing. You can run perf test locally as well to ensure, but locally my results are: Legend: |
src/main/java/org/apache/maven/shared/artifact/filter/PatternIncludesArtifactFilter.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/maven/shared/artifact/filter/GNPatternIncludesArtifactFilter.java
Show resolved
Hide resolved
src/test/java/org/apache/maven/shared/artifact/filter/GNPatternIncludesArtifactFilter.java
Show resolved
Hide resolved
src/test/java/org/apache/maven/shared/artifact/filter/GNPatternIncludesArtifactFilter.java
Show resolved
Hide resolved
src/test/java/org/apache/maven/shared/artifact/filter/GNPatternIncludesArtifactFilter.java
Show resolved
Hide resolved
|
Um. The GN file is copied from master unmodified. No need to review it, should be as it was on master. Reason to keep it to have perf reference (as perf test is using it). |
| private static Artifactoid adapt( final String depTrailString ) | ||
| { | ||
| requireNonNull( depTrailString ); | ||
| // G:A:T:C:V |
There was a problem hiding this comment.
Please document how the pattern looks like with 4 components
There was a problem hiding this comment.
You want me to document org.apache.maven.artifact.Artifact#getDependencyTrail() method here?
There was a problem hiding this comment.
No, the code processes two pattern styles, no? 4 and 5 component. The comment documents on 5 components.
There was a problem hiding this comment.
There was a problem hiding this comment.
That's be fine, then we have context.
| // xxx:*:yyy -> G(xxx) + TC(yyy) | ||
| // xxx:yyy:* -> G(xxx)+A(yyy) | ||
| // xxx:yyy:zzz -> G(xxx)+A(yyy)+T(zzz) | ||
| if ( ANY.equals( tokens[0] ) && ANY.equals( tokens[1] ) && ANY.equals( tokens[2] ) ) |
There was a problem hiding this comment.
I have a problem with those:
- Those aren't documented anywhere
- How can we be sure that those aren't ambigious?
There was a problem hiding this comment.
This logic just re-implement what happened before (old and older code). My intention is not to fix the pattern spec or change any behavior (as test proves).
There was a problem hiding this comment.
Alright, agreed. Please put a comment above that those patterns are in question since they aren't documented. An open TODO.
src/test/java/org/apache/maven/shared/artifact/filter/GNPatternIncludesArtifactFilter.java
Show resolved
Hide resolved
Parsing of pattern with classifier was busted, that
prevented use cases like before rewrite, that was again
busted due early return (not happening since rewrite, but since rewrite
we do no support classifiers in patterns).
So, here is yet another rewrite, that not only fixes old use cases (w/ classifier)
but also makes it even more faster. Last, but not least, new code is
now in high level Java, instead of
char[][]and is self documenting.https://issues.apache.org/jira/browse/MSHARED-1077
(causes https://issues.apache.org/jira/browse/MASSEMBLY-955)