"Andrey Portnoy" <aport...@fastmail.com> writes: Hi Andrey,
> BTW initially I thought the right thing would be to make the path > optimizations opt-in rather than opt-out, i.e. make the default value of > tramp-optimize-remote-path in my old patch nil rather than t. > > After learning that the logic to remove non-existing directories dates > back to 1999 I wasn't so sure :) > > https://github.com/emacsmirror/tramp/commit/420a902d > > Do you know if Tramp has these optimizations for any reason other than > the obvious one that it makes remote program search faster? Ar this time, tramp-remote-path didn't exist yet, so we don't know the usual values of DIRLIST rcp-set-remote-path is called with. But if you see tramp-remote-path's initial value, there's something like "/local/freeware/bin", which doesn't make sense for most of the uses. I wasn't part of the Tramp team in 1999 (I've entered 2002), so I cannot know the reasons. I had always the feeling this was also due to secutiry concerns, but I cannot prove this. I've never spoken with Kai about. > Please see the attached patch. I think it's as simple as that, let me > know if I'm missing anything. Thanks! > Wasn't sure if this warrants an entry in etc/NEWS for Emacs. Definitely. It is a user visible change, which must be documented in etc/NEWS. We'll need also something in tramp-tests.el, but this can wait. > diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el > index 9e813ecc..ba62912b 100644 > --- a/lisp/tramp-sh.el > +++ b/lisp/tramp-sh.el > @@ -4511,7 +4511,8 @@ process to set up. VEC specifies the connection." > (if (string-match-p (rx (| "FreeBSD" "DragonFly")) uname) 500 0)))) > > ;; Set remote PATH variable. > - (tramp-set-remote-path vec) > + (if tramp-remote-path > + (tramp-set-remote-path vec)) I believe this check shouldn't be here, but in tramp-set-remote-path. There are *ELPA packages in the wild which use internal Tramp functions, we shall still support them. I also believe we must adapt tramp-get-remote-path, but I haven't checked in detail. > diff --git a/lisp/tramp.el b/lisp/tramp.el > index 19737bb5..c56e14de 100644 > --- a/lisp/tramp.el > +++ b/lisp/tramp.el > @@ -1428,6 +1428,15 @@ special value `tramp-default-remote-path'. > as given in your `~/.profile'. This entry is represented in > the list by the special value `tramp-own-remote-path'. > > +After `tramp-default-remote-path' and `tramp-own-remote-path' are > +expanded according to their meaning, Tramp deduplicates the list and > +removes entries corresponding to non-existing directories (those not > +found on the remote host by Tramp). > + > +Setting this variable to nil prevents Tramp from modifying the remote ^^^^^^^^ user option > +PATH altogether and preserves the original value assigned by the remote > +host, even if duplicate or invalid entries are present. I would be more explicit. A value of nil means '(tramp-own-remote-path) implicitly, w/o any check. Furthermore, I believe we shall also be more explicit on the :type. Something like --8<---------------cut here---------------start------------->8--- :type '(choice (const :tag "Use remote settings" nil) (repeat (choice ... --8<---------------cut here---------------end--------------->8--- > diff --git a/texi/tramp.texi b/texi/tramp.texi > index 26be593e..d81d15d0 100644 > --- a/texi/tramp.texi > +++ b/texi/tramp.texi > @@ -2449,6 +2449,15 @@ preserves the path value, which can be used to update > > @strong{Note} that this works only if your remote @command{/bin/sh} > shell supports the login argument @samp{-l}. > + > +@strong{Note} that after special values > +@code{tramp-default-remote-path} and @code{tramp-own-remote-path} are > +expanded, Tramp removes duplicate and non-existing directories from > +the resulting list. > + > +To direct Tramp to use exactly the remote path assigned by the remote > +host without any modifications or optimizations, set this variable to > +@code{nil}. > @end defopt <nitpicking> Tramp is spelled out @value{tramp} in tramp.texi, for proper formatting. </nitpicking> We should be explicit here as well. nil means an implicit value of '(tramp-own-remote-path), w/o checks. And we have a further problem. We recommend to change this user option by doing something like --8<---------------cut here---------------start------------->8--- (add-to-list 'tramp-remote-path "/usr/local/perl/bin") --8<---------------cut here---------------end--------------->8--- This has the side effect, that in case tramp-remote-path is nil, its value changes from the implicit '(tramp-own-remote-path) to '("/usr/local/perl/bin"). Likely a surprise for users. We must document this trap, and we must also adapt the places in Tramp, where we use a similar construct ourselves, like in tramp-flatpak-connection-local-default-variables of tramp-container.el. That's all for now :-) Best regards, Michael.
