* bug#56227: 29.0.50; Eshell globs ending with '/' should match directories only
2022-06-26 1:45 bug#56227: 29.0.50; Eshell globs ending with '/' should match directories only Jim Porter
@ 2022-06-26 1:52 ` Jim Porter
2022-06-26 14:52 ` Lars Ingebrigtsen
0 siblings, 1 reply; 3+ messages in thread
From: Jim Porter @ 2022-06-26 1:52 UTC (permalink / raw)
To: 56227
[-- Attachment #1: Type: text/plain, Size: 772 bytes --]
On 6/25/2022 6:45 PM, Jim Porter wrote:
> For consistency with other shells, and to make Eshell's globbing more
> expressive, I think we should make the latter match directories only.
> While this technically changes an existing behavior, it should be a safe
> change to make, since there's currently no reason I'm aware of to add a
> trailing slash to an Eshell glob.
Here's the patch. The first part refactors em-glob.el a bit, and
converts globs into regexps ahead of time. This is partly to make the
second patch simpler, but also improves performance by a few percent
when `eshell-glob-entries' examines many directories: it no longer
repeatedly converts the same glob component for each sibling directory.
The second part is the actual patch for this bug.
[-- Attachment #2: 0001-Convert-Eshell-globs-ahead-of-time-instead-of-doing-.patch --]
[-- Type: text/plain, Size: 11168 bytes --]
From 5b1095e56ff91e4889ea93fd9782f6a432aa4c61 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 24 Jun 2022 08:39:42 -0700
Subject: [PATCH 1/2] Convert Eshell globs ahead of time instead of doing it
repeatedly
* lisp/eshell/em-glob.el (eshell-glob-recursive): New variable.
(eshell-glob-convert-1, eshell-glob-convert): New functions.
(eshell-extended-glob): Use 'eshell-glob-convert'.
(eshell-glob-entries): Adapt function to use pre-converted globs.
* test/lisp/eshell-em-glob-tests.el (em-glob-test/match-dot-files):
New test.
---
lisp/eshell/em-glob.el | 204 +++++++++++++++++-------------
test/lisp/eshell/em-glob-tests.el | 15 +++
2 files changed, 129 insertions(+), 90 deletions(-)
diff --git a/lisp/eshell/em-glob.el b/lisp/eshell/em-glob.el
index 52531ff893..8acdaee233 100644
--- a/lisp/eshell/em-glob.el
+++ b/lisp/eshell/em-glob.el
@@ -183,6 +183,10 @@ eshell-glob-chars-regexp
(defvar eshell-glob-matches)
(defvar message-shown)
+(defvar eshell-glob-recursive-alist
+ '(("**/" . recurse)
+ ("***/" . recurse-symlink)))
+
(defun eshell-glob-regexp (pattern)
"Convert glob-pattern PATTERN to a regular expression.
The basic syntax is:
@@ -232,6 +236,74 @@ eshell-glob-regexp
(regexp-quote (substring pattern matched-in-pattern))
"\\'")))
+(defun eshell-glob-convert-1 (glob &optional last)
+ "Convert a GLOB matching a single element of a file name to regexps.
+If LAST is non-nil, this glob is the last element of a file name.
+
+The result is a pair of regexps, the first for file names to
+include, and the second for ones to exclude."
+ (let ((len (length glob)) (index 1) (incl glob) excl)
+ ;; We can't use `directory-file-name' because it strips away text
+ ;; properties in the string.
+ (let ((last (1- (length incl))))
+ (when (eq (aref incl last) ?/)
+ (setq incl (substring incl 0 last))))
+ ;; Split the glob if it contains a negation like x~y.
+ (while (and (eq incl glob)
+ (setq index (string-search "~" glob index)))
+ (if (or (get-text-property index 'escaped glob)
+ (or (= (1+ index) len)))
+ (setq index (1+ index))
+ (setq incl (substring glob 0 index)
+ excl (substring glob (1+ index)))))
+ (setq incl (eshell-glob-regexp incl)
+ excl (and excl (eshell-glob-regexp excl)))
+ ;; Exclude dot files if requested.
+ (if (or eshell-glob-include-dot-files
+ (eq (aref glob 0) ?.))
+ (unless (or eshell-glob-include-dot-dot
+ (not last))
+ (setq excl (if excl
+ (concat "\\(\\`\\.\\.?\\'\\|" excl "\\)")
+ "\\`\\.\\.?\\'")))
+ (setq excl (if excl
+ (concat "\\(\\`\\.\\|" excl "\\)")
+ "\\`\\.")))
+ (cons incl excl)))
+
+(defun eshell-glob-convert (glob)
+ "Convert an Eshell glob-pattern GLOB to regexps.
+The result is a list, where the first element is the base
+directory to search in, and the second is a list containing
+elements of the following forms:
+
+* Regexp pairs as generated by `eshell-glob-convert-1'.
+
+* `recurse', indicating that searches should recurse into
+ subdirectories.
+
+* `recurse-symlink', like `recurse', but also following symlinks."
+ (let ((globs (eshell-split-path glob))
+ start-dir result last-saw-recursion)
+ (if (and (cdr globs)
+ (file-name-absolute-p (car globs)))
+ (setq start-dir (car globs)
+ globs (cdr globs))
+ (setq start-dir "."))
+ (while globs
+ (if-let ((recurse (cdr (assoc (car globs)
+ eshell-glob-recursive-alist))))
+ (if last-saw-recursion
+ (setcar result recurse)
+ (push recurse result)
+ (setq last-saw-recursion t))
+ (push (eshell-glob-convert-1 (car globs) (null (cdr globs)))
+ result)
+ (setq last-saw-recursion nil))
+ (setq globs (cdr globs)))
+ (list (file-name-as-directory start-dir)
+ (nreverse result))))
+
(defun eshell-extended-glob (glob)
"Return a list of files matched by GLOB.
If no files match, signal an error (if `eshell-error-if-no-glob'
@@ -247,14 +319,10 @@ eshell-extended-glob
Mainly they are not supported because file matching is done with Emacs
regular expressions, and these cannot support the above constructs."
- (let ((paths (eshell-split-path glob))
+ (let ((globs (eshell-glob-convert glob))
eshell-glob-matches message-shown)
(unwind-protect
- (if (and (cdr paths)
- (file-name-absolute-p (car paths)))
- (eshell-glob-entries (file-name-as-directory (car paths))
- (cdr paths))
- (eshell-glob-entries (file-name-as-directory ".") paths))
+ (apply #'eshell-glob-entries globs)
(if message-shown
(message nil)))
(or (and eshell-glob-matches (sort eshell-glob-matches #'string<))
@@ -263,94 +331,50 @@ eshell-extended-glob
glob))))
;; FIXME does this really need to abuse eshell-glob-matches, message-shown?
-(defun eshell-glob-entries (path globs &optional recurse-p)
- "Glob the entries in PATH, possibly recursing if RECURSE-P is non-nil."
+(defun eshell-glob-entries (path globs)
+ "Match the entries in PATH against GLOBS.
+GLOBS is a list of globs as converted by `eshell-glob-convert',
+which see."
(let* ((entries (ignore-errors
- (file-name-all-completions "" path)))
- (case-fold-search eshell-glob-case-insensitive)
- (glob (car globs))
- (len (length glob))
- dirs rdirs
- incl excl
- name isdir pathname)
- (while (cond
- ((and (= len 3) (equal glob "**/"))
- (setq recurse-p 2
- globs (cdr globs)
- glob (car globs)
- len (length glob)))
- ((and (= len 4) (equal glob "***/"))
- (setq recurse-p 3
- globs (cdr globs)
- glob (car globs)
- len (length glob)))))
- (if (and recurse-p (not glob))
- (error "`**/' cannot end a globbing pattern"))
- (let ((index 1))
- (setq incl glob)
- (while (and (eq incl glob)
- (setq index (string-search "~" glob index)))
- (if (or (get-text-property index 'escaped glob)
- (or (= (1+ index) len)))
- (setq index (1+ index))
- (setq incl (substring glob 0 index)
- excl (substring glob (1+ index))))))
- ;; can't use `directory-file-name' because it strips away text
- ;; properties in the string
- (let ((len (1- (length incl))))
- (if (eq (aref incl len) ?/)
- (setq incl (substring incl 0 len)))
- (when excl
- (setq len (1- (length excl)))
- (if (eq (aref excl len) ?/)
- (setq excl (substring excl 0 len)))))
- (setq incl (eshell-glob-regexp incl)
- excl (and excl (eshell-glob-regexp excl)))
- (if (or eshell-glob-include-dot-files
- (eq (aref glob 0) ?.))
- (unless (or eshell-glob-include-dot-dot
- (cdr globs))
- (setq excl (if excl
- (concat "\\(\\`\\.\\.?\\'\\|" excl "\\)")
- "\\`\\.\\.?\\'")))
- (setq excl (if excl
- (concat "\\(\\`\\.\\|" excl "\\)")
- "\\`\\.")))
+ (file-name-all-completions "" path)))
+ (case-fold-search eshell-glob-case-insensitive)
+ glob glob-remainder recurse-p)
+ (if (rassq (car globs) eshell-glob-recursive-alist)
+ (setq recurse-p (car globs)
+ glob (cadr globs)
+ glob-remainder (cddr globs))
+ (setq glob (car globs)
+ glob-remainder (cdr globs)))
(when (and recurse-p eshell-glob-show-progress)
(message "Building file list...%d so far: %s"
- (length eshell-glob-matches) path)
+ (length eshell-glob-matches) path)
(setq message-shown t))
- (if (equal path "./") (setq path ""))
- (while entries
- (setq name (car entries)
- len (length name)
- isdir (eq (aref name (1- len)) ?/))
- (if (let ((fname (directory-file-name name)))
- (and (not (and excl (string-match excl fname)))
- (string-match incl fname)))
- (if (cdr globs)
- (if isdir
- (setq dirs (cons (concat path name) dirs)))
- (setq eshell-glob-matches
- (cons (concat path name) eshell-glob-matches))))
- (if (and recurse-p isdir
- (or (> len 3)
- (not (or (and (= len 2) (equal name "./"))
- (and (= len 3) (equal name "../")))))
- (setq pathname (concat path name))
- (not (and (= recurse-p 2)
- (file-symlink-p
- (directory-file-name pathname)))))
- (setq rdirs (cons pathname rdirs)))
- (setq entries (cdr entries)))
- (setq dirs (nreverse dirs)
- rdirs (nreverse rdirs))
- (while dirs
- (eshell-glob-entries (car dirs) (cdr globs))
- (setq dirs (cdr dirs)))
- (while rdirs
- (eshell-glob-entries (car rdirs) globs recurse-p)
- (setq rdirs (cdr rdirs)))))
+ (when (equal path "./") (setq path ""))
+ (let ((incl (car glob))
+ (excl (cdr glob))
+ dirs rdirs)
+ (dolist (name entries)
+ (let* ((len (length name))
+ (isdir (eq (aref name (1- len)) ?/))
+ pathname)
+ (when (let ((fname (directory-file-name name)))
+ (and (not (and excl (string-match excl fname)))
+ (string-match incl fname)))
+ (if glob-remainder
+ (when isdir
+ (push (concat path name) dirs))
+ (push (concat path name) eshell-glob-matches)))
+ (when (and recurse-p isdir
+ (not (member name '("./" "../")))
+ (setq pathname (concat path name))
+ (not (and (eq recurse-p 'recurse)
+ (file-symlink-p
+ (directory-file-name pathname)))))
+ (push pathname rdirs))))
+ (dolist (dir (nreverse dirs))
+ (eshell-glob-entries dir glob-remainder))
+ (dolist (rdir (nreverse rdirs))
+ (eshell-glob-entries rdir globs)))))
(provide 'em-glob)
diff --git a/test/lisp/eshell/em-glob-tests.el b/test/lisp/eshell/em-glob-tests.el
index 9976b32ffe..65f340a8da 100644
--- a/test/lisp/eshell/em-glob-tests.el
+++ b/test/lisp/eshell/em-glob-tests.el
@@ -160,6 +160,21 @@ em-glob-test/match-x-but-not-y
(should (equal (eshell-extended-glob "[[:digit:]]##~4?")
'("1" "12" "123")))))
+(ert-deftest em-glob-test/match-dot-files ()
+ "Test that dot files are matched correctly."
+ (with-fake-files '("foo.el" ".emacs")
+ (should (equal (eshell-extended-glob ".*")
+ '("../" "./" ".emacs")))
+ (let (eshell-glob-include-dot-dot)
+ (should (equal (eshell-extended-glob ".*")
+ '(".emacs"))))
+ (let ((eshell-glob-include-dot-files t))
+ (should (equal (eshell-extended-glob "*")
+ '("../" "./" ".emacs" "foo.el")))
+ (let (eshell-glob-include-dot-dot)
+ (should (equal (eshell-extended-glob "*")
+ '(".emacs" "foo.el")))))))
+
(ert-deftest em-glob-test/no-matches ()
"Test behavior when a glob fails to match any files."
(with-fake-files '("foo.el" "bar.el")
--
2.25.1
[-- Attachment #3: 0002-Make-Eshell-globs-ending-in-match-directories-only.patch --]
[-- Type: text/plain, Size: 7295 bytes --]
From 0247fc4187db5c15668deff1ca67a0566031014d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 25 Jun 2022 18:19:01 -0700
Subject: [PATCH 2/2] Make Eshell globs ending in "/" match directories only
* lisp/eshell/em-glob.el (eshell-glob-convert): Return whether to
match directories only.
(eshell-glob-entries): Add ONLY-DIRS argument.
* test/lisp/eshell/em-glob-tests.el
(em-glob-test/match-any-directory): New test.
(em-glob-test/match-recursive)
(em-glob-test/match-recursive-follow-symlinks): Add test cases for
when "**/" or "***/" are the last components in a glob.
* etc/NEWS: Announce this change (bug#56227).
---
etc/NEWS | 6 +++++
lisp/eshell/em-glob.el | 45 +++++++++++++++++++++----------
test/lisp/eshell/em-glob-tests.el | 15 +++++++++--
3 files changed, 50 insertions(+), 16 deletions(-)
diff --git a/etc/NEWS b/etc/NEWS
index 40658559d7..32a780129c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1817,6 +1817,12 @@ values passed as a single token, such as '-oVALUE' or
'eshell-eval-using-options' macro. See "Defining new built-in
commands" in the "(eshell) Built-ins" node of the Eshell manual.
+---
+*** Eshell globs ending with '/' now match only directories.
+Additionally, globs ending with '**/' or '***/' no longer raise an
+error, and now expand to all directories recursively (following
+symlinks in the latter case).
+
** Shell
---
diff --git a/lisp/eshell/em-glob.el b/lisp/eshell/em-glob.el
index 8acdaee233..58b7a83c09 100644
--- a/lisp/eshell/em-glob.el
+++ b/lisp/eshell/em-glob.el
@@ -273,17 +273,23 @@ eshell-glob-convert-1
(defun eshell-glob-convert (glob)
"Convert an Eshell glob-pattern GLOB to regexps.
-The result is a list, where the first element is the base
-directory to search in, and the second is a list containing
-elements of the following forms:
+The result is a list of three elements:
-* Regexp pairs as generated by `eshell-glob-convert-1'.
+1. The base directory to search in.
-* `recurse', indicating that searches should recurse into
- subdirectories.
+2. A list containing elements of the following forms:
-* `recurse-symlink', like `recurse', but also following symlinks."
+ * Regexp pairs as generated by `eshell-glob-convert-1'.
+
+ * `recurse', indicating that searches should recurse into
+ subdirectories.
+
+ * `recurse-symlink', like `recurse', but also following
+ symlinks.
+
+3. A boolean indicating whether to match directories only."
(let ((globs (eshell-split-path glob))
+ (isdir (eq (aref glob (1- (length glob))) ?/))
start-dir result last-saw-recursion)
(if (and (cdr globs)
(file-name-absolute-p (car globs)))
@@ -302,7 +308,8 @@ eshell-glob-convert
(setq last-saw-recursion nil))
(setq globs (cdr globs)))
(list (file-name-as-directory start-dir)
- (nreverse result))))
+ (nreverse result)
+ isdir)))
(defun eshell-extended-glob (glob)
"Return a list of files matched by GLOB.
@@ -331,17 +338,21 @@ eshell-extended-glob
glob))))
;; FIXME does this really need to abuse eshell-glob-matches, message-shown?
-(defun eshell-glob-entries (path globs)
+(defun eshell-glob-entries (path globs only-dirs)
"Match the entries in PATH against GLOBS.
GLOBS is a list of globs as converted by `eshell-glob-convert',
-which see."
+which see.
+
+If ONLY-DIRS is non-nil, only match directories; otherwise, match
+directories and files."
(let* ((entries (ignore-errors
(file-name-all-completions "" path)))
(case-fold-search eshell-glob-case-insensitive)
glob glob-remainder recurse-p)
(if (rassq (car globs) eshell-glob-recursive-alist)
(setq recurse-p (car globs)
- glob (cadr globs)
+ glob (or (cadr globs)
+ (eshell-glob-convert-1 "*" t))
glob-remainder (cddr globs))
(setq glob (car globs)
glob-remainder (cdr globs)))
@@ -363,7 +374,13 @@ eshell-glob-entries
(if glob-remainder
(when isdir
(push (concat path name) dirs))
- (push (concat path name) eshell-glob-matches)))
+ (when (or (not only-dirs)
+ (and isdir
+ (not (and (eq recurse-p 'recurse)
+ (file-symlink-p
+ (directory-file-name
+ (concat path name)))))))
+ (push (concat path name) eshell-glob-matches))))
(when (and recurse-p isdir
(not (member name '("./" "../")))
(setq pathname (concat path name))
@@ -372,9 +389,9 @@ eshell-glob-entries
(directory-file-name pathname)))))
(push pathname rdirs))))
(dolist (dir (nreverse dirs))
- (eshell-glob-entries dir glob-remainder))
+ (eshell-glob-entries dir glob-remainder only-dirs))
(dolist (rdir (nreverse rdirs))
- (eshell-glob-entries rdir globs)))))
+ (eshell-glob-entries rdir globs only-dirs)))))
(provide 'em-glob)
diff --git a/test/lisp/eshell/em-glob-tests.el b/test/lisp/eshell/em-glob-tests.el
index 65f340a8da..b733be35d9 100644
--- a/test/lisp/eshell/em-glob-tests.el
+++ b/test/lisp/eshell/em-glob-tests.el
@@ -60,6 +60,12 @@ em-glob-test/match-any-string
(should (equal (eshell-extended-glob "*.el")
'("a.el" "b.el")))))
+(ert-deftest em-glob-test/match-any-directory ()
+ "Test that \"*/\" pattern matches any directory."
+ (with-fake-files '("a.el" "b.el" "dir/a.el" "dir/sub/a.el" "symlink/")
+ (should (equal (eshell-extended-glob "*/")
+ '("dir/" "symlink/")))))
+
(ert-deftest em-glob-test/match-any-character ()
"Test that \"?\" pattern matches any character."
(with-fake-files '("a.el" "b.el" "ccc.el" "d.txt" "dir/a.el")
@@ -71,7 +77,9 @@ em-glob-test/match-recursive
(with-fake-files '("a.el" "b.el" "ccc.el" "d.txt" "dir/a.el" "dir/sub/a.el"
"dir/symlink/a.el" "symlink/a.el" "symlink/sub/a.el")
(should (equal (eshell-extended-glob "**/a.el")
- '("a.el" "dir/a.el" "dir/sub/a.el")))))
+ '("a.el" "dir/a.el" "dir/sub/a.el")))
+ (should (equal (eshell-extended-glob "**/")
+ '("dir/" "dir/sub/")))))
(ert-deftest em-glob-test/match-recursive-follow-symlinks ()
"Test that \"***/\" recursively matches directories, following symlinks."
@@ -79,7 +87,10 @@ em-glob-test/match-recursive-follow-symlinks
"dir/symlink/a.el" "symlink/a.el" "symlink/sub/a.el")
(should (equal (eshell-extended-glob "***/a.el")
'("a.el" "dir/a.el" "dir/sub/a.el" "dir/symlink/a.el"
- "symlink/a.el" "symlink/sub/a.el")))))
+ "symlink/a.el" "symlink/sub/a.el")))
+ (should (equal (eshell-extended-glob "***/")
+ '("dir/" "dir/sub/" "dir/symlink/" "symlink/"
+ "symlink/sub/")))))
(ert-deftest em-glob-test/match-recursive-mixed ()
"Test combination of \"**/\" and \"***/\"."
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread