From: Jim Porter <jporterbugs@gmail.com>
To: Michael Albinus <michael.albinus@gmx.de>
Cc: 51699@debbugs.gnu.org
Subject: bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files
Date: Wed, 10 Nov 2021 16:48:03 -0800 [thread overview]
Message-ID: <0f898153-b128-c39b-f366-55cb1bacad57@gmail.com> (raw)
In-Reply-To: <87wnlgtdbv.fsf@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]
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.
[-- Attachment #2: 0001-Improve-performance-of-tramp-find-foreign-file-name-.patch --]
[-- Type: text/plain, Size: 11585 bytes --]
From 2aec8e21a3e37728a990c4f116f60c8b12bb2110 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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
next prev parent reply other threads:[~2021-11-11 0:48 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-09 3:46 bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files Jim Porter
2021-11-09 19:34 ` Michael Albinus
2021-11-09 21:22 ` Jim Porter
2021-11-10 12:17 ` Michael Albinus
2021-11-11 0:48 ` Jim Porter [this message]
2021-11-11 18:51 ` Michael Albinus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0f898153-b128-c39b-f366-55cb1bacad57@gmail.com \
--to=jporterbugs@gmail.com \
--cc=51699@debbugs.gnu.org \
--cc=michael.albinus@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.