Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection

Gene Goykhman <[email protected]> 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.