[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Provide completion candidates for remote containers over a T
From: |
Michael Albinus |
Subject: |
Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection |
Date: |
Wed, 23 Aug 2023 16:05:25 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Gene Goykhman <gene@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.
- [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Gene Goykhman, 2023/08/22
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection,
Michael Albinus <=
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Gene Goykhman, 2023/08/27
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Michael Albinus, 2023/08/28
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Gene Goykhman, 2023/08/28
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Michael Albinus, 2023/08/29
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Gene Goykhman, 2023/08/29
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Michael Albinus, 2023/08/31
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Gene Goykhman, 2023/08/31
- Re: [PATCH] Provide completion candidates for remote containers over a TRAMP connection, Michael Albinus, 2023/08/31