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

Michael Albinus <[email protected]> wrote:

> I've seen your name on file at the FSF. Congratulations!

Thank you! I'm excited to contribute.

I've been incorporating your suggestions and have some follow-up questions ...

> 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.

I've removed the autoload and renamed it to tramp--last-hop-directory to indicate that it is internal to TRAMP and should not be set manually. I've also added a docstring.

> > +(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:
> ...
> - 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:

I'm not sure how best to handle the lifetime of tramp--last-hop-directory. It needs to be explicity set to nil in tramp-completion-handle-file-name-all-completions UNLESS we're completing a multi-hop path, and only in that case should it be set to the parsed last hop directory.

That value is then consumed later by tramp-container--completion-function. The value remains set after use however, so it is important that it is subsequently reset to nil the next time tramp-completion-handle-file-name-all-completions is called, otherwise we could end up with a stale value of tramp--last-hop-directory for a subsequent single-hop TRAMP connection.

If you can suggest a better way to handle this I'd appreciate it.

> Replace this by

>       (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)))))

I tried this but ended up with a tramp--last-hop-directory that I wasn't able to use to execute the container executable remotely.

Here is a specific example. For the path:

/ssh:[email protected]|sudo:line5.timetiger.com|docker:

My original patch produces this value of tramp--last-hop-directory:

#("/sudo:[email protected]:/" 6 10
  (tramp-default t))

When I set this as the default-directory, I am able to run the container program on the remote host and obtain the list of completion candidates (running containers).

After incorporating your suggested changes, the value of tramp--last-hop-directory becomes:

"/ssh:[email protected]|sudo:line5.timetiger.com:"

Setting this as the default-directory, I am /not/ able to run the container program (TRAMP seems to hang, at least on my macOS system).

I think that, at least on my system, the root@ is required, and I think the multi-hop path also doesn't work as a default-directory.

> - 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.

Yes absolutely true. In my testing, because I am entering the path hop by hop, the connections are being created as I go so it works well.

Thanks for your help so far!