Michael Albinus <michael.albi...@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:in...@line5.timetiger.com|sudo:line5.timetiger.com|docker:
My original patch produces this value of tramp--last-hop-directory:
#("/sudo:r...@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:in...@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!
