Improve host completion

[calestyo]

Hey @junegunn.

This would fix #3450 and incorporates a number of improvements and ideas:

• IMO, __fzf_list_hosts() should be considered to be a function for user customisation, for which we provide however some reasonable default.
  This includes, that it’s up to that function to sort the host list it produces (if desired so) and the remove duplicates, if any.
• To keep things simple for users, I moved out the username@ prefix handling from it into _fzf_complete_ssh() in c64fe9d.
• fbe4c54, makes completion.bash to use bash-completion’s _known_hosts_real(), if available, and falls back to the previous code for that, if not.

Some open points:

• For completion.bash, shall we remove the old host-list code altogether and only use _known_hosts_real()? I guess probably not… we need the same code anyway for zsh, and in principle fzf’s completion stuff can be used without bash-completion (though I doubt many do).
• I'd like to provide an easy way for users to override the sorting of the lists generated by __fzf_list_hosts(), so that one could make it easily configurable, whether e.g. natural sort is used or not, or easily hook in something that does what I've described here or perhaps a sorting that places all IPv4/6 addresses in the end, etc. pp..
  What do you think about introducing e.g. FZF_COMPLETION_HOSTNAME_SORT_CMD, which I'd then default to sort -u --version-sort?
• Possible side effects of invoking _known_hosts_real().
  I went through it’s code, and if I didn’t miss anything, the only side effect is that it sets COMPREPLY, which I therefore made local. But of course, any bash-completion version may change that.
  Another approach would be to invoke _known_hosts_real and the printf in a ( … ) subshell. Would you prefer that?

[calestyo]

If you'd agree to the idea with FZF_COMPLETION_HOSTNAME_SORT_CMD we could provide an example file or so, where some common example commands for various sorts and/or filters are given.

Perhaps one should also name it rather FZF_COMPLETION_HOSTNAME_POSTPROCESS_CMD, which would reflect better, that such a command could also be used to e.g. filter IPv4 and/or 6 address literals.

[calestyo]

Last force push was a minor update to one of the commits.
I now set COMPREPLY=() rather than =''.

I think with the way bash implements local this shouldn’t matter, as the variable is created completely new (and not initialised by an outer-scope one of the same name).

Still COMPREPLY is an array so it feels better to initialise it with =().

[calestyo]

Perhaps one should also name it rather FZF_COMPLETION_HOSTNAME_POSTPROCESS_CMD, which would reflect better, that such a command could also be used to e.g. filter IPv4 and/or 6 address literals.

Maybe that could/should be further extended:

I saw #2123, and if I understand the reporter’s problem correctly (see #2123 (comment)), it could be solved by providing e.g.:

• FZF_COMPLETION_HOSTNAME_POSTPROCESS_CMD
• FZF_COMPLETION_SSH_HOSTNAME_POSTPROCESS_CMD
• and if needed more like FZF_COMPLETION_<use-case>_HOSTNAME_POSTPROCESS_CMD

I think it wouldn't be too hard to actually do that.

The default for any FZF_COMPLETION_<use-case>_HOSTNAME_POSTPROCESS_CMD could be to use the value of FZF_COMPLETION_HOSTNAME_POSTPROCESS_CMD.

That means i a user just wants to some sorting (as proposed above), he'd only need t set the command for that in FZF_COMPLETION_HOSTNAME_POSTPROCESS_CMD, and all others would "inherit" it automatically.

But if a user wants to e.g. filter hosts based on what's being completed for, e.g. ssh, he could e.g. do:

FZF_COMPLETION_SSH_HOSTNAME_POSTPROCESS_CMD="grep -v '^[A-Z]' | someSorting"

or even:

FZF_COMPLETION_SSH_HOSTNAME_POSTPROCESS_CMD="grep -v '^[A-Z]' | eval \"\$FZF_COMPLETION_HOSTNAME_POSTPROCESS_CMD\""

[calestyo]

Found a small bug in the version for ZSH, which doesn't have the same declare -F as bash.

Now the tests run through, though locally I still have flaky tests (namely TestGoFZF#test_kill_default_command_on_abort, TestGoFZF#test_history and TestGoFZF#test_kill_reload_command_on_abort fail every now and then).

[calestyo]

btw: The idea of a convenience script that I had over in #3459 (comment) might also be of use here:

We could simply ship a copy (which we update from time 2 time) of bash-completions’ _known_hosts_real() (which, at least as of my Debian version of bash-completion would require tree further functions _included_ssh_config_files, __expand_tilde_by_ref, __ltrim_colon_completions).

Unfortunately, they don't seem to work out of the box for zsh.

[calestyo]

Rebased that onto all the recent changes in master, and also added missing command s to (hopefully) all places of the commits.

[calestyo]

@junegunn Any thoughts about this whole series?

[junegunn]

I'm fine with allowing advanced users to override __fzf_list_hosts, but I feel strongly that anything beyond that is unnecessary.

__fzf_list_hosts and _fzf_complete_ssh are just 20 lines of code as of now. Advanced users can easily read the source code and define their own completion function with unlimited freedom. It's faster to just copy the code and change it as they like than to figure out what FZF_SOMETHING_SOMETHING is and how to use it and think inside that box.

[junegunn]

Merged, thanks!

[junegunn]

This is fine for now. But I'll have to reconsider this if they decide to change the way the function works in the future.

[calestyo]

I think they're consolidating their function naming right now, so me might need to adapt that soon to work with the new name and fall back to the old one... but that shouldn't be too hard.

Other than that... I'd rather not expect any deeper changes to what the function does, do you?

[junegunn]

I think they're consolidating their function naming right now, so me might need to adapt that soon to work with the new name and fall back to the old one... but that shouldn't be too hard.

Oh, that's a deal breaker for me. I don't want to ship a product that might not work because of a sudden change in an implicit dependency.

[junegunn]

I'm not familiar with their code, and I just assumed _known_hosts_real was one of their documented public API that is guaranteed to be backward-compatible. If that's not the case, I'm not going to make fzf depend on it.

[calestyo]

Well, bash-completion is not the C standard or so... I think it's reasonable to expect that they have some level of stability in their ABI (since people use it), but nothing is ever set fully in stone… not even POSIX and the likes.

I've also only noticed it a few days ago (when looking into that eval issue we had), that they're renaming their functions... and didn't really think any further that this might also affect _known_hosts_real(), but it does … in scop/bash-completion@d311921 they’ve renamed it to _comp_compgen_known_hosts().

But actually I think this consolidation is quite reasonable for them to do… the many different names and prefixes they've started to use over time were much more messy and likely that people accidentally use such names for something else.

I've already mention that earlier here, I think hat fzf might also benefit from such systematic consolidation (e.g. names like FZF_CTRL_T_COMMAND are pretty vague (an ideally the keybinding should be configurable and not fixed to Ctrl-T).

It may even make sense for fzf to integrate in the naming schema used by bash-completion to some extent... e.g. if we'd always use _comp_fzf_* or so for functions.

Anyway I don't see the big deal breaker here?!

It's not that they'd do this every year or so... and that consolidation they do seems to rather make this stable/official now.

APIs do change, or the code is either dead are - less likely - already perfect.

Especially, it's trivially easy to support both (see #3476).

Also, there's already much more complex to read code, that we have now in order to support different versions. The mawk-version check or the code to support old bash versions.

Even if we would start using more bash-completion functions (e.g. also for the eval thing), I'd guess their total number would be rather limited, so I think this is pretty manageable.

Similarly, even if I could persuade you to e.g. rename FZF_CTRL_T_COMMAND and the likes (which I guess might be a tough job to do ;-) ), it would be easy to fall back to the old names, and e.g. print deprecation warnings for a while.

[calestyo]

I'm fine with allowing advanced users to override __fzf_list_hosts, but I feel strongly that anything beyond that is unnecessary.
__fzf_list_hosts and _fzf_complete_ssh are just 20 lines of code as of now. Advanced users can easily read the source code and define their own completion function with unlimited freedom. It's faster to just copy the code and change it as they like than to figure out what FZF_SOMETHING_SOMETHING is and how to use it and think inside that box.

I spent quite some time thinking recently, how fzf's shell/completion integration could be made more generic & powerful and at the same time be simpler for "us" (I mean the fzf side) to maintain.

Haven't fully sorted my thoughts yet but sooner or later I'll write it all up and post an RFC here.