Re: tramp (2.6.1 HEAD/3ff676c2f98cb6c47fecb37f31a589a910dd3876); docker-container ssh multi-hop support

[Gene Goykhman]

Hi Michael,

> Sorry for the delay, but bug fixing has priority ...

Of course!

> However, this would require to assign the copyright of the code to the
> FSF, as all non-trivial contributions to Emacs require. Would you be
> willing to sign this?

Of course, I would be happy to.

> I guess we don't need to advice 
> tramp-completion-handle-file-name-all-completions.
> The code should be added directly there.

Yes that makes sense.

> Well, you disable this for all completions. I don't know whether we want
> this. It is useful for the very first hop, and not only for the docker
> method, but for all methods.

I agree, and it doesn't seem that TRAMP currently allows that level of 
granularity for disabling caching. It does seem out of scope for this 
particular enhancement.

> >   (when-let ((default-directory (if tramp-last-hop-directory
> >                                     tramp-last-hop-directory
> >                                   tramp-compat-temporary-file-directory))

> I would write (or tramp-last-hop-directory 
> tramp-compat-temporary-file-directory)

Yes, thank you. I am still learning Emacs Lisp :)

> > (add-to-list 'tramp-completion-function-alist
> >              `("docker"
> >                (tramp-container--completion-function ,tramp-docker-program)
> >                (my-tramp-container-completion-function 
> > ,tramp-docker-program)))

> So you have two entries. Do you still need 
> tramp-container--completion-function?

I wasn't sure whether it was important to keep the original function in the 
list.

> > (defun my-tramp-docker-host-path (orig)
> >   ;; Strips off "|docker" suffix from ORIG allowing
> >   ;; it to be converted to a tramp-file using TRAMP's path functions.
> >   (let ((regexp "\\(.*\\)|docker"))
> >     (if (string-match regexp orig)
> >         (concat (match-string 1 orig) ":/")
> >       orig)))

> Well, this is docker related. The proper Tramp functions would be

> (tramp-make-tramp-file-name (tramp-dissect-hop-name
> (tramp-file-name-hop (tramp-dissect-file-name FILENAME))))

> Hmm, perhaps this is worth an own function. Or perhaps we have it
> already, and I don't remember. We have a similar logic in computing the
> previous hop in function tramp-compute-multi-hops, but not in the same
> sense as we need it here.

> And this also does not handle the case of more than two hops, like in
> "/ssh:host1|ssh:host2|docker:container::"

Yes I have to defer to your judgement here. It was a very simple attempt that 
worked for my particular use-case.

> As said, I propose to integrate it in 
> tramp-completion-handle-file-name-all-completions.
> And I don't understand, why you call (tramp-make-tramp-file-name 
> (tramp-dissect-file-name ...))
> my-host-path is already a proper file name, isn't it?

They are slightly different. For example, testing in my sutation I have this 
my-host-path:

  /ssh:infra@line5.timetiger.com|sudo:line5.timetiger.com:/

But after the tramp-make-tramp-file-name transformation it is:

  /ssh:infra@line5.timetiger.com|sudo:root@line5.timetiger.com:/

I think the additional username after sudo: is needed in order to execute 
docker on the remote host. I'm not sure if this is the best way to accomplish 
this.