bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Drew Adams]

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

`find-lisp.el' is an old file that doesn't seem to have gotten much
love.  It's apparently been updated just to change things like use of
(set (make-local-variable...)) to (setq-local...).

(I wasn't aware of this library till I saw a blog post by Micky Petersen:
https://www.masteringemacs.org/article/working-multiple-files-dired.)

The attached patch fixes a few bugs that prevent using it in some
important ways: (1) opening a Dired buffer that explicitly lists its
listed files or dirs (i.e. a snapshot Dired buffer, where DIRNAME is a
cons), (2) using its listing with a `dired-actual-switches' value that
contains switch `F', (3) properly handling a listed file or dir name
that starts with a space char.

[#1 is an important use case.  Because a `find' command (Lisp or not)
can take quite a while, it can be useful to make a snapshot of its
result, i.e., save the list of files and later restore a Dired buffer
for that explicit set of files, without rerunning `find'.  For that use
case, at least, these bugs need to be fixed.]

The patch does the following.  1-2 are improvements, 3-4 are bug fixes.

1. Add other-window versions of commands `find-lisp-find-dired' and
   `find-lisp-find-dired-subdirectories'.

   (Really, both command names should be shortened.  Really, most names
   in this library should be shortened/improved.  And really, the name
   `find-lisp-find-dired' is not good - besides a confusing combination
   of name components, it doesn't tell you that it's only about files,
   not files and dirs - its companion for listing only subdirs has that
   in its name.)

2. `find-lisp-find-dired-internal': Added optional arg OTHER-WINDOW.

3. `find-lisp-find-dired-insert-file':

   * Add a doc string.
   * Put text property `dired-filename' on the inserted file/dir name.
     (This fixes the mistreatment of file/dir names that start with a
     space char.)
   * Start with `dired-actual-switches', instead of just "".  In
     particular, this is to keep switch `F'.

4. `find-lisp-format': If `F' listing switch is present then append `/'
   to listed directories.

Attached is a patch against master today.

Other changes could also be made, both to clean up the code and to
improve the behavior.  But that will wait for someone else.



In GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2019-08-29
Repository revision: 96dd0196c28bc36779584e47fffcca433c9309cd
Windowing system distributor `Microsoft Corp.', version 10.0.19044
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''


[-- Attachment #2: find-lisp-2022-06-10a.patch --]
[-- Type: application/octet-stream, Size: 5591 bytes --]

diff -u find-lisp.el find-lisp-PATCHED-2022-06-10a.el
--- find-lisp.el	2022-06-10 08:13:53.540137300 -0700
+++ find-lisp-PATCHED-2022-06-10a.el	2022-06-10 14:24:28.845259100 -0700
@@ -166,7 +166,7 @@
 
 ;;;###autoload
 (defun find-lisp-find-dired (dir regexp)
-  "Find files in DIR, matching REGEXP."
+  "Find, then dired, the files within DIR whose names match REGEXP."
   (interactive "DFind files in directory: \nsMatching regexp: ")
   (let ((find-lisp-regexp regexp))
     (find-lisp-find-dired-internal
@@ -175,34 +175,54 @@
      'find-lisp-default-directory-predicate
      "*Find Lisp Dired*")))
 
+(defun find-lisp-find-dired-other-window (dir regexp)
+  "Same as `find-lisp-find-dired', but use another window."
+  (interactive "DFind files in directory: \nsMatching regexp: ")
+  (let ((find-lisp-regexp regexp))
+    (find-lisp-find-dired-internal
+     dir
+     'find-lisp-default-file-predicate
+     'find-lisp-default-directory-predicate
+     "*Find Lisp Dired*"
+     'OTHER-WINDOW)))
+
 ;; Just the subdirectories
 ;;;###autoload
 (defun find-lisp-find-dired-subdirectories (dir)
   "Find all subdirectories of DIR."
-  (interactive "DFind subdirectories of directory: ")
+  (interactive "DDired descendent dirs of directory: ")
   (find-lisp-find-dired-internal
    dir
    'find-lisp-file-predicate-is-directory
    'find-lisp-default-directory-predicate
    "*Find Lisp Dired Subdirectories*"))
 
+;;;###autoload
+(defun find-lisp-find-dired-subdirs-other-window (dir)
+  "Same as `find-lisp-find-dired-subdirectories', but use another window."
+  (interactive "DDired descendent dirs of directory: ")
+  (find-lisp-find-dired-internal dir
+                                 'find-lisp-file-predicate-is-directory
+                                 'find-lisp-default-directory-predicate
+                                 "*Find Lisp Dired Subdirectories*"
+                                 'OTHER-WINDOW))
+
 ;; Most of this is lifted from find-dired.el
 ;;
 (defun find-lisp-find-dired-internal (dir file-predicate
-					  directory-predicate buffer-name)
-  "Run find (Lisp version) and go into Dired mode on a buffer of the output."
-  (let ((dired-buffers dired-buffers)
-	(regexp find-lisp-regexp))
-    ;; Expand DIR ("" means default-directory), and make sure it has a
-    ;; trailing slash.
+                                          directory-predicate buffer-name
+                                          &optional other-window)
+  "Run Lisp version of `find', then dired the result."
+  (let ((dired-buffers  dired-buffers)
+        (regexp         find-lisp-regexp))
+    ;; Expand DIR ("" means `default-directory'), ensuring a trailing slash.
     (setq dir (file-name-as-directory (expand-file-name dir)))
     ;; Check that it's really a directory.
     (or (file-directory-p dir)
 	(error "find-dired needs a directory: %s" dir))
-    (or
-     (and (buffer-name)
-	  (string= buffer-name (buffer-name)))
-	(switch-to-buffer (get-buffer-create buffer-name)))
+    (unless (and (buffer-name)  (string= buffer-name (buffer-name)))
+      (let ((buf  (get-buffer-create buffer-name)))
+        (if other-window (pop-to-buffer buf) (switch-to-buffer buf))))
     (widen)
     (kill-all-local-variables)
     (setq buffer-read-only nil)
@@ -278,10 +298,18 @@
   (revert-buffer))
 
 (defun find-lisp-find-dired-insert-file (file buffer)
+  "Insert line for FILE in BUFFER.
+FILE is a file or a directory name.
+If `dired-actual-switches' is non-nil in BUFFER then preserve it."
   (set-buffer buffer)
   (insert find-lisp-line-indent
-	  (find-lisp-format file (file-attributes file 'string) (list "")
-			  (current-time))))
+          (find-lisp-format
+           (propertize file 'dired-filename t) 
+           (file-attributes file 'string)
+           (or (and dired-actual-switches
+                    (split-string-and-unquote dired-actual-switches))
+               (list ""))
+           (current-time))))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; Lifted from ls-lisp. We don't want to require it, because that
@@ -289,15 +317,14 @@
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (defun find-lisp-format (file-name file-attr switches now)
-  "Format one line of long ls output for file FILE-NAME.
+  "Format one line of long `ls' output for file or directory FILE-NAME.
 FILE-ATTR and FILE-SIZE give the file's attributes and size.
 SWITCHES and TIME-INDEX give the full switch list and time data."
   (let ((file-type (file-attribute-type file-attr)))
-    (concat (if (memq ?i switches)	; inode number
-		(format "%6d " (file-attribute-inode-number file-attr)))
-	    ;; nil is treated like "" in concat
-	    (if (memq ?s switches)	; size in K
-		(format "%4d " (1+ (/ (file-attribute-size file-attr) 1024))))
+    (concat (and (memq ?i switches)	; inode number
+		 (format "%6d " (file-attribute-inode-number file-attr)))
+	    (and (memq ?s switches)	; size in K
+		 (format "%4d " (1+ (/ (file-attribute-size file-attr) 1024))))
 	    (file-attribute-modes file-attr)
 	    (format " %3d %-8s %-8s %8d "
 		    (file-attribute-link-number file-attr)
@@ -314,9 +341,10 @@
 	    (find-lisp-format-time file-attr switches now)
 	    " "
 	    file-name
-	    (if (stringp file-type)	; is a symbolic link
-		(concat " -> " file-type)
-	      "")
+            (and (eq t file-type)  (memq ?F switches)
+                 "/")                  ; Add `/' for dir if `F' switch
+	    (and (stringp file-type)
+                 (concat " -> " file-type)) ; Add " -> " for symbolic link
 	    "\n")))
 
 (defun find-lisp-time-index (switches)

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> Date: Fri, 10 Jun 2022 22:06:58 +0000
> 
> `find-lisp.el' is an old file that doesn't seem to have gotten much
> love.  It's apparently been updated just to change things like use of
> (set (make-local-variable...)) to (setq-local...).
> 
> (I wasn't aware of this library till I saw a blog post by Micky Petersen:
> https://www.masteringemacs.org/article/working-multiple-files-dired.)
> 
> The attached patch fixes a few bugs that prevent using it in some
> important ways: (1) opening a Dired buffer that explicitly lists its
> listed files or dirs (i.e. a snapshot Dired buffer, where DIRNAME is a
> cons), (2) using its listing with a `dired-actual-switches' value that
> contains switch `F', (3) properly handling a listed file or dir name
> that starts with a space char.

Thanks.

>  (defun find-lisp-find-dired-subdirectories (dir)
>    "Find all subdirectories of DIR."
> -  (interactive "DFind subdirectories of directory: ")
> +  (interactive "DDired descendent dirs of directory: ")

I find the new prompt much more cryptic than the old one.  Can this be
improved, please?  For example, would

  Find and dired subdirectories of directory:

be okay?

> -  "Run find (Lisp version) and go into Dired mode on a buffer of the output."
> -  (let ((dired-buffers dired-buffers)
> -	(regexp find-lisp-regexp))
> -    ;; Expand DIR ("" means default-directory), and make sure it has a
> -    ;; trailing slash.
> +                                          directory-predicate buffer-name
> +                                          &optional other-window)
> +  "Run Lisp version of `find', then dired the result."

Here, the existing doc string explains better what the function does.
I don't think relying too much on the assumption that "dired
SOMETHING" should be clear to users is a good idea.  One cannot find
such a verb in a dictionary, and we don't use that widely enough in
our documentation.  So explanations of that term would help clarifying
the meaning.

>  (defun find-lisp-find-dired-insert-file (file buffer)
> +  "Insert line for FILE in BUFFER.
> +FILE is a file or a directory name.
> +If `dired-actual-switches' is non-nil in BUFFER then preserve it."

What does it mean in this context to "preserve it"?  This part of the
doc string is not clear.

>  (defun find-lisp-format (file-name file-attr switches now)
> -  "Format one line of long ls output for file FILE-NAME.
> +  "Format one line of long `ls' output for file or directory FILE-NAME.

The comment says this was lifted from ls-lisp.el, but the latter has
since improved its formatting capabilities _a_lot_.  How about making
this version more like what ls-lisp.el does nowadays.  In particular,
this:

> -	    (if (stringp file-type)	; is a symbolic link
> -		(concat " -> " file-type)
> -	      "")
> +            (and (eq t file-type)  (memq ?F switches)
> +                 "/")                  ; Add `/' for dir if `F' switch
> +	    (and (stringp file-type)
> +                 (concat " -> " file-type)) ; Add " -> " for symbolic link
>  	    "\n")))

is treated much more thoroughly now in ls-lisp.el, recognizing more
file types than just symlinks and directories.  I think we should do
the same here.

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Drew Adams]

Summary:

 1. Thanks for considering making some improvements
    to `find-lisp.el', and starting with the fixes
    I suggested.

 2. I'm OK with changes you suggested.  Please do
    whatever you think helps best.

Some detailed feedback below.
___

Yes, Emacs typically uses "subdirectory" to mean a
descendent directory (i.e., any level), and not just
a child directory.  So might as well be consistent
with that.

I don't think that "sub" generally suggests such a
meaning, however.  It's typically ambiguous whether
it means child or descendant - which, in effect,
means that it means descendant.

When appropriate we'd be better off using "parent"
and "child" or "ancestor" and "descendant".

But yes, it's no doubt too late to change, and Emacs
is certainly not going to change this everywhere.

Perhaps we could start to rename places (e.g. doc,
if not fun/var names) where we really mean children
to use "child" and "children" instead of "sub___".
Maybe we consistently do that already; dunno.  
___

The existing prompts could be improved, IMO,
by quoting the command names `find' and `ls',
rather than just using them as words.  And I
think "Run Lisp version of `find'..." is
clearer than "Run find (Lisp version)...".

I agree about not using "dired" as a verb, at
least in the first line of a doc string.  I was
trying to avoid a too-long first line.  I'm OK
with whatever you do here.
___

> > +If `dired-actual-switches' is non-nil in BUFFER then preserve it."
> What does it mean in this context to "preserve it"?  

I don't have a good suggestion for the wording,
and I don't insist that the point be communicated
in the doc string.

The point is that `dired-actual-switches', and not
"", is used as the starting point.

The problem with not making this fix is that the
code inserts subdir names without trailing "/",
and yet if `dired-actual-switches' (e.g. from
`dired-listing-switches') contains switch `F' then
functions such as `dired-move-to-end-of-filename'
don't work, because they're off-by-one for dir
names - e.g.:

(and (dired-check-switches dired-actual-switches
                           "F" "classify") ; used-F 
     (or (memq file-type '(?d ?s ?p)) executable)
     (forward-char -1))

> The comment says this was lifted from ls-lisp.el,
> but the latter has since improved its formatting
> capabilities _a_lot_.  How about making this
> version more like what ls-lisp.el does nowadays.

Please do whatever's right here.  I didn't write
that comment, and I didn't try to bring `find-lisp'
completely up-to-date.  I'm quite sure there are
lots of ways the library could/should be improved.

Perhaps the `ls-lisp' code takes care of the `F'
bug.  If not, that should be done in `find-lisp'.

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "55894@debbugs.gnu.org" <55894@debbugs.gnu.org>
> Date: Sat, 11 Jun 2022 16:46:35 +0000
> 
> Summary:
> 
>  1. Thanks for considering making some improvements
>     to `find-lisp.el', and starting with the fixes
>     I suggested.
> 
>  2. I'm OK with changes you suggested.  Please do
>     whatever you think helps best.

Does this mean you won't be submitting a modified patch which takes
into account the comments?

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Drew Adams]

> > Summary:
> >
> >  1. Thanks for considering making some improvements
> >     to `find-lisp.el', and starting with the fixes
> >     I suggested.
> >
> >  2. I'm OK with changes you suggested.  Please do
> >     whatever you think helps best.
> 
> Does this mean you won't be submitting a modified patch which takes
> into account the comments?

I guess so, yes.

I pointed out a couple of bugs.  And I suggested
fixes for those bugs, as a patch.  That should at
least make clear what those problems are and serve
as food for thought in case you want to fix them.

I'm OK with your making whatever doc-string changes
you like.

I'm OK with your integrating `ls-lisp' code or
otherwise updating `find-lisp' to take advantage of
`ls-lisp' code.  That would be welcome.

My overall request is to fix the bugs I pointed to.
I made a suggestion as to how they can be fixed.
HTH.

I'm also OK with your doing nothing.  In that case,
I'll have to create a file (e.g. `find-lisp+.el')
with those fixes, for `find-lisp' to work with my
other code (e.g. `dired+.el' commands that make
snapshot Dired buffers from `find' listings (e.g.
from `find-(dired|lisp)').

(I could add such a file anyway, for use with older
Emacs versions, but I'm OK with foregoing use of
`find-lisp' with Emacs versions prior to whatever
Emacs release you might fix this for.)

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

>> Does this mean you won't be submitting a modified patch which takes
>> into account the comments?
>
> I guess so, yes.
[...]
> I'm also OK with your doing nothing.  In that case,
> I'll have to create a file (e.g. `find-lisp+.el')
> with those fixes, for `find-lisp' to work with my
> other code (e.g. `dired+.el' commands that make
> snapshot Dired buffers from `find' listings (e.g.
> from `find-(dired|lisp)').

Why would you do all that work instead of just submitting a modified
patch?

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Drew Adams]

> >> Does this mean you won't be submitting a modified
> >> patch which takes into account the comments?
> >
> > I guess so, yes.
> [...]
> > I'm also OK with your doing nothing.  In that case,
> > I'll have to create a file (e.g. `find-lisp+.el')
> > with those fixes, for `find-lisp' to work with my
> > other code (e.g. `dired+.el' commands that make
> > snapshot Dired buffers from `find' listings (e.g.
> > from `find-(dired|lisp)').
> 
> Why would you do all that work instead of just
> submitting a modified patch?

I'm not promising to do any such work.

Why would you waste your precious time sending
such an email, instead of just fixing the bug?
Is it that you want to argue?  Why make this
about me, and not the bug?

1. I'm an Emacs user.  I reported a bug.
2. I even suggested fixes for it, showing code.
   That's a suggestion.  The reported problems
   are not a suggestion - they're problems.
3. I said I'm fine with whatever doc-string
   changes someone might want to make.
   Not that it matters whether I'm fine with
   that.  Just politely letting you know: please
   feel free to go ahead and fix the bug however
   you like.

No good deed goes unpunished, apparently.  Forget
the suggested fixes, if you like.  Concentrate on
the bug, please.  Thx.

It's Emacs's bug.

Whether I want to bother to work-around the bug
locally is up to me.  Please don't worry about
whether or why I might or might not want to.
Not your problem.

But I can say that I prefer that Emacs itself
be fixed, for the benefit of all.  If you don't
want to fix it, I can adjust.  It won't be the
first (or last) time.  Not a big deal.

It's a minor bug in a minor library that no one
has paid attention to for quite a while.  One
reason for filing the bug was to draw attention
to the library, which could be made more useful.
Or not.

At ease.

bug#55894: 26.3; [PATCH] `find-lisp.el' bugs

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> The attached patch fixes a few bugs that prevent using it in some
>> important ways: (1) opening a Dired buffer that explicitly lists its
>> listed files or dirs (i.e. a snapshot Dired buffer, where DIRNAME is a
>> cons), (2) using its listing with a `dired-actual-switches' value that
>> contains switch `F', (3) properly handling a listed file or dir name
>> that starts with a space char.
>
> Thanks.

I've now pushed Drew's patch, adjusting for Eli's comments (and I did
some other tweaks to the doc strings) to Emacs 29.