Michael Albinus <michael.albi...@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?
>From f8b01ebdbde63d32299f7d0e3290b5b725c48652 Mon Sep 17 00:00:00 2001
From: Andrey Portnoy <aport...@fastmail.com>
Date: Tue, 1 Oct 2024 18:29:57 -0400
Subject: [PATCH] Use tramp-optimize-remote-path inside tramp-set-remote-path
---
lisp/tramp-sh.el | 56 +++++++++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 20 deletions(-)
diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
index 9e813ecc..1eb3f1c2 100644
--- a/lisp/tramp-sh.el
+++ b/lisp/tramp-sh.el
@@ -4129,6 +4129,9 @@ This function expects to be in the right *tramp* buffer."
(setq result (buffer-substring (point) (line-end-position)))))
result)))
+(defvar tramp-optimize-remote-path t
+ "Whether to remove duplicate and non-existing directories from remote PATH.")
+
;; On hydra.nixos.org, the $PATH environment variable is too long to
;; send it. This is likely not due to PATH_MAX, but PIPE_BUF. We
;; check it, and use a temporary file in case of. See Bug#33781.
@@ -4137,18 +4140,27 @@ This function expects to be in the right *tramp* buffer."
I.e., for each directory in `tramp-remote-path', it is tested
whether it exists and if so, it is added to the environment
variable PATH."
- (let ((command
- (format
- "PATH=%s && export PATH"
- (string-join (tramp-get-remote-path vec) ":")))
- (pipe-buf (tramp-get-remote-pipe-buf vec))
- tmpfile chunk chunksize)
- (tramp-message vec 5 "Setting $PATH environment variable")
- (if (length< command pipe-buf)
- (tramp-send-command vec command)
+ (let* ((should-set-remote-path ; skip setting remote path in some cases
+ (or tramp-optimize-remote-path
+ (not (equal tramp-remote-path '(tramp-own-remote-path)))))
+ (command
+ (and should-set-remote-path
+ (format
+ "PATH=%s && export PATH"
+ (string-join (tramp-get-remote-path vec) ":"))))
+ (pipe-buf (tramp-get-remote-pipe-buf vec))
+ tmpfile chunk chunksize)
+ (cond
+ ((not should-set-remote-path)
+ nil)
+ ((length< command pipe-buf)
+ (tramp-message vec 5 "Setting $PATH environment variable")
+ (tramp-send-command vec command))
+ (t
;; Use a temporary file. We cannot use `write-region' because
;; setting the remote path happens in the early connection
;; handshake, and not all external tools are determined yet.
+ (tramp-message vec 5 "Setting $PATH environment variable")
(setq command (concat command "\n")
tmpfile (tramp-make-tramp-temp-file vec))
(while (not (string-empty-p command))
@@ -5610,17 +5622,21 @@ Nonexistent directories are removed from spec."
(cdr elt2)))
(setq remote-path (delq 'tramp-own-remote-path remote-path)))
- ;; Remove double entries.
- (setq remote-path
- (cl-remove-duplicates
- remote-path :test #'string-equal :from-end t))
-
- ;; Remove non-existing directories.
- (let (remote-file-name-inhibit-cache)
- (tramp-bundle-read-file-names vec remote-path)
- (cl-remove-if
- (lambda (x) (not (tramp-get-file-property vec x "file-directory-p")))
- remote-path))))))
+ (when tramp-optimize-remote-path
+ ;; Remove double entries.
+ (setq remote-path
+ (cl-remove-duplicates
+ remote-path :test #'string-equal :from-end t))
+
+ ;; Remove non-existing directories.
+ (let (remote-file-name-inhibit-cache)
+ (tramp-bundle-read-file-names vec remote-path)
+ (setq remote-path
+ (cl-remove-if
+ (lambda (x)
+ (not (tramp-get-file-property vec x "file-directory-p")))
+ remote-path))))
+ remote-path))))
;; The PIPE_BUF in POSIX [1] can be as low as 512 [2]. Here are the values
;; on various platforms:
--
2.39.5 (Apple Git-154)
>From 761d90a2cdef562576a66fc16060b653adacd676 Mon Sep 17 00:00:00 2001
From: Andrey Portnoy <aport...@fastmail.com>
Date: Tue, 1 Oct 2024 20:48:35 -0400
Subject: [PATCH] Use tramp-optimize-remote-path in a separate function
---
lisp/tramp-sh.el | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
index 9e813ecc..9e5ccc4d 100644
--- a/lisp/tramp-sh.el
+++ b/lisp/tramp-sh.el
@@ -4129,6 +4129,9 @@ This function expects to be in the right *tramp* buffer."
(setq result (buffer-substring (point) (line-end-position)))))
result)))
+(defvar tramp-optimize-remote-path t
+ "Whether to remove duplicate and non-existing directories from remote PATH.")
+
;; On hydra.nixos.org, the $PATH environment variable is too long to
;; send it. This is likely not due to PATH_MAX, but PIPE_BUF. We
;; check it, and use a temporary file in case of. See Bug#33781.
@@ -4162,6 +4165,11 @@ variable PATH."
(tramp-send-command vec (format ". %s" tmpfile))
(tramp-send-command vec (format "rm -f %s" tmpfile)))))
+(defun tramp-maybe-set-remote-path (vec)
+ (if (or tramp-optimize-remote-path
+ (not (equal tramp-remote-path '(tramp-own-remote-path))))
+ (tramp-set-remote-path vec)))
+
;; ------------------------------------------------------------
;; -- Communication with external shell --
;; ------------------------------------------------------------
@@ -4511,7 +4519,7 @@ 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)
+ (tramp-maybe-set-remote-path vec)
;; Search for a good shell before searching for a command which
;; checks if a file exists. This is done because Tramp wants to
@@ -5610,17 +5618,21 @@ Nonexistent directories are removed from spec."
(cdr elt2)))
(setq remote-path (delq 'tramp-own-remote-path remote-path)))
- ;; Remove double entries.
- (setq remote-path
- (cl-remove-duplicates
- remote-path :test #'string-equal :from-end t))
-
- ;; Remove non-existing directories.
- (let (remote-file-name-inhibit-cache)
- (tramp-bundle-read-file-names vec remote-path)
- (cl-remove-if
- (lambda (x) (not (tramp-get-file-property vec x "file-directory-p")))
- remote-path))))))
+ (when tramp-optimize-remote-path
+ ;; Remove double entries.
+ (setq remote-path
+ (cl-remove-duplicates
+ remote-path :test #'string-equal :from-end t))
+
+ ;; Remove non-existing directories.
+ (let (remote-file-name-inhibit-cache)
+ (tramp-bundle-read-file-names vec remote-path)
+ (setq remote-path
+ (cl-remove-if
+ (lambda (x)
+ (not (tramp-get-file-property vec x "file-directory-p")))
+ remote-path))))
+ remote-path))))
;; The PIPE_BUF in POSIX [1] can be as low as 512 [2]. Here are the values
;; on various platforms:
--
2.39.5 (Apple Git-154)
