Find relative filenames recursively in Emacs

I have a set of directories. For each directory I have a set of relative file names. I would like to select any of these relative filenames using completion, and then later reconstruct the absolute path of the filename.

In order to create a list of relative filenames for a given directory, I created this function:

(defun my-get-dir-files (dir pat &optional rel-dir)
"Finds relative filenames. Returns a list of relative filenames/pathnames to
directory dir' matching pattern pat'. The variable rel-dir is only used internally
by recursive calls and should usually not be supplied.

IMPORTANT: This function uses recursion. To avoid excessive copying of lists,
the variable files' is used as a dynamic list variable, and is assumed to be
defined as non-lexical dynamic variable in the caller using a let binding"
(let ((dir-files nil)
(cur-dir nil))
(unless rel-dir (setq rel-dir ""))
(setq cur-dir (expand-file-name rel-dir dir))
(setq dir-files (directory-files cur-dir nil nil t))
(dolist (name dir-files)
(unless (or (string= name ".") (string= name ".."))
(let ((fn (expand-file-name name cur-dir)))
(if (file-directory-p fn)
(let ((new-rel-dir nil))
(if (string= "" rel-dir)
(setq new-rel-dir name)
(setq new-rel-dir (concat rel-dir "/" name)))
(my-get-dir-files dir pat new-rel-dir)) ;; recursive call
(when (string-match pat name)
(let ((rel-fn nil))
(if (string= "" rel-dir)
(setq rel-fn name)
(setq rel-fn (concat rel-dir "/" name))
(setq files (append files (,rel-fn))))))))))))


Then I plan to use code like:

(let ((dir "/home/hakon/dir1")
(files nil))
(my-get-dir-files dir "\\.el$")
files)


to set the list variable files.

One thing that worries me here is the use of the (non-lexical) dynamic variable files. Can or sho

Yeah don't use a dynamic variable for that, it's just confusing for the
user.

It is way easier to deal with input and output from a
function instead of having to take special variables into account, basically it's now part of your interface. And I personally
would be really angry if a random function somewhere took a common
variable like files from outside its scope and filled it with data.

So, after that explanation, I would recommend using either a separate
function, or cl-flet/cl-labels to handle the recursive part and only
expose the necessary parts to the caller. That means no optional
rel-dir, which isn't really part of the interface and no using files
from the outer scope.

To wit:

(defun my-get-dir-files (dir pat)
  "..."
  (let (files)
    (cl-flet ((aux (dir pat rel-dir)
                    (let ((dir-files nil)
                          (cur-dir nil))
                      (unless rel-dir (setq rel-dir ""))
                      (setq cur-dir (expand-file-name rel-dir dir))
                      (setq dir-files (directory-files cur-dir nil nil t))
                      (dolist (name dir-files)
                        (unless (or (string= name ".") (string= name ".."))
                          (let ((fn (expand-file-name name cur-dir)))
                            (if (file-directory-p fn)
                                (let ((new-rel-dir nil))
                                  (if (string= "" rel-dir)
                                      (setq new-rel-dir name)
                                    (setq new-rel-dir (concat rel-dir "/" name)))
                                  (aux dir pat new-rel-dir)) ;; recursive call
                              (when (string-match pat name)
                                (let ((rel-fn nil))
                                  (if (string= "" rel-dir)
                                      (setq rel-fn name)
                                    (setq rel-fn (concat rel-dir "/" name))
                                    (setq files (append files `(,rel-fn)))))))))))))
      (aux dir pat nil)
      files)))

Note that I just wrapped the existing function one time and put a let
around it to keep the side effect more contained. And now that files
is lexical, everything is fine.

But really, it would be better to just use an accumulator. You
only ever add one element to the list, in which case you should just
immediately use push instead (and if you care about the output order,
nreverse the final result).

The following changes could also be made later:

• Use member instead of the or, less typing for you.

• (Short variable names don't necessarily improve readability. Even

though Elisp code seems to disagree in places.)

• The explicit binding to nil isn't necessary as it's the default

anyway.

• Also the setqs there are unnecessary, just put the initial binding

into the let instead, use or and other functional tools.

• cond is more readable for nested conditions, if applicable.

• I would use list for simple list constructions like (,foo).

• (setq rel-fn name) in the if doesn't do anything as it's the only form in the true branch.

So with all that it would be like this (even though personally I'd
rename the fn to filename as well. YMMV:

(defun my-get-dir-files (dir pat)
  (let (files)
    (cl-flet ((aux (dir path rel-dir)
                   (let ((cur-dir (expand-file-name rel-dir dir)))
                     (dolist (name (directory-files cur-dir nil nil t))
                       (unless (member name '("." ".."))
                         (let ((fn (expand-file-name name cur-dir)))
                           (cond
                            ((file-directory-p fn)
                             (aux dir pat (if (string= "" rel-dir)
                                              name
                                            (concat rel-dir "/" name))))
                            ((string-match pat name)
                             (unless (string= "" rel-dir)
                               (push (concat rel-dir "/" name) files))))))))))
      (aux dir pat "")
      files)))

I'm pretty sure all transformations kept the meaning the same; a quick
check via

(set-exclusive-or (let (files) (my-get-dir-files-1 ".." ".*") files) (my-get-dir-files ".." ".*") :test #'equal)

with -1 being the original function resulted in no difference in the
lists (except for order of course), so that was enough for me. If you
return (nreverse files) instead the lists are exactly equal.

And at this point I'm done. There might still be potential
optimisations during the concatenations and so on. The recursion itself here is fine as you won't encounter directory trees deep enough to cause the recursion to fail and it's also fairly efficient now, so no need to change anything there.

You could still extract a local function for (if (string= "" rel-dir) ...) and use that there and in the other branch o

StackExchange Code Review Q#79439, answer score: 3