Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Stefan Monnier]

>     Ignore directory symlinks in directory-files-recursively

Could you work on merging file-tree-walk and
directory-files-recursively?


        Stefan

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     Ignore directory symlinks in directory-files-recursively
>
> Could you work on merging file-tree-walk and
> directory-files-recursively?

As you pointed out, `file-tree-walk' has the nice property that it can
stop recursing based on a predicate, so implementing it on
`directory-files-recursively' would not be optimal.

On the other hand, the interface for `file-tree-walk' is not what people
usually want -- they just want a list of matching files.  I grepped for
"defun.*recurs" and there's a lot of functions basically implementing
`directory-files-recursively'.  Here are the first four:

cedet-files-list-recursively
find-lisp-find-files
gnus-recursive-directory-files
nnfolder-recursive-directory-files

Doing it the other way around would almost be possible, but
`directory-files-recursively' guarantees that leaves are returned before
directories so that implementing "rm -r" is just a `mapc' over the
returned values.  And `file-tree-walk' doesn't call the callback
function in that order, so it's fiddly.

Besides, we don't have copyright paperwork for `file-tree-walk'.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Stefan Monnier]

> As you pointed out, `file-tree-walk' has the nice property that it can
> stop recursing based on a predicate, so implementing it on
> `directory-files-recursively' would not be optimal.

Right, what is needed is to extend directory-files-recursively such that
it can also use a predicate to decide whether to recurse or not.


        Stefan

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> As you pointed out, `file-tree-walk' has the nice property that it can
>> stop recursing based on a predicate, so implementing it on
>> `directory-files-recursively' would not be optimal.
>
> Right, what is needed is to extend directory-files-recursively such that
> it can also use a predicate to decide whether to recurse or not.

file-tree-walk uses the same function for the predicate and for the
... processing function.  So that would be something like the patch
included below.

The interface isn't the same, though.  I think we should perhaps ignore
the non-lexical case and get rid of the ARGS argument.  Supplying a
lexical lambda predicate just makes more sense.

And I don't like passing in the DIR and LEAF separately.  To check
whether the argument is a directory, the predicate can just use
`directory-name-p' .

diff --git a/lisp/files.el b/lisp/files.el
index d55ef1a..14be6c2 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -766,11 +766,17 @@ prevented.  Directory entries are sorted with string-lessp."
   (and (> (length name) 0)
        (char-equal (aref name (1- (length name))) ?/)))
 
-(defun directory-files-recursively (dir match &optional include-directories)
+(defun directory-files-recursively (dir match &optional
+					include-directories predicate)
   "Return all files under DIR that have file names matching MATCH (a regexp).
 This function works recursively.  Files are returned in \"depth first\"
 and alphabetical order.
-If INCLUDE-DIRECTORIES, also include directories that have matching names."
+If INCLUDE-DIRECTORIES, also include directories that have matching names.
+
+If PREDICATE, this should be a function that will be called on
+each matching file and on all directories.  When called on a
+directory, the predicate should return non-nil if the directory
+should be descended into."
   (let ((result nil)
 	(files nil))
     (dolist (file (sort (file-name-all-completions "" dir)
@@ -780,13 +786,17 @@ If INCLUDE-DIRECTORIES, also include directories that have matching names."
 	    (let* ((leaf (substring file 0 (1- (length file))))
 		   (path (expand-file-name leaf dir)))
 	      ;; Don't follow symlinks to other directories.
-	      (unless (file-symlink-p path)
+	      (when (and (not (file-symlink-p path))
+			 (or (null predicate)
+			     (funcall predicate path)))
 		(setq result (nconc result (directory-files-recursively
 					    path match include-directories))))
 	      (when (and include-directories
 			 (string-match match leaf))
 		(setq result (nconc result (list path)))))
 	  (when (string-match match file)
+	    (when predicate
+	      (funcall predicate (expand-file-name file dir)))
 	    (push (expand-file-name file dir) files)))))
     (nconc result (nreverse files))))
 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Stefan Monnier]

> The interface isn't the same, though.  I think we should perhaps ignore
> the non-lexical case and get rid of the ARGS argument.  Supplying a
> lexical lambda predicate just makes more sense.

Agreed.

> And I don't like passing in the DIR and LEAF separately.

It can avoid calling expand-file-name only to call file-name-directory
(or file-name-nondirectory) right after.

> -(defun directory-files-recursively (dir match &optional include-directories)
> +(defun directory-files-recursively (dir match &optional
> +					include-directories predicate)

Can we reduce the number of arguments?

>    "Return all files under DIR that have file names matching MATCH (a regexp).

Please specify more precisely how MATCH is used (whether it's compared
to the "leaf" only, or the full absolute file name, or ...).

>  		   (path (expand-file-name leaf dir)))

This is not a "path", it's a file name.  Remember: by convention
a "path" is a list of directories, as in load-path, $PATH, etc...


        Stefan

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> And I don't like passing in the DIR and LEAF separately.
>
> It can avoid calling expand-file-name only to call file-name-directory
> (or file-name-nondirectory) right after.

It can, but in the use cases I've typically seen, you wouldn't do that,
I think?  You'd say `(directory-name-p file)' and then...  something...

>> -(defun directory-files-recursively (dir match &optional include-directories)
>> +(defun directory-files-recursively (dir match &optional
>> +					include-directories predicate)
>
> Can we reduce the number of arguments?

That would be nice, but I'm not sure what to remove.  The canonical,
that most of the callers use (from the other versions of this function
included in various packages in Emacs) is

(directory-files-recursively "~/" "~\\'")

or something.  And then there are a couple that want the directories,
too.  And then there's `file-tree-walk', which is the new thing.

Uhm...  we could make MATCH be the predicate if it's not a string?

>>  		   (path (expand-file-name leaf dir)))
>
> This is not a "path", it's a file name.  Remember: by convention
> a "path" is a list of directories, as in load-path, $PATH, etc...

Right.  But do we have a word for "string that designates either a file
or a directory"?  That's usually called "a path"...

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Stefan Monnier]

>> It can avoid calling expand-file-name only to call file-name-directory
>> (or file-name-nondirectory) right after.
> It can, but in the use cases I've typically seen, you wouldn't do that,
> I think?  You'd say `(directory-name-p file)' and then...  something...

I don't understand what you mean here.  I'm just pointing out that
calling expand-file-name only to pass the result to the function
parameter means that this work may be wasted.

So passing the two parameters separately is a way to be more lazy.

>> Can we reduce the number of arguments?
> That would be nice, but I'm not sure what to remove.  The canonical,
> that most of the callers use (from the other versions of this function
> included in various packages in Emacs) is
> (directory-files-recursively "~/" "~\\'")
> or something.  And then there are a couple that want the directories,
> too.  And then there's `file-tree-walk', which is the new thing.

Exactly: the use cases are varied, so we need a very generic interface.
I actually find file-tree-walk not too bad in this respect.

> Uhm...  we could make MATCH be the predicate if it's not a string?

But we need 2 answers: whether to include the file in the result, and
whether to recurse.

>>> (path (expand-file-name leaf dir)))
>> This is not a "path", it's a file name.  Remember: by convention
>> a "path" is a list of directories, as in load-path, $PATH, etc...
> Right.  But do we have a word for "string that designates either a file
> or a directory"?

Yes, we say "a file name".  A directory is a kind of file.

> That's usually called "a path"...

No, there are people who like to use "path" for other things, but the
GNU convention is to only use it for "a list of directories, as in
load-path, $PATH, etc...".


        Stefan

Re: [Emacs-diffs] master 3431e82: Ignore directory symlinks in directory-files-recursively

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> It can avoid calling expand-file-name only to call file-name-directory
>>> (or file-name-nondirectory) right after.
>> It can, but in the use cases I've typically seen, you wouldn't do that,
>> I think?  You'd say `(directory-name-p file)' and then...  something...
>
> I don't understand what you mean here.  I'm just pointing out that
> calling expand-file-name only to pass the result to the function
> parameter means that this work may be wasted.
>
> So passing the two parameters separately is a way to be more lazy.

The typical usage (in the callers that call versions of this function
other places in the Emacs tree) seems to be "give me a list of files
matching this regexp".  Making the callers take a separate directory and
leaf name makes the caller do the work of joining those two things up
before anything can be done, so it's more lazy to have the
`directory-files-recursively' do this work.

If there are callers that also want directories, they will have to do
more work, but that extra work is just calling `directory-name-p', which
is a very cheap function.

So I don't really see any advantage to having two parameters in the
callback function.

>>> Can we reduce the number of arguments?
>> That would be nice, but I'm not sure what to remove.  The canonical,
>> that most of the callers use (from the other versions of this function
>> included in various packages in Emacs) is
>> (directory-files-recursively "~/" "~\\'")
>> or something.  And then there are a couple that want the directories,
>> too.  And then there's `file-tree-walk', which is the new thing.
>
> Exactly: the use cases are varied, so we need a very generic interface.
> I actually find file-tree-walk not too bad in this respect.
>
>> Uhm...  we could make MATCH be the predicate if it's not a string?
>
> But we need 2 answers: whether to include the file in the result, and
> whether to recurse.

Er, huhm, yes, I don't know what I was thinking.

Then I don't really see any practical way to reduce the number of
parameters below four.  We could drop INCLUDE-DIRECTORIES, but then the
somewhat handy `(directory-files-recursively "/tmp/foo" "." t)' (to do
`rm r') wouldn't be as handy any more.

>> Right.  But do we have a word for "string that designates either a file
>> or a directory"?
>
> Yes, we say "a file name".  A directory is a kind of file.

Ok; I'll rename away.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no