Fix bug in PathUtils::entries when dir is empty

[dometto]

When working on a Rails app I ran into the following exception from sprockets:

undefined method `sort!' for nil:NilClass | lib/gems.jar!/gems/sprockets-3.3.5/lib/sprockets/path_utils.rb:58:in `entries'|

On investigation it seems that the PathUtils::entries method is performing an Array#reject!, which returns nil for an empty array. This is a rare case because Dir.entries never (I think) returns an empty array on MRI . However, it apparently does on JRuby, and might on other platforms.

Apparently, the reject! was introduced as a small performance improvement in a6bf1f9. This PR fixes the bug by reverting to reject instead again.

EDIT

To clarify, Array#reject! returns nil whenever the regex has no matches, so an empty directory is technically just a specific version of this edge case.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[dometto]

Not sure why the one test is failing. The expected and actual result look identical to me.

[schneems]

Thanks for this PR, sorry for the delayed review. I took a look a the test output they're subtly different:

a = {"version"=>3, "file"=>"sass/main.css", "mappings"=>"AADA,GAAA,CAAA,EAAA,CAAA;EAAA,MAAA,EAAA,CAAA;EAAA,OAAA,EAAA,CAAA;EAAA,UAAA,EAAA,IAAA,GAAA;;AAAA,GAAA,CAAA,EAAA,CAAA;EAAA,OAAA,EAAA,YAAA,GAAA;;AAAA,GAAA,CAAA,CAAA,CAAA;EAAA,OAAA,EAAA,KAAA;EAAA,OAAA,EAAA,QAAA;EAAA,eAAA,EAAA,IAAA,GAAA", "sources"=>["sass/main.source-86fe07ad89fecbab307d376bcadfa23d65ad108e3735b564510246b705f6ced1.scss"], "names"=>[]}

b = {"version"=>3, "file"=>"sass/main.css", "mappings"=>"AADA,GAAA,CAAA,EAAA,CAAA;EAAA,MAAA,EAAA,CAAA;EAAA,OAAA,EAAA,CAAA;EAAA,UAAA,EAAA,IAAA,GAAA;;AAAA,GAAA,CAAA,EAAA,CAAA;EAAA,OAAA,EAAA,YAAA,GAAA;;AAAA,GAAA,CAAA,CAAA,CAAA;EAAA,OAAA,EAAA,KAAA;EAAA,OAAA,EAAA,GAAA,CAAA,IAAA;EAAA,eAAA,EAAA,IAAA,GAAA", "sources"=>["sass/main.source-86fe07ad89fecbab307d376bcadfa23d65ad108e3735b564510246b705f6ced1.scss"], "names"=>[]}
puts a == b
# => false
puts a["mappings"]
# => "AADA,GAAA,CAAA,EAAA,CAAA;EAAA,MAAA,EAAA,CAAA;EAAA,OAAA,EAAA,CAAA;EAAA,UAAA,EAAA,IAAA,GAAA;;AAAA,GAAA,CAAA,EAAA,CAAA;EAAA,OAAA,EAAA,YAAA,GAAA;;AAAA,GAAA,CAAA,CAAA,CAAA;EAAA,OAAA,EAAA,KAAA;EAAA,OAAA,EAAA,QAAA;EAAA,eAAA,EAAA,IAAA,GAAA"
puts b["mappings"]
# => "AADA,GAAA,CAAA,EAAA,CAAA;EAAA,MAAA,EAAA,CAAA;EAAA,OAAA,EAAA,CAAA;EAAA,UAAA,EAAA,IAAA,GAAA;;AAAA,GAAA,CAAA,EAAA,CAAA;EAAA,OAAA,EAAA,YAAA,GAAA;;AAAA,GAAA,CAAA,CAAA,CAAA;EAAA,OAAA,EAAA,KAAA;EAAA,OAAA,EAAA,GAAA,CAAA,IAAA;EAAA,eAAA,EAAA,IAAA,GAAA"

That commit by @tenderlove did introduce a subtle bug that wasn't tested against. When you reject from an array but reject nothing, the return value is nil instead of the original array

a = [1,2,3]
puts a.reject! {|x| x == 4 }
# => nil

versus

a = [1,2,3]
puts a.reject {|x| x == 4 }
# => [1,2,3]

To preserve the speed bump, and revert to the original, correct, behavior we should probably do something like:

entries = Dir.entries(path, encoding: Encoding.default_internal)
entries.reject! { |entry|
  entry =~ /^\.|~$|^\#.*\#$/
}
entries.sort!
entries

We should also add a test case that verifies the correct result when an entry is used that does not have any rejected elements, can you add one in test_path_utils.rb?

[dometto]

Thanks for the review! I'll update the code and add a test.

[bartkamphorst]

Stubbing Dir.entries might help when writing the test. E.g.:

jruby-head :001 > require 'sprockets'
 => true 
jruby-head :002 > require 'minitest/mock'
 => true 
jruby-head :003 > Dir.stub :entries, ['a', 'b', 'c'] do Sprockets::PathUtils.entries('Dir.tmpdir'); end
NoMethodError: undefined method `sort!' for nil:NilClass
    from $HOME/.rvm/gems/jruby-head/gems/sprockets-3.4.0/lib/sprockets/path_utils.rb:58:in `entries'
    from (irb):3:in `evaluate'
    from $HOME/.rvm/gems/jruby-head/gems/minitest-5.8.2/lib/minitest/mock.rb:223:in `stub'
    from (irb):3:in `evaluate'
    from org/jruby/RubyKernel.java:990:in `eval'
    from org/jruby/RubyKernel.java:1307:in `loop'
    from org/jruby/RubyKernel.java:1117:in `catch'
    from org/jruby/RubyKernel.java:1117:in `catch'
    from $HOME/.rvm/rubies/jruby-head/bin/irb:13:in `(root)'

jruby-head :004 > Dir.stub :entries, ['a', 'b', 'c', '.', '..'] do Sprockets::PathUtils.entries('Dir.tmpdir'); end
 => ["a", "b", "c"] 

With the new and improved PathUtils.entries implementation we would also expect ["a", "b", "c"] to be returned in the first case.

[schneems]

Using

entries = Dir.entries(path, encoding: Encoding.default_internal)
entries.reject! { |entry|
  entry =~ /^\.|~$|^\#.*\#$/
}
entries.sort!
entries

entries should never be nil.

I try to never ever use mocks or stubs unless absolutely needed. You can create a folder in test/fixtures directory and add files a and b etc.

[dometto]

On 04-11-15 17:04, Richard Schneeman wrote:

You can create a folder in test/fixtures directory and add files |a|
and |b| etc.

I think @bartkamphorst is right that in this case stubs will
(unfortunately) be necessary. The problem is that on most platforms, the
result of Dir.entries will never be empty (because it will always
contain "." and ".."). So an empty fixture directory won't do the trick.

[schneems]

We don't need it to be empty, we just need the entries to all match that reject regex /^\.|~$|^\#.*\#$/ which i'm pretty sure is looking for . and ..

[dometto]

On 04-11-15 18:01, Richard Schneeman wrote:

we just need the entries to all match that reject regex
|/^.|~$|^#.*#$/| which i'm pretty sure is looking for |.| and |..|

We want to test the case in which no entries match the regex, right?
:) That's what won't be possible without stubs, since Dir.entries on
*nix will always contain ".", which matches the regex.

[schneems]

Yep, you're correct.

[dometto]

I've updated the code and added a test. I worry that the test is not very verbose about what behavior it's actually testing, so it potentially risks getting removed again in the future. If you have any suggestions to make it more clear that would be great (can of course add a comment linking to this issue, but that's not very pretty).

[dometto]

There is still a test failure, but note that I'm seeing the same failure on the master branch.

[bartkamphorst]

How about

[ ['a', 'b', 'c'], ['a', 'b', 'c', '.', '..'] ].each do | dir_entries_array |
  Dir.stub :entries, dir_entries_array do
    assert_equal ['a', 'b', 'c'], Sprockets::PathUtils.entries(Dir.tmpdir)
  end
end

That way, you show the two different possible outcomes of Dir.entries and you make explicit that you are testing PathUtils#entries.

[dometto]

I've incorporated @bartkamphorst's suggestion -- thanks!

The remaining test failure is caused by version 1.8 of the sassc gem being available. The tests pass when I manually install sassc 1.7.1. sprockets depends on sassc ~> 1.7 but it looks like the dependency should be stricter, or otherwise the breaking test should be modified. I guess that should happen in a different PR/commit though?

Just a quick question: will you also backport this fix to the 3.x branch? Please let me know if I should open a separate PR for that.

[bartkamphorst]

The 4th element of the second array should be '..'.

[dometto]

Thx.

[dometto]

@schneems @rafaelfranca does this PR require any more work?

[schneems]

Sorry for dropping off over the weekend. I have a 4 month old. Thanks for the work, this looks great!

[dometto]

Great, thanks! Want me to open one against the 3.x branch too?

[schneems]

It looks like the problem exists on 3.x as well

sprockets/lib/sprockets/path_utils.rb

Lines 58 to 60 in 80d5620

	Dir.entries(path, :encoding => Encoding.default_internal).reject! { |entry|
	entry =~ /^\.|~$|^\#.*\#$/
	}.sort!

would love a patch

[bartkamphorst]

👍 Thanks!