Remote shell support

[montag451]

This is a pull request that add remote shell support to emacs-bash-completion, this commit should close #8. I tried to minimize the number of changes and to keep the behaviour of emacs-bash-completion when no remote shell is used. Some functions have been added but the most important one for me is bash-completion-dynamic-complete-fast which use caching to improve performance (this is noticeable when using a remote shell) but which don't try wordbreak completion (I didn't find a way to conciliate caching and wordbreak completion). I also introduced a new option bash-completion-default-completion which enable/disable Readline default completion when a compsec return no matches. Feel free to ask me about the implementation.

NB: I can squash all the commits if you prefer.

[szermatt]

This is a great feature ! Thank you ! I'm sure it'll be very useful to people using tramp.

Everything looks like it's working and the test all passes, even on older versions of emacs :) I haven't testing it very thoroughly yet. I'd like to give it a spin for a little while before integrating; some people like to use the version at head.

Some comments:

bash-completion-dynamic-complete-fast does this have to be a separate method? Could it be an a customisable option that enables caching - and, for now, disables word break completion. That would be easier to discover and use. Caching is a generally useful feature and I'd love to make it the default, eventually. Also, I have to say that I have trouble keeping in mind the subtle difference between bash-completion-dynamic-complete-nocomint and bash-completion--dynamic-complete-nocomint... Some well, placed if's would be easier to follow.

What's the motivation behind the introduction of bash-completion-default-completion ? Is it linked to tramp, or is there something else? (I have nothing against it, I'd just like to understand, because I know the handling isn't quite correct: it should be configurable per command.)

[montag451]

Thank you for you feedback. I will try to answer all your comments:

bash-completion-dynamic-complete-fast does this have to be a separate method? Could it be an a customisable option that enables caching - and, for now, disables word break completion. That would be easier to discover and use. Caching is a generally useful feature and I'd love to make it the default, eventually.

Yes you are right, it would be better to make this customizable. Do you have any idea regarding the name of this new option?

Also, I have to say that I have trouble keeping in mind the subtle difference between bash-completion-dynamic-complete-nocomint and bash-completion--dynamic-complete-nocomint... Some well, placed if's would be easier to follow.

Yes this part of the implementation is confusing and maybe bash-completion--dynamic-complete-nocomint should be renamed. I will try to explain why I created this function. bash-completion--dynamic-complete-nocomint (which is called by bash-completion-dynamic-complete-fast and by bash-completion-dynamic-complete-nocomint) returns a list which contains the stub start, the stub end and a function which responsible for doing the completion. The fact that a function is returned instead of the full list of possible completions enables two things:

• laziness: the function is called when the completions list is really needed (see the documentation of completion-at-point-functions to see why it's important)
• caching (because completion-table-with-cache is used instead of completion-table-dynamic)

These two points are crucial to get good performance when using a remote shell. The downside of returning a function instead of the list of completions is that it's not possible anymore to know if the completion will return matches or not and so it's not possible to try wordbreak completion when no matches are returned. bash-completion-dynamic-complete-nocomint calls bash-completion--dynamic-complete-nocomint and then retrieve the whole list of possible completion (by calling all-completions). If the list is empty, it tries wordbreak completion. The behaviour is the same as the old function (it returns the stub start, the stub end and the list of possible completions, not a function) and I didn't want to change it because people might be using it to do completion with other shell (e.g eshell) and almost all the tests use it. So bash-completion-dynamic-complete-nocomint is here to maintain backward compatibility (but it can be slow when using remote shell).

What's the motivation behind the introduction of bash-completion-default-completion ? Is it linked to tramp, or is there something else? (I have nothing against it, I'd just like to understand, because I know the handling isn't quite correct: it should be configurable per command.)

In fact this new option has nothing to do with the introduction of remote shell support, it's just that I was too lazy to create another pull request :grin: I think I will remove the commit introducing this new option from this pull request and I will open a new one. Now, why I introduced this option? Because for some command it's normal to not return any matches sometimes. For example, cd only complete directories and when the completion returns nothing it means that there is only files in the directory being completed and it's annoying that in such situation the default Readline completion is called (and in the case of cd, it's wrong). As you said, this behaviour should depend on the command being completed (if it was me I would completely remove this part of the code :grinning:).

[montag451]

I created a new pull request (#29) for the introduction of bash-completion-default-completion and I removed the commit from this pull request

[szermatt]

Here's the what I find puzzling and what I meant by "adding an if":

(defun bash-completion-dynamic-complete ()
  (bash-completion--dynamic-complete #'bash-completion-dynamic-complete-nocomint))

(defun bash-completion-dynamic-complete-fast ()
  (bash-completion--dynamic-complete #'bash-completion--dynamic-complete-nocomint))

(defun bash-completion--dynamic-complete (comp-fun)
  [...]
  (funcall comp-fun (comint-line-beginning-position)

(defun bash-completion-dynamic-complete-nocomint (comp-start comp-pos)
  (let* ((res (bash-completion--dynamic-complete-nocomint comp-start comp-pos))
            [...]
           (table (nth 2 res)))
      [...](completions (all-completions ... table))[...]wordbreak)

(defun bash-completion--dynamic-complete-nocomint (comp-start comp-pos)

Why is the -fast variant needed ?

Is that really different from:

(defun bash-completion--dynamic-complete ()
  [...]
  (bash-completion-dynamic-complete-nocomint (comint-line-beginning-position)

(defun bash-completion-dynamic-complete-nocomint (comp-start comp-pos)
  (let ((res (bash-completion--dynamic-complete-nocomint comp-start comp-pos))
      (if some-customizable-option 
            res
          [...]
          (table (nth 2 res)) [...](completions (all-completions ... table))[...]wordbreak[...]))

The second point is the name of bash-completion--dynamic-complete-nocomint. It is confusing, because it's the same name as bash-completion-dynamic-complete-nocomint. It need either a better name. Does it do something more specific than bash-completion-dynamic-complete-nocomin? I personally would merge bash-completion--dynamic-complete-nocomint into bash-completion-dynamic-complete-nocomin and instead extract the part that does word break completion into its own function (bash-completion--wordbreak-completion ?), called conditionally.

Finally, I would prefer to start with caching being conditional, so some-customisable-option could be bash-completion-enable-caching, which, if enabled, would automatically disable wordbreak completion.

We end up with something like:

(defun bash-completion-dynamic-complete-nocomint (comp-start comp-pos)
  (let ((res (bash-completion--dynamic-complete-nocomint comp-start comp-pos))
      (if bash-completion-enable-caching  
            res
          [...]
          (table (nth 2 res)) [...](completions (all-completions ... table))[...]wordbreak[...]))

[szermatt]

Thanks for the separate pull request! I merged it into the main branch right away, since it's safe and simple.

[montag451]

I have made the changes you requested and introduced a new option bash-completion-enable-caching. Is it fine for you? Before you merge my changes (if you are happy with them :smiley:), I would like to squash all the commit into one.

[szermatt]

It looks good. Thank you for making the changes! I've been using this version without issue so far. I'll switch to the latest one.

I'm planning to merge it this week-end, after deprecating support for older versions of emacs. Please let me know when you're ready.

[montag451]

It's OK for me, I have squashed all the commits and rebased against your master branch. It should be a fast-forward merge. Thanks for your comments, I ended up with a clearer code!