bug#51622: 29.0.50; [PATCH v2] Abbreviate remote home directories in `abbreviate-file-name'

[Michael Albinus <michael.albinus@gmx.de>]

Jim Porter <jporterbugs@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@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.