Avoid using Dir.glob with absolute path #4150

Merged
merged 4 commits into from Dec 1, 2015

Conversation

Projects
None yet
4 participants
@ducktyper
Contributor

ducktyper commented Nov 17, 2015

the absolute path including '[', '{', '?', or '*'
could change the outcome

Avoid using Dir.glob with absolute path
the absolute path including '[', '{', '?', or '*'
could change the outcome
@ducktyper

This comment has been minimized.

Show comment
Hide comment
Contributor

ducktyper commented Nov 17, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 17, 2015

Split this chdir, and glob into a Util method. Util.safe_glob(dir, glob) then Chdir and use the glob sent. That makes this much cleaner and easier to utilize globally, right now it's a coupled utility.

Split this chdir, and glob into a Util method. Util.safe_glob(dir, glob) then Chdir and use the glob sent. That makes this much cleaner and easier to utilize globally, right now it's a coupled utility.

@parkr parkr added this to the 3.0.2 milestone Nov 18, 2015

ducktyper added some commits Nov 18, 2015

Add Utils.safe_glob method
which works the same way as Dir.glob but seperating the input
into two parts ('dir' + '/' + 'pattern') to make sure
the first part('dir') does not act as a pattern.
@ducktyper

This comment has been minimized.

Show comment
Hide comment
@ducktyper

ducktyper Nov 18, 2015

Contributor

That's a good idea. now Utils.safe_glob(dir, pattern, flags=0) is created and used.
I created 'test/safe_glob_test[/find_me.txt' for testing but I'm worrying that the folder name is too noticeable in test folder. Let me know if there is a better place to put it.

Also I found out that there are few more places have the same issue using Dir[] with absolute path. If you change the project folder name from 'jekyll' to 'jekyll[' then many tests fail.
I can fix them to this pull request if it needs to be fixed.

Contributor

ducktyper commented Nov 18, 2015

That's a good idea. now Utils.safe_glob(dir, pattern, flags=0) is created and used.
I created 'test/safe_glob_test[/find_me.txt' for testing but I'm worrying that the folder name is too noticeable in test folder. Let me know if there is a better place to put it.

Also I found out that there are few more places have the same issue using Dir[] with absolute path. If you change the project folder name from 'jekyll' to 'jekyll[' then many tests fail.
I can fix them to this pull request if it needs to be fixed.

lib/jekyll/collection.rb
@@ -74,7 +74,7 @@ def read
def entries
return Array.new unless exists?
@entries ||=
- Dir.glob(collection_dir("**", "*.*")).map do |entry|
+ Utils.safe_glob(collection_dir, "**/*.*").map do |entry|

This comment has been minimized.

@parkr

parkr Nov 18, 2015

Member

If **/*.* platform-independent?

@parkr

parkr Nov 18, 2015

Member

If **/*.* platform-independent?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 18, 2015

Member

Looking good! Just a question above about Windows support.

Also I found out that there are few more places have the same issue using Dir[] with absolute path.

In other parts of Jekyll? Or just the tests? If you want to update those, too, I would love that. Maybe in a different PR?

Member

parkr commented Nov 18, 2015

Looking good! Just a question above about Windows support.

Also I found out that there are few more places have the same issue using Dir[] with absolute path.

In other parts of Jekyll? Or just the tests? If you want to update those, too, I would love that. Maybe in a different PR?

@ducktyper

This comment has been minimized.

Show comment
Hide comment
@ducktyper

ducktyper Nov 19, 2015

Contributor

Yes Windows support, I totally didn't see it. Thanks.
Now safe_glob also accepts array of patterns to support Windows.
I still leave linux style path in the test. Let me know if they need to be changed too.

The other problem I mentioned is in other parts of Jekyll. I will make another pull request if I fix them.

Contributor

ducktyper commented Nov 19, 2015

Yes Windows support, I totally didn't see it. Thanks.
Now safe_glob also accepts array of patterns to support Windows.
I still leave linux style path in the test. Let me know if they need to be changed too.

The other problem I mentioned is in other parts of Jekyll. I will make another pull request if I fix them.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 19, 2015

Member

LGTM!

Member

parkr commented Nov 19, 2015

LGTM!

@parkr parkr added the fix label Nov 19, 2015

parkr added a commit that referenced this pull request Dec 1, 2015

@parkr parkr merged commit b90f8e0 into jekyll:master Dec 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Dec 1, 2015

@envygeeks envygeeks modified the milestones: 3.1, 3.0.2 Dec 4, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.