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

Add global fdignore support #575

Merged
merged 1 commit into from
May 18, 2020
Merged

Add global fdignore support #575

merged 1 commit into from
May 18, 2020

Conversation

soedirgo
Copy link
Contributor

Rust newbie here :-)

This implements global fdignore support in ~/.config/fd/ignore as mentioned here.

I have some questions:

  • I'm using std::env::home_dir() (which is deprecated) instead of using the dirs crate, because this is what the ignore crate does. Which would you prefer?
  • I'm not too sure how to handle the tests. If I have, say, * in my ~/.config/fd/ignore, the tests will break. The simplest way I can think of to deal with this is to add a !* using --ignore-file.
  • Are there parts of the doc I should change?

@sharkdp
Copy link
Owner

sharkdp commented Apr 24, 2020

Rust newbie here :-)

Welcome!

This implements global fdignore support in ~/.config/fd/ignore as mentioned here.

Great!

I'm using std::env::home_dir() (which is deprecated) instead of using the dirs crate, because this is what the ignore crate does. Which would you prefer?

I prefer using the same solution that we have for bat: follow the XDG standard, i.e. use $XDG_CONFIG_HOME, if available. Otherwise, fall back to OS-specific standard directories. On macOS, use ~/.config/fd as well (not what dirs suggests). We could copy some code from bat. We don't need the get_cache_dir part.

I'm not too sure how to handle the tests. If I have, say, * in my ~/.config/fd/ignore, the tests will break. The simplest way I can think of to deal with this is to add a !* using --ignore-file.

I think we definitely need a cleaner solution for that. We could add a --no-global-ignore-file command-line option (can be hidden from the help text) that we would always pass to fd when performing the integration tests.

Are there parts of the doc I should change?

That would be great, yes. This could be mentioned in the README and/or the man page.

src/walk.rs Outdated
.and_then(|x| if x.is_empty() { None } else { Some(PathBuf::from(x)) })
// std::env::home_dir() is deprecated. We should switch to the `dirs` crate in the future.
.or_else(|| env::home_dir().map(|p| p.join(".config")))
.map(|x| x.join("fd/ignore"))
Copy link
Owner

Choose a reason for hiding this comment

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

I guess it works on Windows as well, but maybe just use .join("fd").join("ignore").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it 👍.

@soedirgo
Copy link
Contributor Author

Thanks for the review!

I changed the global ignore file's behavior so that it gets ignored with -I. I think it should behave like --ignore-files, but -I should ignore anything that isn't explicit (i.e. --ignore-files).

@sharkdp
Copy link
Owner

sharkdp commented Apr 29, 2020

Thank you for the updates.

Two things:

First, could we please make the documentation less Unix-specific? (i.e. paths like ~/.config/fd/ignore)

Second, I know this will be hard to add to integration tests. But could you please show a list of tests that you have performed locally to make sure that this works as intended? In particular, I'm thinking about

  • Main functionality. Does it work with globs like *.txt and absolute paths like /sys?
  • Prioritization of different ignore files
  • What happens if invalid patterns are in the global ignore file?
  • Interplay with --no-ignore and --unrestricted
  • Interplay with --no-ignore-vcs (files from global ignore file should still be ignored)

@soedirgo
Copy link
Contributor Author

First, could we please make the documentation less Unix-specific? (i.e. paths like ~/.config/fd/ignore)

Updated.

Second, I know this will be hard to add to integration tests. But could you please show a list of tests that you have performed locally to make sure that this works as intended?

  • Functionality is like --ignore-files, i.e. it works with globs, but absolute paths are matched such that the root is the working directory.
  • From what I can see, priority is fd/ignore < --ignore-files < .gitignore < .ignore < .fdignore.
  • Invalid patterns will print an error and ignore the pattern, but other patterns will still be matched.
  • --no-ignore and --unrestricted ignores the global ignore file.
  • --no-ignore-vsc has no effect.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you very much for the updates!

@sharkdp sharkdp merged commit 79d5a5b into sharkdp:master May 18, 2020
@sharkdp
Copy link
Owner

sharkdp commented May 19, 2020

Released in fd 8.1.0

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.

None yet

2 participants