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

[Gene Goykhman]

Michael Albinus <michael.albinus@gmx.de> 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:infra@line5.timetiger.com|sudo:line5.timetiger.com|docker:

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

#("/sudo:root@line5.timetiger.com:/" 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:infra@line5.timetiger.com|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!