Gene Goykhman <g...@indigo1.com> writes: Hi Gene,
I've seen your name on file at the FSF. Congratulations! > Following up on > https://lists.gnu.org/archive/html/tramp-devel/2023-08/msg00001.html, I have > prepared a patch to add remote container completion support to TRAMP. Thanks for this! As usual, I have some comments. > A couple of questions/things I'm uncertain of in this patch: > > 1. I am removing the check for executable-find for the docker and > podman programs since they need to be run on the remote system (and > executable-find will fail as it only checks the local machine while > multi-hopping). I'm not sure if there is a better way to handle this. That's a problem, indeed. I'll think about a better solution, ATM we should do it as proposed by you. > 2. In tramp-set-completion-function I am short-circuiting the cond > check for a valid "file or registry key" since again, file-exists-p > will fail when checking for the docker or podman program on a remote > system when checking a multi-hop path. I'm open to a better ideas here > since my change effectively no-ops the whole cond. Same as above. > 3. (style question) I've added my required variable tramp-last-hop-directory > as an autoload in tramp.el but if there's a better place for it I'd be happy > to move it. No autoload is needed. tramp-container.el requires tramp.el, the variable is known therefore. > --- a/lisp/net/tramp-container.el > +++ b/lisp/net/tramp-container.el > @@ -157,6 +157,8 @@ If it is nil, the default context will be used." > (defconst tramp-flatpak-method "flatpak" > "Tramp method name to use to connect to Flatpak sandboxes.") > > +(defvar tramp-last-hop-directory) Not needed. > This function is used by `tramp-set-completion-function', please > see its function help for a description of the format." > - (when-let ((default-directory tramp-compat-temporary-file-directory) > + (when-let ((default-directory (or tramp-last-hop-directory > + tramp-compat-temporary-file-directory)) Please give it a verbose comment, that poor souls reading Tramp sources understand the trick. > diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el > index a0092a2d706..427c0871310 100644 > --- a/lisp/net/tramp.el > +++ b/lisp/net/tramp.el > @@ -82,6 +82,9 @@ > (defvar tramp-completion-method-regexp) > (defvar tramp-completion-file-name-regexp) > > +;;;###tramp-autoload > +(defvar tramp-last-hop-directory nil) > + No ;;;###tramp-autoload cookie needed. A docstring is missing, which explains the purpose. For example, that it shouldn't be set manually, but be let-bound. > @@ -2133,8 +2136,8 @@ Example: > ;; DNS-SD service type. > ((string-match-p > tramp-dns-sd-service-regexp (nth 1 (car v)))) > - ;; Configuration file or empty string. > - (t (file-exists-p (nth 1 (car v)))))) > + ;; Allow arbitrary executable name (for remote systems) > too. > + (t t))) Perhaps you mark it with a FIXME: comment. > +(defun container-host-directory (orig) > + "Strips off the `tramp-docker-program' or `tramp-podman-program' suffix > from ORIG." > + "Returned path can be used to start programs on the container host." > + (let ((regexp (format "\\(.*\\)|\\(\\(%s\\)\\|\\(%s\\)\\)" > tramp-docker-program tramp-podman-program))) > + (if (string-match regexp orig) > + (concat (match-string 1 orig) ":/") > + orig))) This function has several problems: - It hard-codes "docker" and "podman". We want to use the new functionality also for other methods. - It just replaces "docker" or "podman" whereever it is in the file name. What if you have "/docker:host|ssh:"? - It expects the default file name syntax, where the method is followed by a colon, like in "/ssh:host|docker:". We have also other Tramp syntaxes, like the separate syntax. The same file name to be completed would be there "/[ssh/host|docker/". - It doesn't allow docker host parts of the file name. What if a user wants to complete "/ssh:host|docker:ab", when she knows the target host starts with "ab"? - We don't need it at all :-) See below. > (defun tramp-completion-handle-file-name-all-completions (filename directory) > "Like `file-name-all-completions' for partial Tramp files." > + (let ((use-container-host-directory (container-host-directory directory))) > + (when (tramp-tramp-file-p use-container-host-directory) > + (let ((minibuffer-completing-file-name nil)) > + (setq tramp-last-hop-directory (tramp-make-tramp-file-name > + (tramp-dissect-file-name > use-container-host-directory)))))) No. Don't do that. Instead, bind tramp-last-hop-directory to nil in that let-clause: > (let ((fullname ... Furthermore, the hop is already evall'ed in the function. We have --8<---------------cut here---------------start------------->8--- (setq hop (match-string 1 fullname) fullname (replace-match "" nil nil fullname 1))) --8<---------------cut here---------------end--------------->8--- Replace this by --8<---------------cut here---------------start------------->8--- (setq hop (match-string 1 fullname) fullname (replace-match "" nil nil fullname 1) tramp-last-hop-directory (file-remote-p (tramp-make-tramp-file-name (tramp-dissect-hop-name hop))))) --8<---------------cut here---------------end--------------->8--- With these changes, it looks good, and the completion on the other host works. However, there are still open points: - Completion for the host name somewhere else but local takes time. People might dislike it. So perhaps we add a user option, where a user can decide, whether she wants this feature. - Completion of "/ssh:host|docker: TAB" works only, if there is already an etablished connection to "/ssh:host:". I'll contemplate how to simplify this. Best regards, Michael.
