romkatv / zsh4humans

A turnkey configuration for Zsh
MIT License
1.67k stars 108 forks source link

:z4h:fzf-complete recurse-dirs interfers with -g and -/ argument to _files completion #209

Open VorpalBlade opened 2 years ago

VorpalBlade commented 2 years ago

When using the _files completer (either directly or as a sub-completer to for example _argument), the -g and -/ flags to restrict to a glob or directories only doesn't work if :z4h:fzf-complete recurse-dirs is set to yes.

Strangely enough it DOES work if using the underlying _path_files directly, which seems to bypass the recurse-dirs style.

Example for reproducing:

$ cd ~
$ zstyle :z4h:fzf-complete recurse-dirs yes
$ touch a.foo b.foo
$ compdef '_files -g "*.foo"' blergh
$ blergh <tab> # Notice that it completes lots of things recursively in your home directory that do not end in .foo
$ zstyle :z4h:fzf-complete recurse-dirs no
$ blergh <tab> # Now it only completes *.foo, though of course only in the current directory.

The same can be observed with -/ which should only complete directories. I have not tested the other flags to _files, it is possible they are also bugged.

VorpalBlade commented 2 years ago

With some further investigation it seems that the filter is correctly applied for files in the current directory but not for "recursive results" (that is: files or directories in sub-directories)

Also: having poked around in the code for a bit, I'm going to guess that either something is up with the call to -z4h-find in -z4h-comp-files or with -z4h-find itself.

Also also: as -z4h-find depends on either find or bfs I tried installing bfs to see if it would make a difference. Unless some cache needs to be clearer (by something more than just opening a new shell), it did not make a difference.

There is also a relevant TODO in _path_files in z4h-fzf-complete. As _path_files seems to work I'm slightly confused as to why it would appear to work properly.

romkatv commented 2 years ago

See: https://github.com/romkatv/zsh4humans/blob/df5c473d344124f74f3c6a36825f460d4aef97b6/fn/z4h-fzf-complete#L85-L89

It's possible to fix that TODO but it's not trivial.

If you need to complete directories, use _directories instead of _files -/.

romkatv commented 2 years ago

Also also: as -z4h-find depends on either find or bfs I tried installing bfs to see if it would make a difference. Unless some cache needs to be clearer (by something more than just opening a new shell), it did not make a difference.

Installing bfs changes the order of elements in recursive traversal. It becomes BFS instead of DFS -- a pretty big improvement in usability, so I highly recommend installing bfs if you can.

romkatv commented 2 years ago

Unrelated to this issue but might be relevant to you in general: https://github.com/romkatv/zsh4humans/issues/35#issuecomment-1027193022 and https://github.com/romkatv/zsh4humans/issues/35#issuecomment-1046205630

VorpalBlade commented 2 years ago

Installing bfs changes the order of elements in recursive traversal. It becomes BFS instead of DFS -- a pretty big improvement in usability, so I highly recommend installing bfs if you can.

Cool! Are there any other optional dependencies of z4h? Since it is mostly undocumented I assume yes, but it would still be nice to know still.

It's possible to fix that TODO but it's not trivial.

Hm, if and when I get time I might give that a go. I assume the general idea would be to extract the patterns & ignores into variables that are then used in -z4h-comp-files to limit what files are found by passing the relevant flags to -z4h-find and then bfs/find in turn.

If you need to complete directories, use _directories instead of _files -/.

Okay, it isn't documented in man zshcompsys and many of the bundled completions in zsh and zsh-completions already use _files -/. As do many of the completions bundled by various system packages:

$ grep -RF '_files -/' /usr/share/zsh | wc -l
474
$ grep -RF '_directories' /usr/share/zsh | wc -l
308

It would be nice if those worked as is. Also _directories is normally implemented in terms of _files (haven't checked if z4h overrides that):

$ cat  /usr/share/zsh/functions/Completion/Unix/_directories
#compdef dircmp -P -value-,*path,-default-

local expl

_wanted directories expl directory _files -/ "$@" -
romkatv commented 2 years ago

Cool! Are there any other optional dependencies of z4h? Since it is mostly undocumented I assume yes, but it would still be nice to know still.

Other optional dependencies are tmux and git but those are a lot more obvious.

It's possible to fix that TODO but it's not trivial.

Hm, if and when I get time I might give that a go. I assume the general idea would be to extract the patterns & ignores into variables that are then used in -z4h-comp-files to limit what files are found by passing the relevant flags to -z4h-find and then bfs/find in turn.

Applying pats and ignore patterns would need to be done after find/bfs because zsh patterns cannot in general be converted to find command line arguments. There is already a zsh process that goes over find output before passing it on to fzf and that code is very heavily optimized due to its being the bottleneck in recursive completions. Any pattern-based filtering will need to go there and will need to be heavily optimized, too. The code I'm referring to is in fn/-z4h-present-files.

It would be nice if those worked as is.

This is fixed.

VorpalBlade commented 2 years ago

[...] zsh patterns cannot in general be converted to find command line arguments [...]

Would it be feasible/worth it trying to detect when it is possible and do a fast-path using find/bfs flags and a slow path when it isn't possible? Looking at the uses in /usr/share/zsh, in many cases -g is simply used to filter on file extensions, and can be converted to either -name or -iname.

There are absolutely some more complex cases though. Also, ignores seem more difficult in general.

romkatv commented 2 years ago

[...] zsh patterns cannot in general be converted to find command line arguments [...]

Would it be feasible/worth it trying to detect when it is possible and do a fast-path using find/bfs flags and a slow path when it isn't possible?

Yes, definitely. It would also be OK to handle common cases only in the fast path. It might even be better (fast completions with too many options can be preferred to slow completing). That's also how I handled _files -/ -- by special-casing some -g arguments.

VorpalBlade commented 2 years ago

Another thought about optimisation (this time for ignore patterns): Looking at -z4h-present-files it seems to handle one file/directory result at a time. For handling ignores (as long as it is possible to convert, I haven't really looked into the gory details of this yet) it seems to me like it would be better to handle it earlier, even using something like find ... | grep -Ev "pattern". Some rather crafty anchoring rules might be required. Alternatively awk or sed could be used to split on the last component and then match on that.

However, premature optimisation etc... First implement. Then profile. Then optimise if needed. Not sure if or when I might get around to trying my hands at that.

romkatv commented 2 years ago

Another thought about optimisation (this time for ignore patterns): Looking at -z4h-present-files it seems to handle one file/directory result at a time.

The common case is vectorized. This is the code: https://github.com/romkatv/zsh4humans/blob/b7fe7f45cd68616ecf813e97bb2a4c754474148b/fn/-z4h-present-files#L123-L146

It processes all non-symlinks as a vector and only symlinks are processed one by one.

For handling ignores (as long as it is possible to convert, I haven't really looked into the gory details of this yet) it seems to me like it would be better to handle it earlier, even using something like find ... | grep -Ev "pattern".

We are on the same page.