Find relative filenames recursively in Emacs

Question

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 should this be avoided?

According to the manual I have violated this principle:

If a variable has no global definition, use it as a local variable only within a binding construct

Answer 1

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 of the cond.

Next up: Would be cool if the function also handled "." for dir.