Repository navigation
RAT-578: Support core.excludesFile from global git config - #746
rmannibucau wants to merge 10 commits into
Conversation
GitIgnoreBuilder now reads the git configuration files ($XDG_CONFIG_HOME/git/config and ~/.gitconfig) to resolve the core.excludesFile option, including [include] path directives, before falling back to the default XDG global gitignore location. New GitConfig class parses the git config syntax (sections, quoted values, comments, line continuations) and resolves paths with git semantics (~/ expansion, relative paths against the current working directory, include paths relative to the including file with cycle detection). Also added RAT-578 changelog entry and unit + integration tests.
ottlinger
left a comment
There was a problem hiding this comment.
Thanks for the wonderful PR and the Jira ticket.
I really like the approach and hope that many users find this addition useful. Apart from my little remarks and the failing tests under Windows I'm okay with the PR.
@Claudenw do you see problems with this PR in the different UIs and the new architecture under development?
- Replace inner Section class with a Java record - Use org.apache.rat.utils.FileUtils in GitConfigTest instead of the local write helper doing raw Files IO - Make testMissingFileReturnsConfiguredFile portable across platforms by using a relative path resolved against the simulated CWD - Keep backslashes for unknown escape sequences in quoted values (fixes Windows style paths like C:\Users\...) - Add test covering backslash preservation in quoted values
Claudenw
left a comment
There was a problem hiding this comment.
Not really requesting changes, requesting clarification and manual test to ensure that your analysis of relative file name resolution is correct.
| /** The maximum depth of nested includes, matching the git limit. */ | ||
| private static final int MAX_INCLUDE_DEPTH = 10; | ||
| /** The home directory prefix of a path. */ | ||
| private static final String HOME_PREFIX = "~/"; |
There was a problem hiding this comment.
What is the HOME_PREFIX on a windows machine?
There was a problem hiding this comment.
it is HOME too - ̀EnvVar, guess you meant to use USERPROFILE` or whatever var git is using?
| return Optional.empty(); | ||
| } | ||
|
|
||
| Optional<File> fromConfig = new GitConfig().coreExcludesFile(); |
There was a problem hiding this comment.
Can you run a test for me please.
As per your notes, the GitConfig code resolves relative files against the current working directory (CWD).
Please configure a systems such that your GitConfig code will resolve the file against the CWD.
Create a directory tree something like this.
CWD
- the relative file
+---> subdirectory
+ .gitignore file
+ same relative file name different contents
When the git ignore is processed in the CWD it shoujld decend and find the .gitignore in the subdirectory. Does the relative file name get reresolved since the CWD has changed?
See https://git-scm.com/docs/gitignore command line execution.
There was a problem hiding this comment.
if i didnt mess up my impl it is resolved once - we speak of global gitconfig there
Patterns read from exclude sources that are outside the working tree, such as $GIT_COMMON_DIR/info/exclude and core.excludesFile, are treated as if they are specified at the root of the working tree."
There was a problem hiding this comment.
Sorry, this got lost in my email swamp.
My understanding is that the git ignore process is re-evaluated by git on every .gitignore. I would expect the relative file to be evaluated against the directory with the .gitignore. but I don't know of that is actually how git evaluates hence the test with the command line git ignore call to verify we get the same answer.
There was a problem hiding this comment.
have to admit i never used excludesFile in a nested gitignore but my understanding of the doc - didnt take time to check git sources but can next week if it helps - is that it is not supported, do you get it differently?
Add the RAT_GIT_GLOBAL_IGNORE environment variable so RAT's global gitignore handling no longer depends on the machine an audit runs on. It accepts 'off' to disable reading the machine global git ignore, 'on' (resolved from the git configuration, the RAT-578 default), or an explicit file path that is always used as the global ignore. The legacy RAT_NO_GIT_GLOBAL_IGNORE keeps precedence. RAT's own CI build now pins RAT_NO_GIT_GLOBAL_IGNORE so runner git config cannot perturb self-checks or integration test reports. Also drop a new spotbugs finding (IM_BAD_CHECK_FOR_ODD) introduced by GitConfig.parseFile by testing the trailing-backslash count with a bitwise check, and document the new option in EnvVar, StandardCollection GIT and changes.xml. New GitIgnoreBuilderTest cases cover 'off', an explicit path (including a missing file) and 'on'/unset resolution. Verification: apache-rat-core verify passes with 1155 tests and clean checkstyle; apache-rat-tools (38) and apache-rat-tasks (113) still pass.
…FILE Rename the EnvVar.USERPROFILE constant to USER_PROFILE for naming consistency with the other constants, while keeping the actual environment variable read as USERPROFILE (the canonical Windows name) via a getValue() override on the enum constant. Convert the RAT_GIT_GLOBAL_IGNORE 'on'/'off' keywords to a tri-state value documented explicitly as not a plain boolean: 'false' disables the global git ignore, 'true' (or unset) resolves it from the git configuration, and any other value is treated as an explicit global git ignore path. This matches the existing path-mode design while aligning the boolean keyword spelling with the rest of the RAT configuration options. Update the EnvVar, GitIgnoreBuilder and StandardCollection javadoc to use well-formed closed block HTML so it builds cleanly across JDK 17/21/25. changes.xml: mark the RAT_GIT_GLOBAL_IGNORE entry as type=add with due-to=Romain Manni-Bucau and reword it for the tri-state semantics. Switch the CI workflow from RAT_NO_GIT_GLOBAL_IGNORE to the new RAT_GIT_GLOBAL_IGNORE=false for reproducible audits. GitIgnoreBuilderTest: use 'false'/'true' (with case-insensitive checks) and keep the explicit-path coverage. Full apache-rat-core build passes: 1155 tests, checkstyle and javadoc clean.
|
@Claudenw anything open here from your side? Thanks |
Claudenw
left a comment
There was a problem hiding this comment.
I have questions about the correctness of the algorithm as well as wanting to see standard libraries used so that we don't have to support custom parsing.
| /** | ||
| * The Windows user profile directory. | ||
| * <p> | ||
| * The environment variable that is read is {@code USERPROFILE} (no underscore), which is the | ||
| * canonical name used on Windows. The enum constant is spelled {@code USER_PROFILE} to stay | ||
| * consistent with the naming of the other constants.</p> | ||
| */ |
There was a problem hiding this comment.
I don't think we want to do this. We have tried not to introduce OS specific variable and options. when you need to access the HOME directory you can use the Java system property user.home
@ottlinger I think this should be removed. Do you agree?
There was a problem hiding this comment.
issue is user.home is not authoritative for OS specific folders it is just a default (and windows one had some bugs years ago) so not sure it is always great in companies setup (with remote user folders)
There was a problem hiding this comment.
I don't like the GitConfig class in the same package as the file processors. It is small and I would like to see it in the GitIgnoreBuilder class.
| /** The maximum depth of nested includes, matching the git limit. */ | ||
| private static final int MAX_INCLUDE_DEPTH = 10; | ||
| /** The home directory prefix of a path. */ | ||
| private static final String HOME_PREFIX = "~/"; |
There was a problem hiding this comment.
The HOME_PREFIX should be build using the FSInfo object.
FSInfo fsInfo = DocumentName.FSInfo.getDefault()'
String home_prefix = "~" + fsInfo.dirSeparator()
you can also use fsInfo.normalize(text) to convert the text into a path for the specified file system.
| /** The home directory prefix of a path. */ | ||
| private static final String HOME_PREFIX = "~/"; | ||
| /** The Windows style home directory prefix of a path. */ | ||
| private static final String HOME_BACKSLASH_PREFIX = "~\\"; |
There was a problem hiding this comment.
not necessary with FSInfo usage.
| * @param includingFile the file that contains the include directive. | ||
| * @return the file to include or an empty Optional if it can not be resolved. | ||
| */ | ||
| private Optional<File> resolveInclude(final String value, final File includingFile) { |
There was a problem hiding this comment.
resolveInclude and resolveExclude seem to be the same. Perhaps a single method to do the resolution is warranted.
| File path = new File(value); | ||
| if (!path.isAbsolute()) { | ||
| // git resolves relative paths against the current working directory | ||
| path = new File(cwd, value); |
There was a problem hiding this comment.
I did some extensive testing and I could not find a configuration where relative paths were resolved within the current working directory. Git always seems to resolve the file relative to the INI file that it was read from.
you can use find . | git check-ignore -v -n --stdin --no-index to evaluate a configuration. I execute this in the resources/GitIgnoreBuilderTest/RAT-578-root directory with multiple configurations.
| * @param line the line to check. | ||
| * @return the number of trailing backslashes. | ||
| */ | ||
| private int trailingBackslashes(final String line) { |
There was a problem hiding this comment.
This method is not necessary with the use of the INIConfiguration.
| * @param value the raw value. | ||
| * @return the unquoted value. | ||
| */ | ||
| private String unquote(final String value) { |
There was a problem hiding this comment.
I believe that this also goes away with use of INIConfiguraiton.
| Set<File> includeChain = new HashSet<>(); | ||
| if (globalConfig != null) { | ||
| // when GIT_CONFIG_GLOBAL is set, neither ~/.gitconfig nor $XDG_CONFIG_HOME/git/config are read | ||
| parseFile(globalConfig, configured, includeChain, 0); | ||
| } else { | ||
| parseFile(xdgConfig, configured, includeChain, 0); | ||
| parseFile(userConfig, configured, includeChain, 0); | ||
| } | ||
| if (!configured.isEmpty()) { | ||
| String value = configured.get(configured.size() - 1); | ||
| return resolveExcludesFile(value); | ||
| } |
There was a problem hiding this comment.
This seem incorrect.
https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration
specifies that all the configuration files should be taken together and the specification closest to the project should take prescidence.
This code does not consider the system config, nor does it consider the .git/config file for the project.
I can not find documentation that says the use of GIT_CONFIG_GLOBAL means that the ~/.gitconfig and XDG based config are not read.
I can see that XDG based config is designed to override the ~/.gitconfig so only one of them would be expected.
I did find a reference that said that GIT_CONFIG_GLOBAL may contain multiple entries. Infact setting XDG_CONFIG_HOME and placing a .gitignore in that directory and then running git var -l lists both the ${XDG_CONFIG_HOME}/.gitignore and the ~/.gitignore as entries for the GIT_CONFIG_GLOBAL value
Explicitly setting GIT_CONFIG_GLOBAL does limit the git var -l results to the single value.
Explicitly setting:
set GIT_CONFIG_GLOBAL="/path/to/dir1
/path/to/dir2"
will cause git var -l to list both files.
There was a problem hiding this comment.
I tested the GIT_CONFIG_GLOBAL behavior against real git and it is a single file path, not a newline-separated list. Setting it to a value containing a newline makes git try to open that literal filename and yields no config:
$ GIT_CONFIG_GLOBAL="/tmp/dir/a.gitconfig\n/tmp/dir/b.gitconfig" git config --get core.excludesFile # exit 1, empty
$ GIT_CONFIG_GLOBAL=/tmp/dir/b.gitconfig git config --get core.excludesFile # exit 0, prints value
The "some can contain multiple values separated by newlines" text in git-var(1) describes that command's output formatting, not the input variable. In the git source (config.c::git_global_config_paths) GIT_CONFIG_GLOBAL is read as a single char * pathname which replaces ~/.gitconfig and the XDG file, exactly as this code does.
On the path resolution question I also verified with git check-ignore -v -n:
- a relative
core.excludesFilevalue is resolved against the current working directory and its patterns are anchored at the root of the working tree (matches the same file both at the root and in a subdirectory); - a relative
[include] pathvalue is resolved against the directory of the including config file.
Both match this implementation. The system config (/etc/gitconfig), includeIf and the repository .git/config remain intentionally out of scope for RAT's audit-side read.
I have updated the code accordingly (GitConfig moved into GitIgnoreBuilder as a protected static nested type, home resolved from user.home, and GIT_CONFIG_GLOBAL kept as a single path).
| * @param includeChain the canonical files of the current include chain to detect cycles. | ||
| * @param depth the current include depth. | ||
| */ | ||
| private void parseFile(final File file, final List<String> excludesFileValues, final Set<File> includeChain, final int depth) { |
There was a problem hiding this comment.
This is parsing what is effectively an INI file. Please use Apache commons configuration2 INICoanfiguration.
Since the documentation for the configurations specifies the order in which the options are accepted (which overrides which) you can use the CombinedConfiguration to get the proper layering to return the values.
In addition, you can parse the input into multiple lines by using BufferedReader.readLine() constructed on the result from fetching configuration the value.
There was a problem hiding this comment.
FTR git config files are not ini files (subsections for ex are diff)
Nest GitConfig into GitIgnoreBuilder as a protected static final inner class so the git-config reading logic stays co-located with its only consumer while remaining independently unit-testable from the same-package test class. Remove the USERPROFILE/USER_PROFILE environment variable handling and resolve the home directory from the user.home system property only. On modern JVMs (JDK 8+) user.home is derived from the OS home/profile location on every platform, so no platform specific environment variable lookup is needed. Deduplicate include and excludesFile resolution into a single resolve(value, base) helper: [include] path values are resolved against the directory of the including file and core.excludesFile values against the current working directory, matching git. Derive the home directory prefix from FSInfo.getDefault().dirSeparator() instead of a hardcoded "~/", while still accepting the alternate separator so portable git configuration files (which may use either / or \) resolve on any host. Keep GIT_CONFIG_GLOBAL as a single file path, matching git: empirical testing shows git treats the value as one pathname and a newline-separated value fails to load, contrary to git-var(1) output formatting. Full apache-rat-core build passes: 1155 tests, checkstyle and javadoc clean.
Support the git configuration option
core.excludesFile(including[include] pathdirectives) when processing global gitignore files.Changes
GitConfigclass inapache-rat-corethat parses the git global configuration files ($XDG_CONFIG_HOME/git/configand~/.gitconfig) with git semantics:core.excludesFilekey (case-insensitive), last value wins#/;) and line continuations~/expansion, relativeexcludesFilepaths resolved against the current working directory[include] pathrecursively, relative to the including file, with cycle detection (depth limit 10 like git)includeIfand/etc/gitconfigare not supported (out of scope)GitIgnoreBuilder.globalGitIgnore()now consultsGitConfigfirst, then falls back to the default XDG locationcore.excludesFileis configured but the file is missing, no fallback occurs (matching git behavior)RAT_NO_GIT_GLOBAL_IGNOREstill disables all global gitignore processingTests
GitConfigTest(16 tests): parsing, case-insensitivity, quoting, comments, precedence XDG vs~/.gitconfig,~/expansion, relative paths, includes (simple/relative/tilde/recursive/cycle), subsections, missing files, default fallbacktest_global_gitignore_from_configintegration test inGitIgnoreBuilderTestVerification
mvn -pl apache-rat-core test→ BUILD SUCCESS (checkstyle included)