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 an option to not follow symlinks #63

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

Conversation

aplanas
Copy link

@aplanas aplanas commented Sep 20, 2017

Add follow_links option in MatchOptions struct to signalize
when to follow symlinks when the directory is traversed. The
default value is false.

@aplanas
Copy link
Author

aplanas commented Sep 20, 2017

Initial proposal for issue #62

@aplanas aplanas mentioned this pull request Sep 22, 2017
Add `follow_links` option in `MatchOptions` struct to signalize
when to follow symlinks when the directory is traversed.  The
default value is `false`.
Copy link

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

I'm interested in this and it looks good, are you willing to pick it up again?

/// }
/// ```
pub fn new() -> MatchOptions {
MatchOptions {
case_sensitive: true,
require_literal_separator: false,
require_literal_leading_dot: false,
follow_links: false,
Copy link

Choose a reason for hiding this comment

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

I think it's the right call (it matches all shells I tried as well as Python's glob module) but a default of false would be a breaking change, right?

Comment on lines +304 to +308
let metadata = if follow_links {
fs::metadata(p)
} else {
fs::symlink_metadata(p)
};
Copy link

Choose a reason for hiding this comment

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

Does the fs::metadata() call in fill_todo() need the same treatment?

@piegamesde
Copy link

I'd be against having this in MatchOptions. Glob matching should always be a comparison against strings. No file system should be harmed in the process :)

As far as I can tell, the symlink problem arises from the helper methods that walk the directory structure in some way. They should have their own separate option system instead.

@aplanas
Copy link
Author

aplanas commented Nov 12, 2021

@piegamesde Oh that is a good argument. I forgot this PR actually, but where do you think that the "follow_symlink" flag should live? As a boolean parameter is some function?

@brmmm3
Copy link
Contributor

brmmm3 commented Apr 15, 2023

@piegamesde I don't really understand your argument against this option. If I traverse a file tree and want to get, or check, the symlinks themselves then I cannot use glob without the follow_symlink option. I would appreciate to have this in included. To not change the default behavior the default value of this option can be true.
@aplanas Please can you consider to merge this with follow_symlink=true as the default value?

@piegamesde
Copy link

@brmmm3 I'm not against having a follow symlinks option. I am against putting it there, in the MatchOptions. The reason is that MatchOptions is also used by functions like Pattern::matches_with, which do not and should not involve any file system operations. Having a file system related option in that struct would be confusing and unfit.

@JohnTitor
Copy link
Member

While @piegamesde's argument is valid, I also think this is the simplest way. Maybe we can add a note describing this option doesn't make sense in some context?

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.

5 participants