Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[WIP] RAT-473: take global gitignore into account #433

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raboof
Copy link
Member

@raboof raboof commented Feb 2, 2025

Take into account exclusions from the global gitignore file if it is present in the default location. It's kind of icky to read environment variables in code called from an enum constructor, but I didn't see a more elegant solution yet either. Relatedly, I have not yet created a unit test (or made sure that a global gitignore will not interfere with a global gitignore being present), hence the WIP status.

https://git-scm.com/docs/gitignore

Take into account exclusions from the global gitignore file if it is
present in the default location. It's kind of icky to read environment
variables in code called from an enum constructor, but I didn't see a
more elegant solution yet either. Relatedly, I have not yet created a
unit test (or made sure that a global gitignore will not interfere with
a global gitignore being present), hence the WIP status.

https://git-scm.com/docs/gitignore
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

Just a couple of questions.

Comment on lines +313 to +326

/** The location of the global gitignore file to process */
private static String globalGitIgnore() {
String xdgConfigHome = System.getenv("XDG_CONFIG_HOME");
if (xdgConfigHome != null && !xdgConfigHome.isEmpty()) {
return xdgConfigHome + File.pathSeparator + "git" + File.pathSeparator + "ignore";
} else {
String home = System.getenv("HOME");
if (home == null) {
home = "";
}
return home + File.pathSeparator + ".config" + File.pathSeparator + "git" + File.pathSeparator + "ignore";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this on a windows system? I just got back from FOSDEM and am trying to clean up a bad merge so I can't check this right now but, I think that the slashes are required to be "/" in this pattern list. There are methods that will convert them for you -- but again that is in the code I am trying to fix.

private static String globalGitIgnore() {
String xdgConfigHome = System.getenv("XDG_CONFIG_HOME");
if (xdgConfigHome != null && !xdgConfigHome.isEmpty()) {
return xdgConfigHome + File.pathSeparator + "git" + File.pathSeparator + "ignore";
Copy link
Contributor

@Claudenw Claudenw Feb 3, 2025

Choose a reason for hiding this comment

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

Are you certain that the default GIT process is to ignore XDG_CONFIG_HOME if it is empy? An empty XDG_CONFIG_HOME could be a way to ignore the HOME based isgnore.

Never mind the above comment. You are correct. However, for complete implementation you need to read a file specified by core.excludesFile in the user’s ~/.gitconfig.

I am also concerned about doing this automatically. It feels like it should be part of the GitIgnoreProcessor. So that users can turn it on/off. We also need to think long and hard about reading files outside of the current directory tree.

The GitIgnoreProcessor is currently undergoing changes which I hope we will have completed by the end of the week. The problem is a massive PR that I am breaking out into smaller reviewable chunks.

Once that is completed, and if you are willing to write the code to read the ~/.giconfig we can then merge this into the GitIgnoreProcessor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll wait until your GitIgnoreProcessor work is finished

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to provide this as a separate configuration option? - I wonder if it takes too much time to find the global ignore file on all platforms/operating systems upon each run. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make sense to provide this as a separate configuration option?

I'll have a look if/how that fits in the new structure Claude is preparing. I think it'd make sense to follow what git does by default, but having a way to suppress it could be good.

I wonder if it takes too much time to find the global ignore file on all platforms/operating systems upon each run. WDYT?

That sounds unlikely to me: finding it is only 1-2 environment variable lookups and opening 1 file. Given RAT then proceeds to open every unignored file in the repo, the extra time seems negligible?

@ottlinger
Copy link
Contributor

@raboof would you mind creating a RAT-jira ticket to describe your intentions? Thanks

Do you have a use case where I have something in my global gitignore that is not part of my project? It seems a bit mysterious to me. Thx for a bit more context.

@ottlinger ottlinger changed the title WIP: take into account global gitignore WIP: take global gitignore into account Feb 3, 2025
@raboof raboof changed the title WIP: take global gitignore into account [RAT-473][WIP] take global gitignore into account Feb 3, 2025
@raboof
Copy link
Member Author

raboof commented Feb 3, 2025

@raboof would you mind creating a RAT-jira ticket to describe your intentions? Thanks

Do you have a use case where I have something in my global gitignore that is not part of my project? It seems a bit mysterious to me. Thx for a bit more context.

Done (RAT-473). Yes, I put some things in there for development tools that I use and that use files in the project directory. That way I don't need to have a perpetually-dirty .gitignore in the project or contribute those unusual special cases to the upstream .gitignore.

@ottlinger ottlinger changed the title [RAT-473][WIP] take global gitignore into account [WIP] RAT-473: take global gitignore into account Feb 3, 2025
@Claudenw
Copy link
Contributor

Claudenw commented Feb 6, 2025

@raboof Are some of the things you put in your private ignore file found in other StandardCollection entries? Would it make sense to create more StandardCollection entries for anything that is missing?

The changes that you want to work against are in Pull request #439
Specifically:

  • in the AbstractFileProcessorBuilder.build(final DocumentName root) you want to add a call to a new method void initializeBuilder(final DocumentName root) that you will create in AbstractFileProcessorBuilder as a method that does nothing.
  • in GitIgnoreBuilder you want to implement initializeBuilder() to perform the file name lookup logic and if a file is found process it using by createing a LevelBuilder for level -1 and calling protected MatcherSet process(final Consumer<MatcherSet> matcherSetConsumer, final DocumentName root, final DocumentName documentName) to process the file.

See AbstractFileProcessBuilder.checkDirectory() for an example of creating and using the LevelBuilder.

Bascically the system works by creating MatcherSets for each level (.gitignore) file and then applies them in reverse order so that the later ones override the earlier ones as is requried by the gitignore spec. A MatcherSet has two matchers, one that specifies the files that are to be explicitly included (overrides earlier excludes), and one to list files to exclude. The process() method will do that correctly if you have the proper root and documentName specified.

@Claudenw
Copy link
Contributor

Claudenw commented Feb 9, 2025

@raboof all code changes are now in master. Please feel free to build against that. There are more changes coming but they are mostly testing and should not impact your changes.

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

Successfully merging this pull request may close these issues.

3 participants