Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

From: Jim Porter <[email-masked]>
Date: Wed, 10 Nov 2021 16:48:03 -0800

On 11/10/2021 4:17 AM, Michael Albinus wrote:
> `tramp-find-foreign-file-name-handler' loops through
> `tramp-register-foreign-file-name-handler'. Only operations registered
> there need to support a VEC-OR-FILENAME argument.
> `tramp-archive-file-name-p' and `tramp-crypt-file-name-p' are not
> registered, so no change is needed.

Ah ha. I didn't realize they weren't included (I thought I just had to 
configure something to add them in 
`tramp-foreign-file-name-handler-alist'). In that case, it definitely 
makes sense eliminate the `accepts-vec' bits from my patch.

> We could (and should) inform the authors of both packages, that the
> signature for the `tramp-FOO-file-name-p' functions will change with
> Tramp 2.6. As I can see, there's no problem to adapt
> `magit-tramp-file-name-p' and `tramp-jumper-file-name-p'. And yes, an
> entry in etc/NEWS should be added as well.

Sounds good to me. I've added a NEWS entry that hopefully explains the 
change fairly clearly. Just to be extra-certain, I re-ran my performance 
tests from before and the results are the same.

> A  little bit more friendly for debugging:
> 
> --8<---------------cut here---------------start------------->8---
> 	;; The signature of `tramp-FOO-file-name-p' has changed, it
> 	;; expects a VEC here.
>          (when (with-demoted-errors "Error: %S" (funcall (car elt) vec))
> --8<---------------cut here---------------end--------------->8---

I went with this since it should make it easier for third parties to 
notice the issue and make the appropriate update to their code. Thanks 
for the pointer.
From 2aec8e21a3e37728a990c4f116f60c8b12bb2110 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Wed, 10 Nov 2021 16:41:00 -0800
Subject: [PATCH] Improve performance of 'tramp-find-foreign-file-name-handler'

Previously, each function in 'tramp-foreign-file-name-handler-alist'
would call 'tramp-dissect-file-name', resulting in it being called
several times whenever 'tramp-find-foreign-file-name-handler' was
called.  Now, functions take the dissected file name to avoid this
duplicated effort.

* lisp/net/tramp.el (tramp-ensure-dissected-file-name): New function.
(tramp-find-foreign-file-name-handler): Pass dissected file name to
functions.
(tramp-connectable-p): Use 'tramp-ensure-dissected-file-name'.

* lisp/net/tramp-adb.el (tramp-adb-file-name-p):
* lisp/net/tramp-ftp.el (tramp-ftp-file-name-p):
* lisp/net/tramp-gvfs.el (tramp-gvfs-file-name-p):
* lisp/net/tramp-rclone.el (tramp-rclone-file-name-p):
* lisp/net/tramp-smb.el (tramp-smb-file-name-p):
* lisp/net/tramp-sshfs.el (tramp-sshfs-file-name-p):
* lisp/net/tramp-sudoedit.el (tramp-sudoedit-file-name-p):
Accept dissected file names.

* etc/NEWS: Announce this change.
---
 etc/NEWS                   |  6 ++++++
 lisp/net/tramp-adb.el      |  9 ++++-----
 lisp/net/tramp-ftp.el      |  9 ++++-----
 lisp/net/tramp-gvfs.el     | 11 +++++------
 lisp/net/tramp-rclone.el   |  9 ++++-----
 lisp/net/tramp-smb.el      |  9 ++++-----
 lisp/net/tramp-sshfs.el    |  9 ++++-----
 lisp/net/tramp-sudoedit.el |  9 ++++-----
 lisp/net/tramp.el          | 26 ++++++++++++++++++--------
 9 files changed, 53 insertions(+), 44 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 807f31fa33..9f3a773a5b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -571,6 +571,12 @@ get proper locale-dependent downcasing, the string has to be converted
 to multibyte first.  (This goes for the other case-changing functions,
 too.)
 
+---
+** Functions in 'tramp-foreign-file-name-handler-alist' have changed.
+Functions to determine which Tramp file name handler to use are now
+passed a file name in dissected form (via 'tramp-dissect-file-name')
+instead of in string form.
+
 ---
 ** 'def' indentation changes.
 In 'emacs-lisp-mode', forms with a symbol with a name that start with
diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index 362a258f43..e7fe07e417 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -191,11 +191,10 @@ tramp-adb-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-adb-file-name-p (filename)
-  "Check if it's a FILENAME for ADB."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-adb-method)))
+(defsubst tramp-adb-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for ADB."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-adb-method)))
 
 ;;;###tramp-autoload
 (defun tramp-adb-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-ftp.el b/lisp/net/tramp-ftp.el
index 11ccdc8a4c..f78c08ec41 100644
--- a/lisp/net/tramp-ftp.el
+++ b/lisp/net/tramp-ftp.el
@@ -175,11 +175,10 @@ tramp-ftp-file-name-handler
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-ftp-file-name-p (filename)
-  "Check if it's a FILENAME that should be forwarded to Ange-FTP."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-ftp-method)))
+(defsubst tramp-ftp-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME that should be forwarded to Ange-FTP."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-ftp-method)))
 
 ;;;###tramp-autoload
 (tramp--with-startup
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index cab912bd93..1f9d9d9415 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -834,12 +834,11 @@ tramp-gvfs-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-gvfs-file-name-p (filename)
-  "Check if it's a FILENAME handled by the GVFS daemon."
-  (and (tramp-tramp-file-p filename)
-       (let ((method
-	      (tramp-file-name-method (tramp-dissect-file-name filename))))
-	 (and (stringp method) (member method tramp-gvfs-methods)))))
+(defsubst tramp-gvfs-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME handled by the GVFS daemon."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (let ((method (tramp-file-name-method vec)))
+      (and (stringp method) (member method tramp-gvfs-methods)))))
 
 ;;;###tramp-autoload
 (defun tramp-gvfs-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
index 812e06f3f1..64b0176d08 100644
--- a/lisp/net/tramp-rclone.el
+++ b/lisp/net/tramp-rclone.el
@@ -156,11 +156,10 @@ tramp-rclone-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-rclone-file-name-p (filename)
-  "Check if it's a FILENAME for rclone."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-rclone-method)))
+(defsubst tramp-rclone-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for rclone."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-rclone-method)))
 
 ;;;###tramp-autoload
 (defun tramp-rclone-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index 49f049d3f3..aeabc69246 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -330,11 +330,10 @@ tramp-smb-winexe-shell-command-switch
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-smb-file-name-p (filename)
-  "Check if it's a FILENAME for SMB servers."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-smb-method)))
+(defsubst tramp-smb-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for SMB servers."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-smb-method)))
 
 ;;;###tramp-autoload
 (defun tramp-smb-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sshfs.el b/lisp/net/tramp-sshfs.el
index a100786345..4bc804571e 100644
--- a/lisp/net/tramp-sshfs.el
+++ b/lisp/net/tramp-sshfs.el
@@ -156,11 +156,10 @@ tramp-sshfs-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-sshfs-file-name-p (filename)
-  "Check if it's a FILENAME for sshfs."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-	        tramp-sshfs-method)))
+(defsubst tramp-sshfs-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for sshfs."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-sshfs-method)))
 
 ;;;###tramp-autoload
 (defun tramp-sshfs-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index 845f31d09b..48c81a5988 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -148,11 +148,10 @@ tramp-sudoedit-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-sudoedit-file-name-p (filename)
-  "Check if it's a FILENAME for SUDOEDIT."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-sudoedit-method)))
+(defsubst tramp-sudoedit-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for SUDOEDIT."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-sudoedit-method)))
 
 ;;;###tramp-autoload
 (defun tramp-sudoedit-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index a8972ce69e..85effe1a04 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -1669,6 +1669,16 @@ tramp-dissect-file-name
 
 (put #'tramp-dissect-file-name 'tramp-suppress-trace t)
 
+(defun tramp-ensure-dissected-file-name (vec-or-filename)
+  "Return a `tramp-file-name' structure for VEC-OR-FILENAME.
+
+VEC-OR-FILENAME may be either a string or a `tramp-file-name'.
+If it's not a Tramp filename, return nil."
+  (cond
+   ((tramp-file-name-p vec-or-filename) vec-or-filename)
+   ((tramp-tramp-file-p vec-or-filename)
+    (tramp-dissect-file-name vec-or-filename))))
+
 (defun tramp-dissect-hop-name (name &optional nodefault)
   "Return a `tramp-file-name' structure of `hop' part of NAME.
 See `tramp-dissect-file-name' for details."
@@ -2552,11 +2562,14 @@ tramp-find-foreign-file-name-handler
   "Return foreign file name handler if exists."
   (when (tramp-tramp-file-p filename)
     (let ((handler tramp-foreign-file-name-handler-alist)
+          (vec (tramp-dissect-file-name filename))
 	  elt res)
       (while handler
 	(setq elt (car handler)
 	      handler (cdr handler))
-	(when (funcall (car elt) filename)
+        ;; Previously, this function was called with FILENAME, but now
+        ;; it's called with the VEC.
+        (when (with-demoted-errors "Error: %S" (funcall (car elt) vec))
 	  (setq handler nil
 		res (cdr elt))))
       res)))
@@ -2755,8 +2768,9 @@ tramp-register-file-name-handlers
 (defun tramp-register-foreign-file-name-handler
     (func handler &optional append)
   "Register (FUNC . HANDLER) in `tramp-foreign-file-name-handler-alist'.
-FUNC is the function, which determines whether HANDLER is to be called.
-Add operations defined in `HANDLER-alist' to `tramp-file-name-handler'."
+FUNC is the function, which takes a dissected filename and determines
+whether HANDLER is to be called.  Add operations defined in
+`HANDLER-alist' to `tramp-file-name-handler'."
   (add-to-list
    'tramp-foreign-file-name-handler-alist `(,func . ,handler) append)
   ;; Mark `operations' the handler is responsible for.
@@ -2814,11 +2828,7 @@ tramp-connectable-p
 This is true, if either the remote host is already connected, or if we are
 not in completion mode."
   (let ((tramp-verbose 0)
-	(vec
-	 (cond
-	  ((tramp-file-name-p vec-or-filename) vec-or-filename)
-	  ((tramp-tramp-file-p vec-or-filename)
-	   (tramp-dissect-file-name vec-or-filename)))))
+	(vec (tramp-ensure-dissected-file-name vec-or-filename)))
     (or ;; We check this for the process related to
 	;; `tramp-buffer-name'; otherwise `start-file-process'
 	;; wouldn't run ever when `non-essential' is non-nil.
-- 
2.25.1