Re: [PATCH] * tramp-sh.el (tramp-optimize-remote-path): Add it.

[Andrey Portnoy]

Michael Albinus <michael.albinus@gmx.de> writes:

>> 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.
>
>> ...
>
> 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
>
> :type '(choice
>         (const :tag "Use remote settings" nil)
>         (repeat (choice ...
>
>
>> ...
>
> And we have a further problem. We recommend to change this user option
> by doing something like
>
> (add-to-list 'tramp-remote-path "/usr/local/perl/bin")
>
> 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.

I've thought more about this and I can't shake the feeling that "nil
tramp-remote-path = '(tramp-own-remote-path) with no checks" will be too
confusing.

As you say above, we'd be overloading nil tramp-remote-path to mean "use
the remote path set by the remote host with no checks" on top of the
existing meaning "make the 'base' remote path empty".

How about the following:

1. We introduce a new variable tramp-optimize-remote-path with a default
   value of t for backward compatibility (same as in my original patch).

2. We add a "fast path" to tramp-set-remote-path that skips the whole
   "get/optimize/set" process when tramp-optimize-remote-path is non-nil
   and tramp-remote-path is '(tramp-own-remote-path).
   Alternatively, we keep the original behavior of tramp-set-remote-path
   but add a new function tramp-maybe-set-remote-path that invokes
   tramp-set-remote-path when we can't take the "fast path" as described
   above.

This would allow to very explicitly express "'(tramp-own-remote-path)
with no checks" by setting tramp-remote-path to '(tramp-own-remote-path)
and by setting tramp-optimize-remote-path to nil. I have a feeling it's
better to transfer control to the user by giving them an explicit option
rather than introduce a tricky implicit behavior.

I understand the value of keeping the API surface as small as possible,
but in this case it feels worth it to expand it.

Some users might want the ability to disable the path optimizations in
cases other than '(tramp-own-remote-path), so it seems good to give them
that option separately.

I'm attaching two patches sketching out the alternatives in paragraph
(2) above.

What do you think?

0001-Use-tramp-optimize-remote-path-inside-tramp-set-remo.patch
Description: update tramp-set-remote-path

0001-Use-tramp-optimize-remote-path-in-a-separate-functio.patch
Description: add tramp-maybe-set-remote-path