You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@creadur.apache.org by "nielsbasjes (via GitHub)" <gi...@apache.org> on 2023/11/25 00:03:22 UTC
[PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
nielsbasjes opened a new pull request, #167:
URL: https://github.com/apache/creadur-rat/pull/167
Fixing the issue described in https://issues.apache.org/jira/browse/RAT-335
Correctly following the rules in a gitignore file is not simply checking them all and stopping if one matches. So for the actual parsing and evaluation I have used a library I wrote a while ago for this exact purpose.
To allow having more complex rules like this and the default rules side by side I have added a `IgnoreMatcher` interface. A new subclass of `DirectoryScanner` is used which will check both the provided `IgnoreMatcher` instances and the existing rules.
Please let me know what you think of this approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1829593157
@Claudenw & @ottlinger: It now works as I have in mind also on Windows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1826271391
My main reason for the way I have done it is indeed the fact that the relatively simple "Find the first matching expression" is simply incorrect when it comes to the .gitignore rules.
I would not be surprised if something similar is true for other ignore file formats/rulesets.
Moving what I have towards something like FileFilters should be quite easy as the code I'm using already has a very similar interface.
At this point I'm totally fine with either:
- Wait with this until the design discussion is done and then I resubmit this in a new form.
- Merge this (after me having updated it with any additional feedback) and then these changes can be included as part of the whole refactoring (which means the newly provided tests can be used).
And yes, I would like to contribute where possible.
In general what I see as a pattern in the rules:
- The default glob rules simply match on a single expression or don't.
- The gitignore rules can have rules that override other rules in a single file.
- You can have multiple gitignore files in different places in a single project which only affect that area.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1826735875
I just realized I should add some more tests about the interaction between multiple gitignore files. I suspect I have not covered all corners.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "ottlinger (via GitHub)" <gi...@apache.org>.
ottlinger commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1840692560
@nielsbasjes thanks for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on code in PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#discussion_r1406836893
##########
apache-rat-plugin/src/main/java/org/apache/rat/mp/util/ignore/IgnoreMatcher.java:
##########
@@ -0,0 +1,35 @@
+package org.apache.rat.mp.util.ignore;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+public interface IgnoreMatcher {
+ /**
+ * Checks if the file matches the stored expressions.
+ * @param filename The filename to be checked
+ * @return NULL: not matched, True: must be ignored, False: it must be UNignored
+ */
+ Boolean isIgnoredFile(String filename);
Review Comment:
I have changed it to an Optional
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1826258970
First, thank you for your contribution.
All in all this looks like a good solution given the current Maven implementation. However, we have a change to the ReportConfiguration to specify a directory pattern to skip (RAT-322). This solution may indicate that that design is incomplete.
We are headed down the path of using FileFilters to filter inclusion/exclusion of files and directories.
There is also an open ticket to harmonize the UIs (RAT-323) by making sure that all functionality is supported by the `ReportConfiguraiton` and then ensuring that each UI implementation (CLI, Ant, Maven) support all the options.
Would you be interested in participating in the discussions around how to denote the inclusion/exclusion of files and directories?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "ottlinger (via GitHub)" <gi...@apache.org>.
ottlinger commented on code in PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#discussion_r1405259745
##########
apache-rat-plugin/src/main/java/org/apache/rat/mp/util/ignore/IgnoreMatcher.java:
##########
@@ -0,0 +1,35 @@
+package org.apache.rat.mp.util.ignore;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+public interface IgnoreMatcher {
+ /**
+ * Checks if the file matches the stored expressions.
+ * @param filename The filename to be checked
+ * @return NULL: not matched, True: must be ignored, False: it must be UNignored
+ */
+ Boolean isIgnoredFile(String filename);
Review Comment:
Personally I'd prefer Optional<Boolean> here in order to make it clearer & to avoid NPE in the future ..... the 3fold logics of Boolean are a bit more complex to understand IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1826730817
I have not measured for any performance impact yet.
I suspect filtering the files is a bit slower because:
- It will check all provided gitignore rules and not stop at the first match (which is part of the fix that is needed).
- It will walk through the filetree searching for .gitignore files.
- So it goes over the tree an additional time. I suspect this is the bit that may impact large projects and can be improved (see below).
- In general more gitignore files will be found that will slow the matching itself even further (which is part of the fix that is needed).
Because all .gitignore files only apply to the directory they are in it is possible to combine the two walks over the file tree. That would however mean a complete architecture change which you are already considering.
Something like: Recursively: In each directory; first load any local ignore files; do the rest; on return discard these local ignore rules again and continue.
Looking at this as a whole: This plugin will read all not ignored files. Spending a few ms more on ignoring more files should as a whole speed things up because actually reading a file and analyzing the content takes a lot more time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1828727108
I noticed that much of the code I wrote for this project really belonged at the side of the gitignore library (finding and managing all the separate gitignore files). This was mainly because of the delicate handling needed to ensure that the rules are checked in the correct order.
I also ran into a whole bunch of problems regarding to doing all of this in Windows also.
So I created a `GitIgnoreFileSet` which is really container for `all the .gitignore files in your project`.
You can create one either like this:
GitIgnoreFileSet gitIgnoreFileSet = new GitIgnoreFileSet(projectBaseDir);
or
GitIgnoreFileSet gitIgnoreFileSet = new GitIgnoreFileSet(projectBaseDir, false);
gitIgnoreFileSet.addGitIgnoreFile(new File("somedir/.gitignore"));
and then can use it like this:
if(gitIgnoreFileSet.ignoreFile(filename)) {
or use it as a FileFilter (you guys indicated this to be desirable)
File[] normalFiles = directory.listFiles(gitIgnoreFileSet);
This all means the operational code for this in this project was reduced.
I have released this new version to maven central, seems I was a bit too fast with pushing my code here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "nielsbasjes (via GitHub)" <gi...@apache.org>.
nielsbasjes commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1828775517
More fixing to do it seems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "ottlinger (via GitHub)" <gi...@apache.org>.
ottlinger merged PR #167:
URL: https://github.com/apache/creadur-rat/pull/167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RAT-335: Improve the parsing of gitignore files. [creadur-rat]
Posted by "ottlinger (via GitHub)" <gi...@apache.org>.
ottlinger commented on PR #167:
URL: https://github.com/apache/creadur-rat/pull/167#issuecomment-1826442068
@nielsbasjes thanks for the great contribution and possibility to use more feature of gitignore than before.
When I originally added this naiive GitIgnore parsing many people were not sure if it were to introduce a performance degression to add another source code management system's ignore mechanism. Have you been able to check if your feature slows RAT down a bit or boosts its performance?
To my mind this is not only a bugfix but a generally new feature as it allows many more possibilites to exclude/include files in RAT runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@creadur.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org