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

feature request: host completion improvements #3450

Closed
5 of 10 tasks
calestyo opened this issue Sep 25, 2023 · 4 comments · Fixed by #3454
Closed
5 of 10 tasks

feature request: host completion improvements #3450

calestyo opened this issue Sep 25, 2023 · 4 comments · Fixed by #3454

Comments

@calestyo
Copy link
Contributor

  • I have read through the manual page (man fzf)
  • I have the latest version of fzf
  • I have searched through the existing issues

Info

  • OS
    • Linux
    • Mac OS X
    • Windows
    • Etc.
  • Shell
    • bash
    • zsh
    • fish

Hey.

I think the current host completion has a number of areas where it could be improved:

  1. It's not (easily) configurable, e.g. it would be nice to have a user switch that disables v4 and/or v6 addresses, and perhaps options that do sorting
  2. It lacks many names that are shown by bash-completion’s _known_hosts_real(), which is quite powerful and IIRC even checks ssh_config for other configured GlobalKnownHostsFile and UserKnownHostsFile. fzf’s current completion seems to ignore any host (which are more or less all of them) that I have in the GlobalKnownHostsFile. _known_hosts_real() also has many more features, I think it can optionally use Avahi and ruptime.

For (2), I think it would be best if fzf would simply use _known_hosts_real() - why re-inventing the wheel, if bash-completion guys already maintain that properly?

Based on the 0.42.0 fzf, I tried roughly the following:

_fzf_host_completion() {
  _fzf_complete +m -- "$@" < <(
        _known_hosts_real ''
        printf '%s\n' "${COMPREPLY[@]}" | sort -u -V
  ) 
} 

and at a first glance it seemed to work:

  • The sort-u is necessary as COMPREPLY contained many duplicates (no idea why).
  • -V is just for sorting ... ideally this would be configurable,... plus I'd like a sort mode that sort by domains[0].

Some open points:

  • Is _known_hosts_real() available for sure? I mean bash-completion does load completions for commands dynamically (i.e. only when the command is actually completed), but I'd blindly guess that _known_hosts_real() is always available and we don't need to "import" it first.
  • Do we have any side-effects? At least COMPREPLY is set. Haven't checked whether _known_hosts_real() uses other non-local variables. As long as the function is called within process substitution <(…) this shouldn't be a problem, but I've seen that in edfdcc8 you placed the printing code in a separate function, so if that would ever be called outside process substitution it might cause unwanted side effects.
  • What about the config env vars recognised by _known_hosts_real(), like COMP_KNOWN_HOSTS_WITH_AVAHI? Should we just use them directly, or clean them and initialise them only by our own e.g. FZF_KNOWN_HOSTS_WITH_AVAHI counterparts (so that people could configure bash-completion and fzf differently)?
@calestyo
Copy link
Contributor Author

calestyo commented Sep 25, 2023

[0] With sorting by domains I mean e.g.

9.aq
5.a.com
6.a.com
1.b.com

I.e. sorted reversely by each domain component, and within the same component one should again do alphabetical (or better natural) sort.

The advantage of such a sorting is, that host that (possibly) "belong together" are sorted next to each other

@junegunn
Copy link
Owner

Thanks for the suggestion, but before we discuss the topic in detail I have to mention that I'm not particularly looking to improve/expand the shell features (key bindings and fuzzy completion) we provide here. They are intentionally kept simple (with minimal configuration knobs)

  • to reduce the maintenance burden of keeping three implementations (bash, zsh, fish) in sync
  • to serve as the basic reference implementation with no bells and whistles for those who are trying to learn how to write fzf-powered shell functionality on their own.

I believe extended versions of the scripts should be maintained as separate projects.

https://github.com/Aloxaf/fzf-tab is a good example.

A separate project focused on a specific shell doesn't have the problem of maintaining the same functionality across three different shells, so it should be easier to add more advanced features. It's going to be used by advanced users who know what they're doing so the maintainer can experiment, break, and fix things with more confidence.

@calestyo
Copy link
Contributor Author

Thanks for the suggestion, but before we discuss the topic in detail I have to mention that I'm not particularly looking to improve/expand the shell features (key bindings and fuzzy completion) we provide here. They are intentionally kept simple (with minimal configuration knobs)

Well, while on one side I can see your points (especially keeping fzf itself clean and simple and also not having to add functionality for numerous shells at the same time), in practise this poses IMO several problems.

  • Generally it's very bad to distribute software outside some form of distribution or other package management system (no security support, no systematic way for regular updates, dependency mess, breaking any form up centralised management (checking for updates, automatic deplyoment, etc. pp.). While this seems to have become a new fashion, one should still try to avoid it as best as one can.
    If any such functionality is now put in 3rd party repos, there's in practise little chance that the respective code gets ever packaged, especially if there are many (like a separate fzf-* for every little thing). By keeping at least generic stuff in the main fzf project, platform that packages this, should at least sooner or later get the new functionality.
  • In practise, such 3rd party repos are also rather prone to get outdated or have other issues that are never fixed. This is even more likely when the 3rd party project would expand features that are already in the main fzf. Consider there was now another project that extends host completion by overwriting _fzf_host_completion() it's all too likely that sooner or later you make a change that doesn't work with their state anymore.

I think bash-completion might serve as a good model:

  • it tries to include base functionality of generic use
  • it ships the "plugins" for major programs or such of generic use, if the respective project doesn't ship the completions by itself
  • it still easily possible for some actual project (like git or) so to maintain and ship its own set of completions

Another possible way, that would help you to keep the fzf repo clean from all to many shell stuff, could be that you generally outsource all shell integration into separate fzf-<shell> repos.

I guess these would have at least some chance to get packaged, and there would no longer be so many possible areas of conflict between shell integration code shipped by the main fzf package and such from 3rd party project, since, that would be again in one repo.

That would be again similar to how things are handled in the bash world:

  • there's bash itself, just the shell - which would be fzf - just the fuzzy finder
  • bash-completion, generic completion infrastructure and support for some major programs - which would be fzf-bash, fzf-zsh, etc.
  • downstream projects like git shipping their own completions - which would be 3rd party fzf-something projects

What's your opinion on that?

Any beyond that, would you still accept code for the actual feature idea itself?

@calestyo
Copy link
Contributor Author

btw: (Your) current implementation of __fzf_list_hosts() also gives errors, if ~/.ssh/known_hosts or /etc/hosts doesn't exist. But TBH, I'd very much prefer to go the "use _known_hosts_real() way" proposed here, rather than fixing this. ;-)

So would be happy if you'd agree to get something like that merged.

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 a pull request may close this issue.

2 participants