bug#54001: 29.0.50; abbreviate-file-name has side-effects

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package: Emacs
Version: 29.0.50


Since:

    commit bf505a63f98ed61934a8fb81ec65c96859606b6e
    Author: Jim Porter <jporterbugs@gmail.com>
    Date:   Mon Nov 15 13:33:07 2021 +0100

    Support abbreviating home directory of Tramp filenames

`abbreviate-file-name` has significantly changed in its behavior:
- it's slower (because it goes through file-name-handlers)
- it can have very visible side effects like prompting the user for a password.

I haven't measured the slowdown, so I'll assume it's acceptable, but
asking for a password (or contacting a remote host) is not.

I suggest we take a step back and think of how to get that feature
without having to contact any remote host during `abbreviate-file-name`.
Maybe we can do that by making Tramp opportunistically add entries to
`directory-abbrev-alist` when it performs expansion?


        Stefan

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Jim Porter]

On 2/14/2022 9:19 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Package: Emacs
> Version: 29.0.50
> 
> 
> Since:
> 
>      commit bf505a63f98ed61934a8fb81ec65c96859606b6e
>      Author: Jim Porter <jporterbugs@gmail.com>
>      Date:   Mon Nov 15 13:33:07 2021 +0100
> 
>      Support abbreviating home directory of Tramp filenames
> 
> `abbreviate-file-name` has significantly changed in its behavior:
> - it's slower (because it goes through file-name-handlers)
> - it can have very visible side effects like prompting the user for a password.
> 
> I haven't measured the slowdown, so I'll assume it's acceptable, but
> asking for a password (or contacting a remote host) is not.

Sorry about that. I did what I could to minimize the slowdown (including 
some more general optimizations to make Tramp faster). There are some 
benchmarks in the original bug here (these are with 1000 iterations; 
you'll want to compare the first section with the last): 
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg01293.html>.

> I suggest we take a step back and think of how to get that feature
> without having to contact any remote host during `abbreviate-file-name`.
> Maybe we can do that by making Tramp opportunistically add entries to
> `directory-abbrev-alist` when it performs expansion?

I think Michael Albinus suggested doing that in the original bug, 
although I was concerned about modifying defcustoms invisibly like that. 
Is that ok to do? Another option might be to store the abbreviations for 
a given file-name-handler somewhere internally and consult that when 
calling that file-name-handler's implementation of `abbreviate-file-name'.

Maybe this patch should be backed out for now; it shouldn't be 
interrupting the user. (I thought I'd tested that, but maybe it was on 
an earlier revision of the patch.) I'll probably have time to look into 
a new solution in a few weeks, but anyone else who's interested should 
feel free to fix it in the meantime.

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I suggest we take a step back and think of how to get that feature
>> without having to contact any remote host during `abbreviate-file-name`.
>> Maybe we can do that by making Tramp opportunistically add entries to
>> `directory-abbrev-alist` when it performs expansion?
>
> I think Michael Albinus suggested doing that in the original bug, although
> I was concerned about modifying defcustoms invisibly like that. Is that ok
> to do?

No it's not, but we can add a non-defcustom'd variable holding
additional entries to circumvent this problem.

> Another option might be to store the abbreviations for a given
> file-name-handler somewhere internally and consult that when calling that
> file-name-handler's implementation of `abbreviate-file-name'.

That would work as well.


        Stefan

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Jim Porter]

On 2/14/2022 11:48 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>>> I suggest we take a step back and think of how to get that feature
>>> without having to contact any remote host during `abbreviate-file-name`.
>>> Maybe we can do that by making Tramp opportunistically add entries to
>>> `directory-abbrev-alist` when it performs expansion?
[snip]
>> Another option might be to store the abbreviations for a given
>> file-name-handler somewhere internally and consult that when calling that
>> file-name-handler's implementation of `abbreviate-file-name'.
> 
> That would work as well.

Looking at my original patch again, this value is already cached as a 
Tramp connection property named "home-directory". If the code were 
changed to set that connection property in a more-appropriate place, and 
`tramp-handle-abbreviate-file-name' simply *reads* that connection 
property, then I think that should resolve this issue.

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Looking at my original patch again, this value is already cached as a Tramp
> connection property named "home-directory". If the code were changed to set
> that connection property in a more-appropriate place, and
> `tramp-handle-abbreviate-file-name' simply *reads* that connection property,
> then I think that should resolve this issue.

Sounds like it,


        Stefan

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

On 2/14/2022 1:00 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> Looking at my original patch again, this value is already cached as a Tramp
>> connection property named "home-directory". If the code were changed to set
>> that connection property in a more-appropriate place, and
>> `tramp-handle-abbreviate-file-name' simply *reads* that connection property,
>> then I think that should resolve this issue.
> 
> Sounds like it,

I've only lightly tested this patch, but I think it should resolve the 
issue. It only sets the "home-directory" connection property when a 
connection is already established. Otherwise, it just uses the cached 
value (if any).

Maybe there's a cleaner way to do this though; opportunistically setting 
a connection property like this seems like something there might be a 
special function for in Tramp, but I didn't see one after a bit of 
looking so this is what I went with.

[-- Attachment #2: 0001-Don-t-attempt-to-connect-to-a-remote-server-during-a.patch --]
[-- Type: text/plain, Size: 2206 bytes --]

From f66dcbdbcd26ad97423e37034508a32f55263597 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 14 Feb 2022 13:16:05 -0800
Subject: [PATCH] Don't attempt to connect to a remote server during
 'abbreviate-file-name'

* lisp/net/tramp.el (tramp-handle-abbreviate-file-name): Only use the
"home-directory" when a connection has been established.
---
 lisp/net/tramp.el | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 32712efb3e..3c06ad1630 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -3343,16 +3343,24 @@ tramp-handle-abbreviate-file-name
   (let* ((case-fold-search (file-name-case-insensitive-p filename))
 	 (vec (tramp-dissect-file-name filename))
          (home-dir
-          (with-tramp-connection-property vec "home-directory"
-            (tramp-compat-funcall
-	     'directory-abbrev-apply
-	     (expand-file-name (tramp-make-tramp-file-name vec "~"))))))
+          (if (let ((non-essential t)) (tramp-connectable-p vec))
+              ;; If a connection has already been established, make
+              ;; sure the "home-directory" connection property is
+              ;; properly set.
+              (with-tramp-connection-property vec "home-directory"
+                (tramp-compat-funcall
+	         'directory-abbrev-apply
+	         (expand-file-name (tramp-make-tramp-file-name vec "~"))))
+            ;; Otherwise, just use the cached value.
+            (tramp-get-connection-property vec "home-directory" nil))))
     ;; If any elt of `directory-abbrev-alist' matches this name,
     ;; abbreviate accordingly.
     (setq filename (tramp-compat-funcall 'directory-abbrev-apply filename))
     ;; Abbreviate home directory.
-    (if (string-match
-	 (tramp-compat-funcall 'directory-abbrev-make-regexp home-dir) filename)
+    (if (and home-dir
+             (string-match
+	      (tramp-compat-funcall 'directory-abbrev-make-regexp home-dir)
+              filename))
         (tramp-make-tramp-file-name
 	 vec (concat "~" (substring filename (match-beginning 1))))
       (tramp-make-tramp-file-name (tramp-dissect-file-name filename)))))
-- 
2.25.1

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter [2022-02-14 13:38:58] wrote:
> I've only lightly tested this patch, but I think it should resolve the
> issue. It only sets the "home-directory" connection property when
> a connection is already established. Otherwise, it just uses the cached
> value (if any).
>
> Maybe there's a cleaner way to do this though; opportunistically setting
> a connection property like this seems like something there might be
> a special function for in Tramp, but I didn't see one after a bit of looking
> so this is what I went with.

Thanks Jim.

Michael, I assume you'll want to take care of this patch.  Let me know
if you prefer that I install it, of course,


        Stefan

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Jim, Stefan,

>> I've only lightly tested this patch, but I think it should resolve the
>> issue. It only sets the "home-directory" connection property when
>> a connection is already established. Otherwise, it just uses the cached
>> value (if any).
>
> Thanks Jim.
>
> Michael, I assume you'll want to take care of this patch.  Let me know
> if you prefer that I install it, of course,

Yes, it is OK. Please install.

>> Maybe there's a cleaner way to do this though; opportunistically setting
>> a connection property like this seems like something there might be
>> a special function for in Tramp, but I didn't see one after a bit of looking
>> so this is what I went with.

I've recently made some changes for home directories in the other Tramp
backends. Likely, I need to revise these settings once the patch has
arrived on master.

>         Stefan

Best regards, Michael.

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I've only lightly tested this patch, but I think it should resolve the
> issue. It only sets the "home-directory" connection property when
> a connection is already established. Otherwise, it just uses the cached
> value (if any).

Thanks, pushed to master.  And closing.
Daniel, feel to ping us back if the fix isn't good enough.


        Stefan

bug#54001: 29.0.50; abbreviate-file-name has side-effects

[Daniel Mendler]

On 2/15/22 14:11, Stefan Monnier wrote:
>> I've only lightly tested this patch, but I think it should resolve the
>> issue. It only sets the "home-directory" connection property when
>> a connection is already established. Otherwise, it just uses the cached
>> value (if any).
> 
> Thanks, pushed to master.  And closing.
> Daniel, feel to ping us back if the fix isn't good enough.

Thanks! I will if the issue comes up again.

Daniel