Skip to content

Fix Dir.glob when readdir doesn't report file type#9877

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
straight-shoota:fix/unix-dir-entry-type
Nov 12, 2020
Merged

Fix Dir.glob when readdir doesn't report file type#9877
straight-shoota merged 1 commit intocrystal-lang:masterfrom
straight-shoota:fix/unix-dir-entry-type

Conversation

@straight-shoota
Copy link
Copy Markdown
Member

@straight-shoota straight-shoota commented Nov 3, 2020

Dir.next_entry was missing a validation for d_type value. In case it is DT_UNKNOWN, we can't determine wether it's a directory or not. Entry#dir is an internal API currently only used by Dir.glob so changing its type to Bool | Nil to signal unknown is fine.
Fixes #9876

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:files labels Nov 3, 2020
Copy link
Copy Markdown
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works as a charm! 🎉
Thanks! ⚡

It usually itches me to use Bool?, because there are two falsey values in that type.

@asterite
Copy link
Copy Markdown
Member

asterite commented Nov 3, 2020

I would use an enum with File, Dir and Unknown instead of a Bool?. That way you don't even need to document that "if it's nil it means it's unknown".

@straight-shoota
Copy link
Copy Markdown
Member Author

This is just an internal ivar for optimizing Dir. glob, not anything near a public API. I'd prefer to keep it that simple. At least for now. We can revisit when considering to use more info from readdir for other purposes.

@asterite
Copy link
Copy Markdown
Member

asterite commented Nov 3, 2020

I'd prefer to keep it that simple

In my mind, an enum is simpler than Bool?. Specially when it comes to understanding what each of those values is.

@straight-shoota
Copy link
Copy Markdown
Member Author

Alternatively, Entry could just hold the d_type value...

@bcardiff
Copy link
Copy Markdown
Member

bcardiff commented Nov 4, 2020

Since the entry type is internal i'm fine with the current state.

But having an enum with all the values of https://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html, or at least the one we use today sounds like a path that can evolve smoothly. All unmapped d_type can default to the enum equivalent to unknown. That way there is no need to do the full mapping today if not wanted.

I would not assign directly the d_type value to abstraction leak.

@bcardiff
Copy link
Copy Markdown
Member

Given the discussion after the approval, should this PR expect more work or is good to go since the discussion is about non public design?

Copy link
Copy Markdown
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, the internal implementation details can be changed later on 👍

@straight-shoota straight-shoota merged commit 4b89c15 into crystal-lang:master Nov 12, 2020
@straight-shoota straight-shoota deleted the fix/unix-dir-entry-type branch November 12, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dir.glob does not work sshfs

3 participants