Package: emacs;
Reported by: Jim Porter <jporterbugs <at> gmail.com>
Date: Sat, 6 Nov 2021 03:45:02 UTC
Severity: wishlist
Tags: patch
Found in version 29.0.50
Fixed in version 29.1
Done: Michael Albinus <michael.albinus <at> gmx.de>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Michael Albinus <michael.albinus <at> gmx.de> To: Jim Porter <jporterbugs <at> gmail.com> Cc: 51622 <at> debbugs.gnu.org Subject: 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.