GNU bug report logs - #51622
29.0.50; [PATCH] Abbreviate remote home directories in `abbreviate-file-name'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 51622 in the body.
You can then email your comments to 51622 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Abbreviate remote home directories in
 `abbreviate-file-name'
Date: Fri, 5 Nov 2021 20:44:44 -0700

[Message part 1 (text/plain, inline)]

Currently, `abbreviate-file-name' abbreviates home directories, but only 
for the local system. For example:

   $ emacs -Q
   M-: (abbreviate-file-name "/home/jim/src") RET
   ;;  => "~/src"
   M-: (abbreviate-file-name "/sshx:localhost:/home/jim/src") RET
   ;;  => "/sshx:localhost:/home/jim/src"

It'd be nice to abbreviate TRAMP home dirs, especially for the buffer 
list, where the long length of TRAMP paths means that I often just see 
the same leading bits of the paths repeated in the File column. As a 
result, it can be hard to tell the exact file it refers to. (Of course, 
as a workaround, I could just widen the window.)

Attached is a patch series to do this, but the patches probably warrant 
some explanation. First, I removed `automount-dir-prefix'; it's been 
obsolete since 24.3, and it would have made implementation of the second 
part more complex.

Second, I removed the caching of the abbreviated home dir. Since adding 
TRAMP support means there are multiple home dirs (one per host), keeping 
the caching would have been fairly complex, and it's already the source 
of potential bugs (e.g. when temporarily setting HOME to something 
else). I did some benchmarking on this (see attached), and while it is 
indeed slower without the caching, I don't think it's worth keeping the 
caching around. The real performance cost comes from calling 
`abbreviate-file-name' with a TRAMP file (even before my patch), but 
abbreviating a local file is quite fast, even with a pathologically 
large `directory-abbrev-alist'. I also wrote a couple of unit tests to 
make sure this function works correctly.

Finally, I added the actual TRAMP support. This has a pretty significant 
  performance hit to TRAMP files. Looking at profiles, this appears to 
be because my patch calls both `file-name-case-insensitive-p' and 
`file-remote-p' on the TRAMP path, and these duplicate quite a bit of 
work. Is there a way to make this more efficient (e.g. by getting the 
file handler just once instead of twice)? It might also be useful to add 
some unit tests here, but I wasn't 100% sure how to do that with TRAMP 
paths (the tests in my benchmark actually open an SSH connection, so 
that probably won't work on all systems).

In addition to the patches, I've also attached a simple benchmark script 
that I used to measure the performance of these patches as well as the 
results from my system. The performance for local paths is still quite 
good I think, and even the worst-case scenario for TRAMP paths 
(abbreviating with a 500-item `directory-abbrev-alist') clocks in at 
4.6ms per call. It'd be nice to make that faster, but maybe that's the 
best we can do if we want this feature.

[0001-Remove-automount-dir-prefix.patch (text/plain, attachment)]

[0002-Don-t-cache-abbreviated-homedir-for-abbreviate-file-.patch (text/plain, attachment)]

[0003-Abbreviate-home-directory-for-remote-files.patch (text/plain, attachment)]

[benchmark.el (text/plain, attachment)]

[benchmark-results.txt (text/plain, attachment)]

Message #8 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50;
 [PATCH] Abbreviate remote home directories in `abbreviate-file-name'
Date: Sat, 06 Nov 2021 10:06:23 +0200

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Fri, 5 Nov 2021 20:44:44 -0700
> 
> Second, I removed the caching of the abbreviated home dir. Since adding 
> TRAMP support means there are multiple home dirs (one per host), keeping 
> the caching would have been fairly complex, and it's already the source 
> of potential bugs (e.g. when temporarily setting HOME to something 
> else). I did some benchmarking on this (see attached), and while it is 
> indeed slower without the caching, I don't think it's worth keeping the 
> caching around.

If some user never uses Tramp, this removal will be a net loss for
that user.  So I think we should have this customizable.  Or maybe
even better: as long as Tramp is not used, keep the cache, and only
stop updating it when Tramp is being used.

Message #11 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Sat, 06 Nov 2021 16:34:47 +0100

[Message part 1 (text/plain, inline)]

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> Currently, `abbreviate-file-name' abbreviates home directories, but
> only for the local system. For example:
>
>    $ emacs -Q
>    M-: (abbreviate-file-name "/home/jim/src") RET
>    ;;  => "~/src"
>    M-: (abbreviate-file-name "/sshx:localhost:/home/jim/src") RET
>    ;;  => "/sshx:localhost:/home/jim/src"
>
> It'd be nice to abbreviate TRAMP home dirs, especially for the buffer
> list, where the long length of TRAMP paths means that I often just see
> the same leading bits of the paths repeated in the File column. As a
> result, it can be hard to tell the exact file it refers to. (Of
> course, as a workaround, I could just widen the window.)

Nice idea :-)

> Attached is a patch series to do this, but the patches probably
> warrant some explanation. First, I removed `automount-dir-prefix';
> it's been obsolete since 24.3, and it would have made implementation
> of the second part more complex.

This is not related to the problem, and it isn't needed because I
propose to skip your second patch (see below). Personally I have no
opinion about, you might apply this patch if you believe it makes sense,
but independently from the feature request.

> Second, I removed the caching of the abbreviated home dir. Since
> adding TRAMP support means there are multiple home dirs (one per
> host), keeping the caching would have been fairly complex, and it's
> already the source of potential bugs (e.g. when temporarily setting
> HOME to something else). I did some benchmarking on this (see
> attached), and while it is indeed slower without the caching, I don't
> think it's worth keeping the caching around. The real performance cost
> comes from calling `abbreviate-file-name' with a TRAMP file (even
> before my patch), but abbreviating a local file is quite fast, even
> with a pathologically large `directory-abbrev-alist'. I also wrote a
> couple of unit tests to make sure this function works correctly.

I disagree. We shall keep the cached abbreviated-home-dir as *local*
home directory. Remote home directories shall be handled in Tramp, and
nowhere else.

This is a general design goal which I try to follow. Mixing Tramp needs
with other packages is good for trouble, and shall be avoided if possible.

> Finally, I added the actual TRAMP support. This has a pretty
> significant    performance hit to TRAMP files. Looking at profiles,
> this appears to be because my patch calls both
> `file-name-case-insensitive-p' and `file-remote-p' on the TRAMP path,
> and these duplicate quite a bit of work. Is there a way to make this
> more efficient (e.g. by getting the file handler just once instead of
> twice)? It might also be useful to add some unit tests here, but I
> wasn't 100% sure how to do that with TRAMP paths (the tests in my
> benchmark actually open an SSH connection, so that probably won't work
> on all systems).

I believe there is a much simpler solution: Add the following entry
(derived from your example) to directory-abbrev-alist:

("\\`/sshx:localhost:/home/jim" . "/sshx:localhost:~")

The appended patch is a proof of concept w/o any systematic testing, it
is not ready for commit. You might use it in order to get the idea, and
to provide an applicable patch. It handles only tramp-sh.el; other Tramp
backends might need something similar.

Tramp tests could be added to tramp-tests.el. You'll see there a Tramp
mockup method which gives you the possibility to add a test w/o a
working ssh connection or alike.

> In addition to the patches, I've also attached a simple benchmark
> script that I used to measure the performance of these patches as well
> as the results from my system. The performance for local paths is
> still quite good I think, and even the worst-case scenario for TRAMP
> paths (abbreviating with a 500-item `directory-abbrev-alist') clocks
> in at 4.6ms per call. It'd be nice to make that faster, but maybe
> that's the best we can do if we want this feature.

I'm eager to see new figures with an adapted patch.

Best regards, Michael.

[Message part 2 (text/x-patch, attachment)]

Message #14 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH] Abbreviate remote home directories in
 `abbreviate-file-name'
Date: Sat, 6 Nov 2021 09:38:59 -0700

On 11/6/2021 8:34 AM, Michael Albinus wrote:
> I disagree. We shall keep the cached abbreviated-home-dir as *local*
> home directory. Remote home directories shall be handled in Tramp, and
> nowhere else.
> 
> This is a general design goal which I try to follow. Mixing Tramp needs
> with other packages is good for trouble, and shall be avoided if possible.

Ok, I can do that. I could even add caching for remote hosts if people 
think it would help (it would probably improve performance, at least a 
little bit). However, while I was looking at the implementation of 
`abbreviate-file-name', I saw the following comment:

      ;; FIXME Is it even worth caching abbreviated-home-dir?
      ;; Ref: https://debbugs.gnu.org/19657#20

After looking over the explanation in that link, I decided to see what 
the performance impact would be if I removed the caching. In my opinion, 
the benchmarks suggest that the caching a small enough impact that the 
brittleness with using the cache outweighed the benefits. However, I 
don't feel strongly about that and if the cache should stay, that's ok 
with me.

> I believe there is a much simpler solution: Add the following entry
> (derived from your example) to directory-abbrev-alist:
> 
> ("\\`/sshx:localhost:/home/jim" . "/sshx:localhost:~")

I had thought about doing that originally, but when I looked into the 
implementation to understand why home-dir abbreviation didn't work on 
remote files, I figured the better long-term solution is to fix 
`abbreviate-file-name' somehow. Then everyone benefits from the improvement.

> The appended patch is a proof of concept w/o any systematic testing, it
> is not ready for commit. You might use it in order to get the idea, and
> to provide an applicable patch. It handles only tramp-sh.el; other Tramp
> backends might need something similar.

Is it a general rule that all Tramp-specific stuff goes into the Tramp 
files, or would it be ok to write a patch that only touches files.el, so 
long as the performance for local files isn't hurt? It wouldn't be 
terribly difficult to replace `abbreviated-home-dir' with something that 
handles multiple hosts, similar to `grep-host-defaults-alist'.

If I understand things correctly, `file-remote-p' can be non-nil for a 
non-Tramp file if it has a file name handler that says so (e.g. if I 
wrote my own package that handles some remote files in a different way 
from Tramp). Maybe that's an argument in favor of changing this in 
`abbreviate-file-name'. Then it works with any remote file. On the other 
hand, maybe we need more protocol-specific information to do this 
correctly, and I should do this inside Tramp...

Either way, I'll look at your patch and see how it compares; if doing it 
that way ends up being better, then I'll try to implement something like 
that. I see in your patch that you add to `directory-abbrev-alist'. Is 
it ok to change a defcustom automatically like this? It seems to work in 
my limited tests, but I thought defcustoms were for users to set 
themselves. Should I come up with a different way to do this if I want 
to merge it into Emacs?

> Tramp tests could be added to tramp-tests.el. You'll see there a Tramp
> mockup method which gives you the possibility to add a test w/o a
> working ssh connection or alike.

Thanks, I'll take a look.

Message #17 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Sat, 06 Nov 2021 18:41:31 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

>> I disagree. We shall keep the cached abbreviated-home-dir as *local*
>> home directory. Remote home directories shall be handled in Tramp, and
>> nowhere else.
>> This is a general design goal which I try to follow. Mixing Tramp
>> needs
>> with other packages is good for trouble, and shall be avoided if possible.
>
> Ok, I can do that. I could even add caching for remote hosts if people
> think it would help (it would probably improve performance, at least a
> little bit). However, while I was looking at the implementation of
> `abbreviate-file-name', I saw the following comment:
>
>       ;; FIXME Is it even worth caching abbreviated-home-dir?
>       ;; Ref: https://debbugs.gnu.org/19657#20
>
> After looking over the explanation in that link, I decided to see what
> the performance impact would be if I removed the caching. In my
> opinion, the benchmarks suggest that the caching a small enough impact
> that the brittleness with using the cache outweighed the
> benefits. However, I don't feel strongly about that and if the cache
> should stay, that's ok with me.

As said, I have no opinion about this. However, we shall care Eli's
opinion, who dislikes such a change.

>> The appended patch is a proof of concept w/o any systematic testing, it
>> is not ready for commit. You might use it in order to get the idea, and
>> to provide an applicable patch. It handles only tramp-sh.el; other Tramp
>> backends might need something similar.
>
> Is it a general rule that all Tramp-specific stuff goes into the Tramp
> files, or would it be ok to write a patch that only touches files.el,
> so long as the performance for local files isn't hurt? It wouldn't be
> terribly difficult to replace `abbreviated-home-dir' with something
> that handles multiple hosts, similar to `grep-host-defaults-alist'.
>
> If I understand things correctly, `file-remote-p' can be non-nil for a
> non-Tramp file if it has a file name handler that says so (e.g. if I
> wrote my own package that handles some remote files in a different way
> from Tramp). Maybe that's an argument in favor of changing this in
> `abbreviate-file-name'. Then it works with any remote file. On the
> other hand, maybe we need more protocol-specific information to do
> this correctly, and I should do this inside Tramp...

The general rule is, to keep all this specific handling away from
files.el and companions. Emacs has the concept of file name handlers,
and if there is something to be done special, a file name handler shall
offer an alternative implementation. This is true for remote file names
(packages tramp*.el, ange-ftp.el, url-handler.el) as well as for other
special handling like automatic compression/decompression of files
(jka-compr.el), GPG encryption/decryption (epa*.el), handling image
files (mage-file.el), handling of tar files (tar-mode.el) etc. Not every
file name handler must implement all "magic" functions, if there's no
specific implementation, the original function is used.

See (info "(elisp) Magic File Names") for the list of basic functions a
file name handler could offer its own implementation, currently these
are 75 functions.

> Either way, I'll look at your patch and see how it compares; if doing
> it that way ends up being better, then I'll try to implement something
> like that. I see in your patch that you add to
> `directory-abbrev-alist'. Is it ok to change a defcustom automatically
> like this? It seems to work in my limited tests, but I thought
> defcustoms were for users to set themselves. Should I come up with a
> different way to do this if I want to merge it into Emacs?

If we want to implement a general purpose solution, not only for remote
file names in Tramp, we shall add `abbreviate-file-name' to that
list. Different file name handlers could start to offer their own
implementation, for the other file name handlers the current default
behavior will be kept. Perhaps we shall go this way.

And the first candidate would be tramp-sh.el.

Best regards, Michael.

Message #20 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Sat, 6 Nov 2021 20:30:39 -0700

[Message part 1 (text/plain, inline)]

On 11/6/2021 10:41 AM, Michael Albinus wrote:
> The general rule is, to keep all this specific handling away from
> files.el and companions. Emacs has the concept of file name handlers,
> and if there is something to be done special, a file name handler shall
> offer an alternative implementation. 

Thanks for the pointers. I've attached a new version of the patch, along 
with updated benchmark results. When abbreviating Tramp files, not only 
is this version faster than my previous patch, it's also 2-4x faster(!) 
than Emacs trunk.

I included a couple of related patches in this series, although I can 
split them out if it would be easier. The first patch just reorders a 
couple of Tramp tests that got added in the wrong order previously (I 
only did this because I wanted to add my new test to the end, and 
figured it would be simpler to fix the order first).

The second patch is the main patch and uses a file name handler as you 
suggested. Hopefully I got everything right here; I'm not very familiar 
with these parts of Tramp. The test I added passes for me, though a 
bunch of the other Tramp tests fail for me (with or without my patches).

Finally, since I already had them lying around, I added a few tests for 
`abbreviate-file-name' for local files. They're pretty simple, but 
should help catch any regressions in the future.

[0001-test-lisp-net-tramp-tests.el-Rearrange-tests-to-be-i.patch (text/plain, attachment)]

[0002-Support-abbreviating-home-directory-of-Tramp-filenam.patch (text/plain, attachment)]

[0003-Add-some-unit-tests-for-abbreviate-file-name.patch (text/plain, attachment)]

[benchmark-results.txt (text/plain, attachment)]

Message #23 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Sun, 07 Nov 2021 19:37:55 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> Thanks for the pointers. I've attached a new version of the patch,
> along with updated benchmark results. When abbreviating Tramp files,
> not only is this version faster than my previous patch, it's also 2-4x
> faster(!) than Emacs trunk.

Thanks, it looks very promising. According to the benchmarks I'm not
surprised, because you use Tramp caches.

> I included a couple of related patches in this series, although I can
> split them out if it would be easier. The first patch just reorders a
> couple of Tramp tests that got added in the wrong order previously (I
> only did this because I wanted to add my new test to the end, and
> figured it would be simpler to fix the order first).

Thanks. I've kept that patch on hold for a while. During my illness, it
got applied, and so you did the dirty task to rearrange everything. I've
pushed it in your name to the master branch. Thanks.

> The second patch is the main patch and uses a file name handler as you
> suggested. Hopefully I got everything right here; I'm not very
> familiar with these parts of Tramp. The test I added passes for me,
> though a bunch of the other Tramp tests fail for me (with or without
> my patches).

Thanks, as said it looks promising. More detailed comments below. For
the other failing Tramp tests please report them. If you like as another
Emacs bug, or directly to me :-)

> Finally, since I already had them lying around, I added a few tests
> for `abbreviate-file-name' for local files. They're pretty simple, but
> should help catch any regressions in the future.

Nice. I've pushed them to the emacs-28 branch in your name, merged also
to the master branch. Maybe you could also add a test for quoted file
names, i.e. a file name "/:/home/albinus/" should *not* be
abbreviated. See files-tests-file-name-non-special-* tests in
files-tests.el.

> diff --git a/etc/NEWS b/etc/NEWS
> index 78c848126a..07861ceee5 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -361,6 +361,13 @@ the buffer will take you to that directory.
>  This is a convenience function to extract the field data from
>  'exif-parse-file' and 'exif-parse-buffer'.
>
> +** Tramp
> +
> ++++
> +*** Tramp supports abbreviating remote home directories now.
> +When calling 'abbreviate-file-name' on a Tramp filename, the result
> +will abbreviate the home directory to "~".

You made it under the Tramp heading. I believe there shall be a more
general entry, that 'abbreviate-file-name' respects file name handlers
now. The entry in the Tramp section can be kept, but in a more general
sense. We don't abbreviate only to "~", but also to "~user", see below.

> diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
> index 6292190940..1151cd2ae8 100644
> --- a/lisp/net/tramp-sh.el
> +++ b/lisp/net/tramp-sh.el
>
> +(defun tramp-sh-handle-abbreviate-file-name (filename)
> +  "Like `abbreviate-file-name' for Tramp files."
> +  (let (home-dir)
> +    (with-parsed-tramp-file-name filename nil
> +      (setq home-dir (tramp-sh-handle-expand-file-name
> +                      (tramp-make-tramp-file-name v "~"))))

Tramp can more. If there are two remote users "jim" and "micha", then
you have

(expand-file-name "/ssh:jim <at> host:~/") => "/ssh:jim <at> host:/home/jim/"
(expand-file-name "/ssh:jim <at> host:~micha/") => "/ssh:jim <at> host:/home/micha/"

abbreviate-file-name is somehow the reverse function of
expand-file-name. So it would be great, if we could have

(abbreviate-file-name "/ssh:jim <at> host:/home/micha/") => "/ssh:jim <at> host:~micha/"

Of course, we cannot check for any remote user's home directory. But we
have the Tramp cache. expand-file-name adds connection properties for
home directories, like ("~" . "/home/jim") and ("~micha" . "/home/micha")
in the above examples. If somebody has already used
"/ssh:jim <at> host:~micha/", and this is cached as ("~micha" . "/home/micha"),
then abbreviate-file-name shall do such an abbreviation. WDYT?

> +    ;; If any elt of directory-abbrev-alist matches this name or the
> +    ;; home dir, abbreviate accordingly.
> +    (dolist (dir-abbrev directory-abbrev-alist)
> +      (when (string-match (car dir-abbrev) filename)
> +        (setq filename
> +              (concat (cdr dir-abbrev)
> +                      (substring filename (match-end 0)))))
> +      (when (string-match (car dir-abbrev) home-dir)
> +        (setq home-dir
> +              (concat (cdr dir-abbrev)
> +                      (substring home-dir (match-end 0))))))

Well, this is copied code from abbreviate-file-name. Usually I try to
avoid such code duplication, it's hard to maintain when it changes in
the first place . Instead, I call the original function via
tramp-run-real-handler, and use the result for further changes.

But abbreviate-file-name does much more than handling
directory-abbrev-alist. Hmm.

> diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
> index 3d6ce963ee..5eea00c41e 100644
> --- a/test/lisp/net/tramp-tests.el
> +++ b/test/lisp/net/tramp-tests.el
> +(ert-deftest tramp-test46-abbreviate-file-name ()

I prefer to keep things together. So I would like to see
tramp-testNN-abbreviate-file-name closed to
tramp-test05-expand-file-name. Could we call the new function
tramp-test07-abbreviate-file-name? I know it will be a lot of work to
rename all the other test functions, and I herewith volunteer to do this
dirty task.

> +  (let ((home-dir (expand-file-name "/mock:localhost:~")))

You hard-code the mock method. But this is available only if
$REMOTE_TEMPORARY_FILE_DIRECTORY is not set; I'd prefer to run
tramp-tests.el in many different environments. So please use something
like

(let ((home-dir (expand-file-name (concat (file-remote-p tramp-test-temporary-file-directory) "~")))))

Beside of these comments, I repeat myself: pretty good job! Thanks!

Best regards, Michael.

Message #26 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Sun, 7 Nov 2021 20:54:25 -0800

[Message part 1 (text/plain, inline)]

On 11/7/2021 10:37 AM, Michael Albinus wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> Thanks for the pointers. I've attached a new version of the patch,
>> along with updated benchmark results. When abbreviating Tramp files,
>> not only is this version faster than my previous patch, it's also 2-4x
>> faster(!) than Emacs trunk.
> 
> Thanks, it looks very promising. According to the benchmarks I'm not
> surprised, because you use Tramp caches.

Hmm, actually it turns out that my patch was only this fast because I 
forgot to check whether the host has case-sensitive file names or not. 
Adding that check back in slows things down again. How I update my 
previous patch will depend on whether we can make 
`file-name-case-insensitive-p' fast for Tramp files, so I'll just focus 
on this part for now and then follow up on the other parts of your 
message after we've decided on what to do here.

Currently on case-sensitive hosts, 
`tramp-handle-file-name-case-insensitive-p' performs its checks on the 
connection every time this function is called. The beginning of tramp.el 
says the following:

  * `tramp-case-insensitive'
    Whether the remote file system handles file names case insensitive.
    Only a non-nil value counts, the default value nil means to
    perform further checks on the remote host.  See
    `tramp-connection-properties' for a way to overwrite this.

I interpret this to mean that Tramp *intentionally* performs checks on 
the host every time if the result is nil. Is there a reason this is 
necessary? Are there any systems out there where the check would return 
nil, but it's still case-insensitive in some cases? Even if there are, I 
imagine that *most* of the time, this check is reliable, and it would be 
nice if we could cache the result for case-sensitive hosts.

I've attached the beginnings of a patch to do this. What do you think? 
If the general idea makes sense, I'll finish it up and file a separate 
bug to track it. If Tramp needs to perform the checks every time for 
some remote hosts, maybe the user could set `tramp-case-insensitive' to 
`never-cache' for those connections?

> Thanks. I've kept that patch on hold for a while. During my illness, it
> got applied, and so you did the dirty task to rearrange everything. I've
> pushed it in your name to the master branch. Thanks.

I hope your health is doing better now. Thanks again for taking a look 
at this patch (and merging the two smaller ones).

[tramp-cache-case-sensitive.patch (text/plain, attachment)]

Message #29 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Mon, 08 Nov 2021 16:58:11 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> Hmm, actually it turns out that my patch was only this fast because I
> forgot to check whether the host has case-sensitive file names or
> not. Adding that check back in slows things down again. How I update
> my previous patch will depend on whether we can make
> `file-name-case-insensitive-p' fast for Tramp files, so I'll just
> focus on this part for now and then follow up on the other parts of
> your message after we've decided on what to do here.
>
> Currently on case-sensitive hosts,
> `tramp-handle-file-name-case-insensitive-p' performs its checks on the
> connection every time this function is called. The beginning of
> tramp.el says the following:
>
>   * `tramp-case-insensitive'
>     Whether the remote file system handles file names case insensitive.
>     Only a non-nil value counts, the default value nil means to
>     perform further checks on the remote host.  See
>     `tramp-connection-properties' for a way to overwrite this.
>
> I interpret this to mean that Tramp *intentionally* performs checks on
> the host every time if the result is nil.

No. It just means, that the value of `tramp-case-insensitive' in
`tramp-methods' is used only if it is non-nil. BUT there is also a check
for a connection property "case-insensitive", and that value is used if
it exists, be it nil or non-nil. Therefore, the whole check in
`tramp-handle-file-name-case-insensitive-p' is applied only once, and
afterwards the cached connection property is used.

See the saved connection properties in file ~/.emacs.d/tramp.

> Is there a reason this is necessary?

The data in `tramp-methods' are static. By default, only non-nil values
are relevant, and we shouldn't change this w/o any need, since we have
connection properties for dynamic data.

> Are there any systems out there where the check would return nil, but
> it's still case-insensitive in some cases? Even if there are, I
> imagine that *most* of the time, this check is reliable, and it would
> be nice if we could cache the result for case-sensitive hosts.

See the comment in `tramp-handle-file-name-case-insensitive-p':

--8<---------------cut here---------------start------------->8---
  ;; We make it a connection property, assuming that all file systems
  ;; on the remote host behave similar.  This might be wrong for
  ;; mounted NFS directories or SMB/AFP shares; such more granular
  ;; tests will be added in case they are needed.
--8<---------------cut here---------------end--------------->8---

So we cache the result, even if it might be problematic. But honestly, I
haven't seen reports yet about such a problem.

> I've attached the beginnings of a patch to do this. What do you think?
> If the general idea makes sense, I'll finish it up and file a separate
> bug to track it. If Tramp needs to perform the checks every time for
> some remote hosts, maybe the user could set `tramp-case-insensitive'
> to `never-cache' for those connections?

Before we start such changes, we should understand your case. Why is it
computed every time? Why isn't the cached value used?

Can you debug? The cache for "case-insensitive" is read by

--8<---------------cut here---------------start------------->8---
(with-tramp-connection-property v "case-insensitive" ...
--8<---------------cut here---------------end--------------->8---

If there isn't a cached value, this macro evaluates its body, and sets
the cache.

You might set tramp-verbose to 7, and follow entries in the debug buffer like

--8<---------------cut here---------------start------------->8---
16:26:43.718831 tramp-get-connection-property (7) # case-insensitive undef; cache used: nil
16:26:43.729457 tramp-set-connection-property (7) # case-insensitive nil
16:26:43.734749 tramp-get-connection-property (7) # case-insensitive nil; cache used: t
--8<---------------cut here---------------end--------------->8---

> I hope your health is doing better now.

It will be up and down, as it happens for years. You will see me to
disappear for some days in the future, from time to time. That's (my) life.

Best regards, Michael.

Message #32 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Mon, 8 Nov 2021 10:32:53 -0800

On 11/8/2021 7:58 AM, Michael Albinus wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
>
>> I've attached the beginnings of a patch to do this. What do you think?
>> If the general idea makes sense, I'll finish it up and file a separate
>> bug to track it. If Tramp needs to perform the checks every time for
>> some remote hosts, maybe the user could set `tramp-case-insensitive'
>> to `never-cache' for those connections?
> 
> Before we start such changes, we should understand your case. Why is it
> computed every time? Why isn't the cached value used?

Thanks for the explanation above. It turns out I was just mistaken about 
what's happening. The check on the remote host *is* performed only once. 
Sorry for the confusion.

If I profile `tramp-handle-file-name-case-insensitive-p' on Emacs 29 
master, I see that 52% of the time is spent in `file-remote-p' (and a 
further 30% in `expand-file-name'). This is where the difference in 
performance comes from; I don't think my patch helps with that, but when 
testing, I eliminated the `file-remote-p' call (and the remote check) 
and saw performance improve. I just got mixed up about what the issue was.

If the calls to `file-remote-p' and `expand-file-name' could be 
optimized or replaced, that would make this function a lot faster. I'll 
take a look at this and see what I can do to improve things. If you have 
any suggestions though, I'm happy to hear about them.

(The main reason I'm digging into this right now is that you mentioned 
it would be nice if we didn't have to copy-and-paste so much code from 
`abbreviate-file-name'. I'm looking into making a new function called 
something like `directory-abbrev-apply', which *only* does the 
`directory-abbrev-alist' replacements so that Tramp can call this. 
However, to make it easier to use correctly, I wanted to make sure 
`case-fold-search' was set based on `file-name-case-insensitive-p'. I 
only want to do that though if I can make that function fast enough that 
it doesn't show up near the top of a performance profile. Otherwise, 
I'll just try to keep the number of calls to 
`file-name-case-insensitive-p' to the bare minimum.)

Message #35 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Mon, 08 Nov 2021 20:18:45 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> If I profile `tramp-handle-file-name-case-insensitive-p' on Emacs 29
> master, I see that 52% of the time is spent in `file-remote-p' (and a
> further 30% in `expand-file-name'). This is where the difference in
> performance comes from; I don't think my patch helps with that, but
> when testing, I eliminated the `file-remote-p' call (and the remote
> check) and saw performance improve. I just got mixed up about what the
> issue was.
>
> If the calls to `file-remote-p' and `expand-file-name' could be
> optimized or replaced, that would make this function a lot
> faster. I'll take a look at this and see what I can do to improve
> things. If you have any suggestions though, I'm happy to hear about
> them.

The check (file-remote-p filename nil 'connected) is applied to prevent
unintended work. It returns non-nil when there is already a connection
to the remote host. The intention is to avoid, that
`tramp-handle-file-name-case-insensitive-p' opens a new connection just
in order to see, whether the remote file system is case
insensitive. This is not needed when there's no connection yet.

Hmm, we could replace this check by something cheaper: is there already
a connection process?

--8<---------------cut here---------------start------------->8---
     (and (let ((non-essential t)) (tramp-connectable-p filename))
          ...
--8<---------------cut here---------------end--------------->8---

The `non-essential' variable must be bound to non-nil; otherwise
`tramp-connectable-p' would always return non-nil.

Does this help? In case of, you might offer a patch, with a comment why
we don't use (file-remote-p filename nil 'connected) .

For the `expand-file-name' case I have no offer yet. But let's see how
the timing changes with that change above.

Btw, when you run benchmark checks, you shall set (or let-bind)
`tramp-verbose' to 0. Writing debug data has a cost.

> (The main reason I'm digging into this right now is that you mentioned
> it would be nice if we didn't have to copy-and-paste so much code from
> `abbreviate-file-name'. I'm looking into making a new function called
> something like `directory-abbrev-apply', which *only* does the
> `directory-abbrev-alist' replacements so that Tramp can call
> this.

Good idea, I was also thinking about. For backward compatibility we will
keep a mirror of that function in tramp-compat.el, until we support only
Emacs 29+. Isn't this a glory future? :-)

Best regards, Michael.

Message #40 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Sat, 13 Nov 2021 18:10:50 -0800

[Message part 1 (text/plain, inline)]

On 11/7/2021 10:37 AM, Michael Albinus wrote:
> Nice. I've pushed them to the emacs-28 branch in your name, merged also
> to the master branch. Maybe you could also add a test for quoted file
> names, i.e. a file name "/:/home/albinus/" should *not* be
> abbreviated. See files-tests-file-name-non-special-* tests in
> files-tests.el.

Ok, I added a test for this in the first patch.

>> +** Tramp
>> +
>> ++++
>> +*** Tramp supports abbreviating remote home directories now.
>> +When calling 'abbreviate-file-name' on a Tramp filename, the result
>> +will abbreviate the home directory to "~".
> 
> You made it under the Tramp heading. I believe there shall be a more
> general entry, that 'abbreviate-file-name' respects file name handlers
> now. The entry in the Tramp section can be kept, but in a more general
> sense. We don't abbreviate only to "~", but also to "~user", see below.

I added a NEWS entry to mention that `abbreviate-file-name' respects 
file name handlers now. I didn't update the Tramp entry though since I 
haven't added "~user" support (at least, not yet...). See below for some 
explanation.

> Tramp can more. If there are two remote users "jim" and "micha", then
> you have
> 
> (expand-file-name "/ssh:jim <at> host:~/") => "/ssh:jim <at> host:/home/jim/"
> (expand-file-name "/ssh:jim <at> host:~micha/") => "/ssh:jim <at> host:/home/micha/"
> 
> abbreviate-file-name is somehow the reverse function of
> expand-file-name. So it would be great, if we could have
> 
> (abbreviate-file-name "/ssh:jim <at> host:/home/micha/") => "/ssh:jim <at> host:~micha/"
> 
> Of course, we cannot check for any remote user's home directory. But we
> have the Tramp cache. expand-file-name adds connection properties for
> home directories, like ("~" . "/home/jim") and ("~micha" . "/home/micha")
> in the above examples. If somebody has already used
> "/ssh:jim <at> host:~micha/", and this is cached as ("~micha" . "/home/micha"),
> then abbreviate-file-name shall do such an abbreviation. WDYT?

I looked at doing this, but it was a bit more complex than I thought it 
would be initially, so I've held off for now. This does seem like a 
useful feature though, and would probably be nice to have for local 
paths too. It should be possible to enhance `expand-file-name' to cache 
the "~user" -> "/home/user" mapping similarly to how 
`tramp-sh-handle-expand-file-name' does.

I could keep working on this patch to add "~user" support, or maybe that 
could be a followup for later. Incidentally, another interesting feature 
would be abbreviating default methods/users. That's probably easy when 
Tramp has filled in those values since the file name has `tramp-default' 
properties set. I'm not sure how tricky it would be to do without those 
properties though.

>> +    ;; If any elt of directory-abbrev-alist matches this name or the
>> +    ;; home dir, abbreviate accordingly.
>> +    (dolist (dir-abbrev directory-abbrev-alist)
>> +      (when (string-match (car dir-abbrev) filename)
>> +        (setq filename
>> +              (concat (cdr dir-abbrev)
>> +                      (substring filename (match-end 0)))))
>> +      (when (string-match (car dir-abbrev) home-dir)
>> +        (setq home-dir
>> +              (concat (cdr dir-abbrev)
>> +                      (substring home-dir (match-end 0))))))
> 
> Well, this is copied code from abbreviate-file-name. Usually I try to
> avoid such code duplication, it's hard to maintain when it changes in
> the first place . Instead, I call the original function via
> tramp-run-real-handler, and use the result for further changes.
> 
> But abbreviate-file-name does much more than handling
> directory-abbrev-alist. Hmm.

I've tried to reduce as much duplication as possible here by creating 
two new functions: `directory-abbrev-make-regexp' and 
`directory-abbrev-apply'. The former just takes a directory and returns 
a regexp that would match it, and the latter loops over 
`directory-abbrev-alist' and applies the transformations.

I tried to do this via `(tramp-run-real-handler #'abbreviate-file-name 
...)', but it ended up being simpler (and faster to execute) this way.

> I prefer to keep things together. So I would like to see
> tramp-testNN-abbreviate-file-name closed to
> tramp-test05-expand-file-name. Could we call the new function
> tramp-test07-abbreviate-file-name? I know it will be a lot of work to
> rename all the other test functions, and I herewith volunteer to do this
> dirty task.

Ok, fixed.

>> +  (let ((home-dir (expand-file-name "/mock:localhost:~")))
> 
> You hard-code the mock method. But this is available only if
> $REMOTE_TEMPORARY_FILE_DIRECTORY is not set; I'd prefer to run
> tramp-tests.el in many different environments. So please use something
> like
> 
> (let ((home-dir (expand-file-name (concat (file-remote-p tramp-test-temporary-file-directory) "~")))))

Fixed this too.

I also attached a slightly-updated benchmark script as well as new 
results. Performance on local file names is the same as before the 
patch, and just slightly faster than before with Tramp file names. (Most 
of the performance improvements here happened in bug#51699, so I mainly 
wanted to maintain the current performance in this patch.)

[0001-Add-another-abbreviate-file-name-test.patch (text/plain, attachment)]

[0002-Support-abbreviating-home-directory-of-Tramp-filenam.patch (text/plain, attachment)]

[benchmark.el (text/plain, attachment)]

[benchmark-results.txt (text/plain, attachment)]

Message #43 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Sun, 14 Nov 2021 15:43:20 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

>> Nice. I've pushed them to the emacs-28 branch in your name, merged also
>> to the master branch. Maybe you could also add a test for quoted file
>> names, i.e. a file name "/:/home/albinus/" should *not* be
>> abbreviated. See files-tests-file-name-non-special-* tests in
>> files-tests.el.
>
> Ok, I added a test for this in the first patch.

Thanks. However, I believe this test shall be called
`files-tests-file-name-non-special-file-abbreviate-file-name', like the
other non-special tests. And perhaps it shall be located prior
`files-tests-file-name-non-special-access-file'.

> I added a NEWS entry to mention that `abbreviate-file-name' respects
> file name handlers now. I didn't update the Tramp entry though since I
> haven't added "~user" support (at least, not yet...). See below for
> some explanation.

Agreed.

> I looked at doing this, but it was a bit more complex than I thought
> it would be initially, so I've held off for now. This does seem like a
> useful feature though, and would probably be nice to have for local
> paths too. It should be possible to enhance `expand-file-name' to
> cache the "~user" -> "/home/user" mapping similarly to how
> `tramp-sh-handle-expand-file-name' does.
>
> I could keep working on this patch to add "~user" support, or maybe
> that could be a followup for later.

ATM, it might be sufficient to push what we have. Adding "~user" support
might come later.

> Incidentally, another interesting
> feature would be abbreviating default methods/users. That's probably
> easy when Tramp has filled in those values since the file name has
> `tramp-default' properties set. I'm not sure how tricky it would be to
> do without those properties though.

You cannot trust the `tramp-default' property. It is set when a method
or user or host name is expanded as in "/ssh::". But when the host name
is used explicitly by the user, as in "/ssh:host:", the property is not
set, even if "host" is the default. Same for user.

But it shouldn't be too hard to determine the defaults. We have
tramp-default-method{-alist}, tramp-default-user{-alist}, and
tramp-default-host{-alist}. All needed information is there.

> I've tried to reduce as much duplication as possible here by creating
> two new functions: `directory-abbrev-make-regexp' and
> `directory-abbrev-apply'. The former just takes a directory and
> returns a regexp that would match it, and the latter loops over
> `directory-abbrev-alist' and applies the transformations.
>
> I tried to do this via `(tramp-run-real-handler #'abbreviate-file-name
> ...)', but it ended up being simpler (and faster to execute) this way.

Fine, let's go this way. After your patch, we'll need some backward
compatibility voodoo in Tramp, but this can wait until the dust has settled.

> I also attached a slightly-updated benchmark script as well as new
> results. Performance on local file names is the same as before the
> patch, and just slightly faster than before with Tramp file
> names. (Most of the performance improvements here happened in
> bug#51699, so I mainly wanted to maintain the current performance in
> this patch.)

Good, no regression :-)

Some few comments on the code:

> --- a/etc/NEWS
> +++ b/etc/NEWS

> +** Tramp
> +
> ++++

This shall be rather "---". We don't add documentation (yet) for this
new Tramp feature.

> +*** Tramp supports abbreviating remote home directories now.
> +When calling 'abbreviate-file-name' on a Tramp filename, the result
> +will abbreviate the home directory to "~".

This might be misleading. ... the result will abbreviate the remote home
directory to "/ssh:user <at> host:~" (for example).

> --- a/lisp/net/tramp-sh.el
> +++ b/lisp/net/tramp-sh.el
> @@ -942,7 +942,8 @@ tramp-vc-registered-read-file-names
>  ;; New handlers should be added here.
>  ;;;###tramp-autoload
>  (defconst tramp-sh-file-name-handler-alist
> -  '((access-file . tramp-handle-access-file)
> +  '((abbreviate-file-name . tramp-sh-handle-abbreviate-file-name)
> +    (access-file . tramp-handle-access-file)

Well, I believe we can implement abbreviation also for other Tramp
backends, like in tramp-sudoedit.el. So it might be better to call this
handler `tramp-handle-abbreviate-file-name'.

> +(defun tramp-sh-handle-abbreviate-file-name (filename)

This shall be in tramp.el then, as `tramp-handle-abbreviate-file-name'.

> +  "Like `abbreviate-file-name' for Tramp files."
> +  (let* ((case-fold-search (tramp-handle-file-name-case-insensitive-p filename))

Please use `case-insensitive-p'. We don't know whether there will be
other implementation for this magic function in the future. And we shall
not bypass the checks in `tramp-file-name-handler', which are important
for parallel invocations of Tramp handlers.

> +         (home-dir
> +          (with-parsed-tramp-file-name filename nil
> +            (with-tramp-connection-property v "home-directory"
> +              (directory-abbrev-apply (tramp-sh-handle-expand-file-name

Same here. Please use `expand-file-name'.

Best regards, Michael.

Message #46 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v3] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Sun, 14 Nov 2021 22:58:31 -0800

[Message part 1 (text/plain, inline)]

On 11/14/2021 6:43 AM, Michael Albinus wrote:
> Thanks. However, I believe this test shall be called
> `files-tests-file-name-non-special-file-abbreviate-file-name', like the
> other non-special tests. And perhaps it shall be located prior
> `files-tests-file-name-non-special-access-file'.

Ok, done.

>> Incidentally, another interesting
>> feature would be abbreviating default methods/users. That's probably
>> easy when Tramp has filled in those values since the file name has
>> `tramp-default' properties set. I'm not sure how tricky it would be to
>> do without those properties though.
> 
> You cannot trust the `tramp-default' property. It is set when a method
> or user or host name is expanded as in "/ssh::". But when the host name
> is used explicitly by the user, as in "/ssh:host:", the property is not
> set, even if "host" is the default. Same for user.
> 
> But it shouldn't be too hard to determine the defaults. We have
> tramp-default-method{-alist}, tramp-default-user{-alist}, and
> tramp-default-host{-alist}. All needed information is there.

Right, that confirms what I suspected. I'll try to look into this in 
more detail later when I get the chance.

>> I also attached a slightly-updated benchmark script as well as new
>> results. Performance on local file names is the same as before the
>> patch, and just slightly faster than before with Tramp file
>> names. (Most of the performance improvements here happened in
>> bug#51699, so I mainly wanted to maintain the current performance in
>> this patch.)
> 
> Good, no regression :-)

Fixing your comments below *did* regress performance for abbreviating 
Tramp file names compared to current master (it takes 1.47x as long now 
in the worst case), but it's still considerably faster than Emacs 28. 
I've attached updated benchmark results to show the difference.

> This shall be rather "---". We don't add documentation (yet) for this
> new Tramp feature.

Fixed.

>> +*** Tramp supports abbreviating remote home directories now.
>> +When calling 'abbreviate-file-name' on a Tramp filename, the result
>> +will abbreviate the home directory to "~".
> 
> This might be misleading. ... the result will abbreviate the remote home
> directory to "/ssh:user <at> host:~" (for example).

Ok, I tried to make this section clearer.

> Well, I believe we can implement abbreviation also for other Tramp
> backends, like in tramp-sudoedit.el. So it might be better to call this
> handler `tramp-handle-abbreviate-file-name'.

Done. I added this for the sudoedit and smb methods, since both support
"~" expansion in `expand-file-name'. That *should* be sufficient, but 
I've never used either of those methods, so I could be wrong...

> Please use `case-insensitive-p'. We don't know whether there will be
> other implementation for this magic function in the future. And we shall
> not bypass the checks in `tramp-file-name-handler', which are important
> for parallel invocations of Tramp handlers.

Fixed (same with `expand-file-name'). These changes slow things down a 
fair bit, but that's mostly due to checking for the right file name 
handler more often. Like I said above though, it's still a lot faster 
than Emacs 28 (thanks to bug#51699).

[0001-Add-another-abbreviate-file-name-test.patch (text/plain, attachment)]

[0002-Support-abbreviating-home-directory-of-Tramp-filenam.patch (text/plain, attachment)]

[benchmark-results.txt (text/plain, attachment)]

Message #49 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v3] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Mon, 15 Nov 2021 17:59:25 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

>> Thanks. However, I believe this test shall be called
>> `files-tests-file-name-non-special-file-abbreviate-file-name', like the
>> other non-special tests. And perhaps it shall be located prior
>> `files-tests-file-name-non-special-access-file'.
>
> Ok, done.

Thanks, I've pushed this to the master branch.

>> But it shouldn't be too hard to determine the defaults. We have
>> tramp-default-method{-alist}, tramp-default-user{-alist}, and
>> tramp-default-host{-alist}. All needed information is there.
>
> Right, that confirms what I suspected. I'll try to look into this in
> more detail later when I get the chance.

Would be interesting to see the result :-)

>>> I also attached a slightly-updated benchmark script as well as new
>>> results. Performance on local file names is the same as before the
>>> patch, and just slightly faster than before with Tramp file
>>> names. (Most of the performance improvements here happened in
>>> bug#51699, so I mainly wanted to maintain the current performance in
>>> this patch.)
>> Good, no regression :-)
>
> Fixing your comments below *did* regress performance for abbreviating
> Tramp file names compared to current master (it takes 1.47x as long
> now in the worst case), but it's still considerably faster than Emacs
> 28. I've attached updated benchmark results to show the difference.

Yes, that's the price we have to pay for clean code. I've explained why
it is needed.

However, the `tramp-file-name-handler' machinery has been grown over 20+
years. I'm not aware of useless stuff, but maybe it's worth to have an
*external* look at this wrt performance. Are you interested?

>> Well, I believe we can implement abbreviation also for other Tramp
>> backends, like in tramp-sudoedit.el. So it might be better to call this
>> handler `tramp-handle-abbreviate-file-name'.
>
> Done. I added this for the sudoedit and smb methods, since both support
> "~" expansion in `expand-file-name'. That *should* be sufficient, but
> I've never used either of those methods, so I could be wrong...

Thanks. I didn't find the time to check it for these methods, but it's
on my TODO for next days.

>> Please use `case-insensitive-p'. We don't know whether there will be
>> other implementation for this magic function in the future. And we shall
>> not bypass the checks in `tramp-file-name-handler', which are important
>> for parallel invocations of Tramp handlers.
>
> Fixed (same with `expand-file-name'). These changes slow things down a
> fair bit, but that's mostly due to checking for the right file name
> handler more often. Like I said above though, it's still a lot faster
> than Emacs 28 (thanks to bug#51699).

I've committed everything to master. Then I ran the regression tests,
and there were indeed some few surprises. All of them shall be fixed now
with my commit after yours.

I tend to close this bug report now, since everything reported has been
implemented. The open points don't need this bug anymore for progress.

WDYT?

Best regards, Michael.

Message #52 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v3] Abbreviate remote home directories
 in `abbreviate-file-name'
Date: Mon, 15 Nov 2021 17:14:33 -0800

On 11/15/2021 8:59 AM, Michael Albinus wrote:
>> Fixing your comments below *did* regress performance for abbreviating
>> Tramp file names compared to current master (it takes 1.47x as long
>> now in the worst case), but it's still considerably faster than Emacs
>> 28. I've attached updated benchmark results to show the difference.
> 
> Yes, that's the price we have to pay for clean code. I've explained why
> it is needed.

Agreed. It's better to be "slow and right" than "fast and wrong". :)

> I've committed everything to master. Then I ran the regression tests,
> and there were indeed some few surprises. All of them shall be fixed now
> with my commit after yours.

Thanks for merging everything. I'll be sure to keep the changes in your 
followup commit in mind (especially the `tramp-compat-funcall' parts) if 
I make any future Tramp patches.

> I tend to close this bug report now, since everything reported has been
> implemented. The open points don't need this bug anymore for progress.
> 
> WDYT?

Sounds good to me, we can close this.

Message #57 received at 51622-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622-done <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v3] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Tue, 16 Nov 2021 12:43:46 +0100

Version: 29.1

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

>> I tend to close this bug report now, since everything reported has been
>> implemented. The open points don't need this bug anymore for progress.
>> WDYT?
>
> Sounds good to me, we can close this.

Thanks for the feedback, closing the bug.

Best regards, Michael.

Message #60 received at 51622 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51622 <at> debbugs.gnu.org
Subject: Re: bug#51622: 29.0.50; [PATCH v3] Abbreviate remote home
 directories in `abbreviate-file-name'
Date: Tue, 16 Nov 2021 13:57:17 +0100

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

Hi Jim,

>> Done. I added this for the sudoedit and smb methods, since both support
>> "~" expansion in `expand-file-name'. That *should* be sufficient, but
>> I've never used either of those methods, so I could be wrong...
>
> Thanks. I didn't find the time to check it for these methods, but it's
> on my TODO for next days.

FTR, I've just tested both methods, and they work out-of-the-box, as expected.

Best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.