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

Implement -m / --mount flag #74

Closed
wants to merge 1 commit into from
Closed

Conversation

Detegr
Copy link
Contributor

@Detegr Detegr commented Oct 7, 2017

-mount flag in find prevents find from descending into directories that reside on other filesystems. This flag does the same, and prevents fd from descending into directories that are mountpoints of other filesystems.

As this implementation relies on /proc/mounts, a check is performed when parsing command line arguments to check whether it exists and is readable. If not, the -m / --mount option is disabled.

It looks like that MacOS support would be quite easy to implement too, but I'm not sure how often this would be useful on that platform. Also Windows supports mounting filesystems under directories, but those will be symlinks and not followed by fd by default, and my guess is that nearly nobody does that anyway.

@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2017

Thank you very much for taking the time to implement this!

I'm kind of torn on this. On the one hand, this looks like a useful feature. On the other hand, I'd also like fd to stay true to its goals as stated in the first two lines in the README: "a simple, fast, and user-friendly alternative to find" and "While it does not seek to mirror all of find's powerful functionality...".

You could argue that there is no downside in adding an additional feature, and I would tend to agree with you.

However, I believe there are a few possible downsides to keep in mind when we agree to add more features like this (and I'm speaking a bit more in general, not necessarily specific to this PR):

  1. Possible performance degradation
  2. Added complexity in the code and increased maintenance burden.
  3. Added complexity in the user interface, specifically, the -h and --help texts.

Concerning point (1), this is something I would likely not tolerate. This specific PR seems harmless in this respect (however, there is at least one more if-clause, even if the feature is deactivated - but that's probably negligible).

Concerning point (2), I'm not yet decided. So far, fds code doesn't seem too complex and we can potentially handle a few more features..

Concerning point (3), I would really like the help text to stay rather concise and I want the most important command line options to be easily visible. We could experiment with showing some options in the short -h text and showing all options in the long --help text.

Sorry for the long discussion, but I'm also interested in your view concerning these points.

Coming back to this specific feature: do you happen to have a real-world example where this feature is needed (see #73)?

(Side note: I've added a CONTRIBUTING.md file to the repo that asks potential future contributors to open an issue first before opening a PR. Not that there is any harm in discussing things here in the PR, but I'd like to avoid people spending their free time on things that might not be merged later on)

@Detegr
Copy link
Contributor Author

Detegr commented Oct 9, 2017

I get your points and I won't be offended if you decide to reject this PR. I like fd and this feature is useful for me, so I would have used the time to implement it any case.

I do agree that this kind of programs need to be as efficient as possible, the if-clause in the hot path is the best I could think of. I think branch prediction will work quite well here, so as you said, the performance hit is probably negligible. Maybe after #36 we could have some hard data on this.

About the code complexity, I agree that the current code is not very complex, though I think it could be more readable/less complex/easier to maintain by splitting the large main file to a smaller components. I think this feature is quite okay in terms of code complexity vs usefulness of fd, but I understand if you want to avoid feature creep and reject the feature.

We could experiment with showing some options in the short -h text and showing all options in the long --help text.

I like this idea, we just need to make it explicit in -h help that --help includes more features.

Sorry for the long discussion, but I'm also interested in your view concerning these points.
Coming back to this specific feature: do you happen to have a real-world example where this feature is needed (see #73)?

Discussion is fine, no worries :)
On my desktop machine I have a couple of backup drives mounted in my home directory in ~/backup and ~/backup2 and I don't want those to be searched while searching the home directory. However I think that's not the best real-world scenario for the README, simpler one would be to have an imaginary sd-card mounted under the example directory with few files to keep the file list small.

@tmccombs
Copy link
Collaborator

tmccombs commented Oct 9, 2017

Given the use cases, I think it would make more sense to add a more general --exclude option (and maybe a matching --include option).

As a workaround with the current fd, you could put a .ignore file in your home directory that ignores the backup and backup2 directories.

@Detegr
Copy link
Contributor Author

Detegr commented Oct 9, 2017

Given the use cases, I think it would make more sense to add a more general --exclude option (and maybe a matching --include option).

Now that you brought this up I think this is indeed a nice way to handle this with more added value than just skipping the mountpoints.

@sharkdp
Copy link
Owner

sharkdp commented Oct 9, 2017

Now that you brought this up I think this is indeed a nice way to handle this with more added value than just skipping the mountpoints.

If you like this better, it would be great if someone could write up a proposal as a GitHub issue. I'm not exactly sure how this would look like, yet.

@sharkdp
Copy link
Owner

sharkdp commented Oct 10, 2017

Just for reference - the proposal by @tmccombs: #89 (thanks!)

@sharkdp sharkdp mentioned this pull request Oct 13, 2017
@Detegr
Copy link
Contributor Author

Detegr commented Oct 21, 2017

Doh, I used master branch for this PR and now I used it again for another. I think I'll close this out as we're discussing about better alternatives already.

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