-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Exclude macOS containers from default find commands #3647
Conversation
Specifically, exclude any file with the com.apple.containermanager.uuid extended attribute from the default Ctrl-T and Alt-C commands.
shell/key-bindings.bash
Outdated
local cmd opts macos_exclude | ||
if [[ "$(uname -s)" == "Darwin" ]]; then | ||
macos_exclude="-o -xattrname 'com.apple.containermanager.uuid'" | ||
fi | ||
cmd="${FZF_CTRL_T_COMMAND:-"command find -L . -mindepth 1 \\( -path '*/.*' -o -fstype 'sysfs' -o -fstype 'devfs' -o -fstype 'devtmpfs' -o -fstype 'proc' $macos_exclude \\) -prune \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about if we keep it simple and just put -xattrname 'com.apple.containermanager.uuid'
regardless of the platform? Will there be any negative consequences for non-macOS users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately yes. When find encounters a predicate it doesn't recognize it bails immediately and produces no results. Which actually brings up another issue: if someone on macOS has installed GNU find and deliberately placed it earlier on their PATH this command will similarly fail. On the one hand doing this is a "buyer beware" sort of situation but it's not that uncommon.
I can think of a few ways of dealing with this:
- Ignore it. Buyer beware.
- Instead of
uname
check iffind /dev/null -xattrname 'com.apple.containermanager.uuid' 2> /dev/null
succeeds. - Instead of
command find
use/usr/bin/find
.
2 is probably the least disruptive but is still adding complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, -xattrname
is not available on GNU find. It's unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and did number 2. I haven't been able to test this on fish yet but otherwise I think this should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Honestly, I don't know how I feel about this. Are these conditional branches which add complexity and degrade the readability worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm no longer sure. The fact that my most recent commit had a bug is a vote against it. I'll keep it open for a bit longer though.
Closed in favor of #3649 |
Specifically, exclude any file with the com.apple.containermanager.uuid extended attribute from the default Ctrl-T and Alt-C commands.
See additional discussion at #2705