uiop:directory-files doesn't behave on paths without a trailing slash

List overview All Threads
Download

newer

older

slime meta-dot in ASDF sources

What replaced the files contained...

Neil Lindquist

24 Jun 2019 24 Jun '19

5:22 p.m.

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")

Show replies by date

Robert Goldman

24 Jun 24 Jun

8:36 p.m.

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")

Spenser Truex

25 Jun 25 Jun

5:26 a.m.

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

2:24 p.m.

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
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

5:48 p.m.

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
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

Spenser Truex

10:10 p.m.

"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

11:01 p.m.

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

Spenser Truex

26 Jun 26 Jun

9:41 p.m.

"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

1901

Age (days ago)

1903

Last active (days ago)

asdf-devel@common-lisp.net

7 comments

4 participants

tags (0)

participants (4)

Neil Lindquist
Robert Goldman
Spenser Truex
Spenser Truex