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

[Michael Albinus]

Jim Porter <jporterbugs@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@host:~/") => "/ssh:jim@host:/home/jim/"
(expand-file-name "/ssh:jim@host:~micha/") => "/ssh:jim@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@host:/home/micha/") => "/ssh:jim@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@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.