uiop:directory-files doesn't behave on paths without a trailing slash
Hello, I recently noticed that uiop's DIRECTORY-FILES does not ensure that the path is always interpreted as a directory. On sbcl (and presumably other implementations), if the path does not have a trailing slash, the files in the parent directory are instead returned. This does not appear to be the indented behavior, given that SUBDIRECTORIES ensures that the path is a directory. A patch for this new behavior and current/proposed results are below. Neil Lindquist ## Patch ## --- a/uiop/filesystem.lisp +++ b/uiop/filesystem.lisp @@ -209,7 +209,7 @@ Subdirectories should NOT be returned. override the default at your own risk. DIRECTORY-FILES tries NOT to resolve symlinks if the implementation permits this, but the behavior in presence of symlinks is not portable. Use IOlib to handle such situations." - (let ((dir (pathname directory))) + (let ((dir (ensure-directory-pathname directory)))) (when (logical-pathname-p dir) ;; Because of the filtering we do below, ;; logical pathnames have restrictions on wild patterns. ## Current behavior ## CL-USER> (uiop:directory-files "doc") (#P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/README.md" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/asdf-driver.asd" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/backward-driver.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/common-lisp.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/configuration.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/driver.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/filesystem.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/image.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/launch-program.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/lisp-build.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/os.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/package.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/pathname.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/run-program.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/stream.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/uiop.asd" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/utility.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/version.lisp") ## Proposed Behavior ## COMMON-LISP-USER> (uiop:directory-files "doc") (#P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/.gitignore" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/Makefile" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/docstrings.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/template.texinfo")
Thanks for this patch. I have pushed it (it can be seen in master as version 3.3.4), together with a simple test. On 24 Jun 2019, at 12:22, Neil Lindquist wrote:
Hello,
I recently noticed that uiop's DIRECTORY-FILES does not ensure that the path is always interpreted as a directory. On sbcl (and presumably other implementations), if the path does not have a trailing slash, the files in the parent directory are instead returned. This does not appear to be the indented behavior, given that SUBDIRECTORIES ensures that the path is a directory. A patch for this new behavior and current/proposed results are below.
Neil Lindquist
## Patch ## --- a/uiop/filesystem.lisp +++ b/uiop/filesystem.lisp @@ -209,7 +209,7 @@ Subdirectories should NOT be returned. override the default at your own risk. DIRECTORY-FILES tries NOT to resolve symlinks if the implementation permits this, but the behavior in presence of symlinks is not portable. Use IOlib to handle such situations." - (let ((dir (pathname directory))) + (let ((dir (ensure-directory-pathname directory)))) (when (logical-pathname-p dir) ;; Because of the filtering we do below, ;; logical pathnames have restrictions on wild patterns.
## Current behavior ## CL-USER> (uiop:directory-files "doc") (#P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/README.md" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/asdf-driver.asd" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/backward-driver.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/common-lisp.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/configuration.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/driver.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/filesystem.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/image.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/launch-program.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/lisp-build.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/os.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/package.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/pathname.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/run-program.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/stream.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/uiop.asd" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/utility.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/version.lisp")
## Proposed Behavior ## COMMON-LISP-USER> (uiop:directory-files "doc") (#P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/.gitignore" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/Makefile" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/docstrings.lisp" #P"C:/Users/Neil/Documents/coding/lisp/asdf/uiop/doc/template.texinfo")
Neil Lindquist <neillindquist5@gmail.com> writes:
Hello,
I recently noticed that uiop's DIRECTORY-FILES does not ensure that the path is always interpreted as a directory. On sbcl (and presumably other implementations), if the path does not have a trailing slash, the files in the parent directory are instead returned. This does not appear to be the indented behavior, given that SUBDIRECTORIES ensures that the path is a directory.
May as well also do that for uiop:with-current-directory. I've attached a diff, and have a test case below Test case: (uiop:with-current-directory ("/home/user") (run-program "ls" :output t)) Behaviour is simiar to DIRECTORY-FILES. ;=> Current: lists my /home dir ;=> With diff: lists my /home/user dir ~~~~~~~~~~ DIFF ~~~~~~~~~~~~~ 498c498 < `(call-with-current-directory ,dir #'(lambda () ,@body)))) ---
`(call-with-current-directory (ensure-directory-pathname ,dir) #'(lambda () ,@body))))
Coleslaw has been using a similar version of the above for awhile.
A patch for this new behavior and current/proposed results are below. ## Patch ## --- a/uiop/filesystem.lisp +++ b/uiop/filesystem.lisp @@ -209,7 +209,7 @@ Subdirectories should NOT be returned. override the default at your own risk. DIRECTORY-FILES tries NOT to resolve symlinks if the implementation permits this, but the behavior in presence of symlinks is not portable. Use IOlib to handle such situations." - (let ((dir (pathname directory))) + (let ((dir (ensure-directory-pathname directory)))) (when (logical-pathname-p dir) ;; Because of the filtering we do below, ;; logical pathnames have restrictions on wild patterns.
-- Spenser Truex usenet@spensertruex.com https://spensertruex.com/ San Francisco, USA
Generally this looks good, but why did you put the change in `with-current-directory` instead of in `call-with-current-directory`, since the former is just a thin wrapper around the latter? They are both exported, so I think it would be better to put it there. That leaves us with the following (rather ugly) form: ``` (let ((dir (resolve-symlinks* (get-pathname-defaults (pathname-directory-pathname (ensure-directory-pathname dir))))) ...) ...) ``` It's redundant to call `pathname-directory-pathname` on `ensure-directory-pathname`, so we just need the latter. I will push this after testing. On 25 Jun 2019, at 0:26, Spenser Truex wrote:
Neil Lindquist <neillindquist5@gmail.com> writes:
Hello,
I recently noticed that uiop's DIRECTORY-FILES does not ensure that the path is always interpreted as a directory. On sbcl (and presumably other implementations), if the path does not have a trailing slash, the files in the parent directory are instead returned. This does not appear to be the indented behavior, given that SUBDIRECTORIES ensures that the path is a directory.
May as well also do that for uiop:with-current-directory. I've attached a diff, and have a test case below
Test case: (uiop:with-current-directory ("/home/user") (run-program "ls" :output t))
Behaviour is simiar to DIRECTORY-FILES. ;=> Current: lists my /home dir ;=> With diff: lists my /home/user dir
~~~~~~~~~~ DIFF ~~~~~~~~~~~~~
498c498 < `(call-with-current-directory ,dir #'(lambda () ,@body)))) ---
`(call-with-current-directory (ensure-directory-pathname ,dir) #'(lambda () ,@body))))
Coleslaw has been using a similar version of the above for awhile.
A patch for this new behavior and current/proposed results are below. ## Patch ## --- a/uiop/filesystem.lisp +++ b/uiop/filesystem.lisp @@ -209,7 +209,7 @@ Subdirectories should NOT be returned. override the default at your own risk. DIRECTORY-FILES tries NOT to resolve symlinks if the implementation permits this, but the behavior in presence of symlinks is not portable. Use IOlib to handle such situations." - (let ((dir (pathname directory))) + (let ((dir (ensure-directory-pathname directory)))) (when (logical-pathname-p dir) ;; Because of the filtering we do below, ;; logical pathnames have restrictions on wild patterns.
-- Spenser Truex usenet@spensertruex.com https://spensertruex.com/ San Francisco, USA
OK, applied the revised fix, added tests, and pushed. Note that Faré points out I mis-numbered the earlier version. The latest version is now 3.3.3.1 and the incorrect 3.3.4 tag **has been removed**. We now return you to your regularly scheduled Common Lisp. Cheers, R On 25 Jun 2019, at 9:24, Robert Goldman wrote:
Generally this looks good, but why did you put the change in `with-current-directory` instead of in `call-with-current-directory`, since the former is just a thin wrapper around the latter?
They are both exported, so I think it would be better to put it there. That leaves us with the following (rather ugly) form:
``` (let ((dir (resolve-symlinks* (get-pathname-defaults (pathname-directory-pathname (ensure-directory-pathname dir))))) ...) ...) ```
It's redundant to call `pathname-directory-pathname` on `ensure-directory-pathname`, so we just need the latter.
I will push this after testing.
On 25 Jun 2019, at 0:26, Spenser Truex wrote:
Neil Lindquist <neillindquist5@gmail.com> writes:
Hello,
I recently noticed that uiop's DIRECTORY-FILES does not ensure that the path is always interpreted as a directory. On sbcl (and presumably other implementations), if the path does not have a trailing slash, the files in the parent directory are instead returned. This does not appear to be the indented behavior, given that SUBDIRECTORIES ensures that the path is a directory.
May as well also do that for uiop:with-current-directory. I've attached a diff, and have a test case below
Test case: (uiop:with-current-directory ("/home/user") (run-program "ls" :output t))
Behaviour is simiar to DIRECTORY-FILES. ;=> Current: lists my /home dir ;=> With diff: lists my /home/user dir
~~~~~~~~~~ DIFF ~~~~~~~~~~~~~
498c498 < `(call-with-current-directory ,dir #'(lambda () ,@body)))) ---
`(call-with-current-directory (ensure-directory-pathname ,dir) #'(lambda () ,@body))))
Coleslaw has been using a similar version of the above for awhile.
A patch for this new behavior and current/proposed results are below. ## Patch ## --- a/uiop/filesystem.lisp +++ b/uiop/filesystem.lisp @@ -209,7 +209,7 @@ Subdirectories should NOT be returned. override the default at your own risk. DIRECTORY-FILES tries NOT to resolve symlinks if the implementation permits this, but the behavior in presence of symlinks is not portable. Use IOlib to handle such situations." - (let ((dir (pathname directory))) + (let ((dir (ensure-directory-pathname directory)))) (when (logical-pathname-p dir) ;; Because of the filtering we do below, ;; logical pathnames have restrictions on wild patterns.
-- Spenser Truex usenet@spensertruex.com https://spensertruex.com/ San Francisco, USA
"Robert Goldman" <rpgoldman@sift.info> writes:
OK, applied the revised fix, added tests, and pushed.
On 25 Jun 2019, at 9:24, Robert Goldman wrote:
Generally this looks good, but why did you put the change in with-current-directory instead of in call-with-current-directory, since the former is just a thin wrapper around the latter?
They are both exported, so I think it would be better to put it there. That leaves us with the following (rather ugly) form:
(let ((dir (resolve-symlinks* (get-pathname-defaults (pathname-directory-pathname (ensure-directory-pathname dir))))) ...) ...)
It's redundant to call pathname-directory-pathname on ensure-directory-pathname, so we just need the latter.
Indeed, so why did you push the above code to <URL:https://gitlab.common-lisp.net/asdf/asdf/blob/b1ffbc47442973a18c43a44f6e17f0... >? I suggest you do that reduction (below) *** /uiop/filesystem.lisp *** 491,499 **** Note that this operation is usually NOT thread-safe." (if dir (let* ((dir (resolve-symlinks* - (get-pathname-defaults - (ensure-directory-pathname - dir)))) (cwd (getcwd)) (*default-pathname-defaults* dir)) (chdir dir) --- 491,498 ---- Note that this operation is usually NOT thread-safe." (if dir (let* ((dir (resolve-symlinks* + (ensure-directory-pathname + dir))) (cwd (getcwd)) (*default-pathname-defaults* dir)) (chdir dir) -- Spenser Truex https://spensertruex.com/ San Francisco, USA
I checked and `pathname-directory-pathname` is redundant, because of the guarantees provided by `ensure-directory-pathname`, but when I look at `get-pathname-defaults`, I am pretty sure it *cannot* be removed in the same way. After `get-pathname-defaults` any relative pathnames will be resolved by pathname merging, and we should be guaranteed to have an absolute pathname. AFAICT, `ensure-directory-pathname` can return a relative pathname. To be honest, I don't know if an absolute pathname is required, but I would have to understand a lot more of `uiop` to be sure it was safe to relax that constraint, so I left it. I'm willing to be convinced if you have more energy than I do. Best, R P.S. please send diffs with some more context if you can -- I find the following diff flags helpful: `--ignore-space-change -u -F '\''^(def'\''` On 25 Jun 2019, at 17:10, Spenser Truex wrote:
"Robert Goldman" <rpgoldman@sift.info> writes:
OK, applied the revised fix, added tests, and pushed.
On 25 Jun 2019, at 9:24, Robert Goldman wrote:
Generally this looks good, but why did you put the change in with-current-directory instead of in call-with-current-directory, since the former is just a thin wrapper around the latter?
They are both exported, so I think it would be better to put it there. That leaves us with the following (rather ugly) form:
(let ((dir (resolve-symlinks* (get-pathname-defaults (pathname-directory-pathname (ensure-directory-pathname dir))))) ...) ...)
It's redundant to call pathname-directory-pathname on ensure-directory-pathname, so we just need the latter.
Indeed, so why did you push the above code to <URL:https://gitlab.common-lisp.net/asdf/asdf/blob/b1ffbc47442973a18c43a44f6e17f0...
?
I suggest you do that reduction (below)
*** /uiop/filesystem.lisp *** 491,499 **** Note that this operation is usually NOT thread-safe." (if dir (let* ((dir (resolve-symlinks* - (get-pathname-defaults - (ensure-directory-pathname - dir)))) (cwd (getcwd)) (*default-pathname-defaults* dir)) (chdir dir) --- 491,498 ---- Note that this operation is usually NOT thread-safe." (if dir (let* ((dir (resolve-symlinks* + (ensure-directory-pathname + dir))) (cwd (getcwd)) (*default-pathname-defaults* dir)) (chdir dir)
-- Spenser Truex https://spensertruex.com/ San Francisco, USA
"Robert Goldman" <rpgoldman@sift.info> writes:
I checked and pathname-directory-pathname is redundant, because of the guarantees provided by ensure-directory-pathname, but when I look at get-pathname-defaults, I am pretty sure it cannot be removed in the same way.
After get-pathname-defaults any relative pathnames will be resolved by pathname merging, and we should be guaranteed to have an absolute pathname.
AFAICT, ensure-directory-pathname can return a relative pathname.
To be honest, I don't know if an absolute pathname is required, but I would have to understand a lot more of uiop to be sure it was safe to relax that constraint, so I left it. I'm willing to be convinced if you have more energy than I do.
Indeed, it does seem to be quite a mess. I'll let it alone.
Best, R
P.S. please send diffs with some more context if you can -- I find the following diff flags helpful: --ignore-space-change -u -F '\''^(def'\''
Thank you for those diff flags, I will use them in future lisp diffs. -- Spenser Truex https://spensertruex.com/ San Francisco, USA
participants (4)
-
Neil Lindquist -
Robert Goldman -
Spenser Truex -
Spenser Truex