New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dir.glob returns doubles, if used with recursive directory matchers "**" #4353

Closed
MartinKoerner opened this Issue Dec 2, 2016 · 9 comments

Comments

Projects
None yet
5 participants
@MartinKoerner

MartinKoerner commented Dec 2, 2016

Environment

  • Mac OS Sierra 10.12.1
  • Directory structure with multiple subdirectories:
   tmp
    |\2
       |\abc
       |   \2 - file1.pdf
       |   \4 - file2.pdf
       |\def
       |   \2 - file3.pdf
       |   \4 - file4.pdf

Expected Behavior

   # MRI
   # rbenv shell 2.3.1
   # irb
      Dir.glob("tmp").size # = 1
      Dir.glob("tmp/2/abc/2/*.pdf").size # = 1
      Dir.glob("tmp/**/**/**/*.pdf").size # = 4

Actual Behavior

   # rbenv shell jruby-9.1.6.0
   # jirb
     Dir.glob("tmp").size # = 1
     Dir.glob("tmp/2/abc/2/*.pdf").size # = 1
     Dir.glob("tmp/**/**/**/*.pdf").size # = 12 <- WRONG!
     Dir.glob("tmp/**/**/**/*.pdf").uniq.size # 4 # doubles are equals according to String

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 7, 2017

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 7, 2017

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 7, 2017

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 9, 2017

@kares does the following PR fixes the issue?
#4428

sumitmah commented Jan 9, 2017

@kares does the following PR fixes the issue?
#4428

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 9, 2017

Member

@sumitmah well, we can't tell for sure until we do not have a test-case, right? :)

Member

kares commented Jan 9, 2017

@sumitmah well, we can't tell for sure until we do not have a test-case, right? :)

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 9, 2017

@kares I can add test case, I was concerned about other scenarios? do we have tests for them?

sumitmah commented Jan 9, 2017

@kares I can add test case, I was concerned about other scenarios? do we have tests for them?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 9, 2017

Member

The PR seems logical. You can run other test cases by running spec/mspec/bin/mspec ci spec/ruby/core/dir or some tests somewhere in test/mri (probably test/mri/ruby/test_dir.rb). You can also add cases to spec/ruby or test/jruby (rather not add to test/mri) that mimic the above cases.

Member

headius commented Jan 9, 2017

The PR seems logical. You can run other test cases by running spec/mspec/bin/mspec ci spec/ruby/core/dir or some tests somewhere in test/mri (probably test/mri/ruby/test_dir.rb). You can also add cases to spec/ruby or test/jruby (rather not add to test/mri) that mimic the above cases.

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 10, 2017

Thanks! @headius I'll check the specs.

sumitmah commented Jan 10, 2017

Thanks! @headius I'll check the specs.

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 10, 2017

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 10, 2017

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 10, 2017

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jan 10, 2017

Member

@sumitmah I do not want to land this right before 9.1.7.0 but we will merge afterwards.

Member

enebo commented Jan 10, 2017

@sumitmah I do not want to land this right before 9.1.7.0 but we will merge afterwards.

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 10, 2017

@enebo when is 9.1.7.0 release planned?

sumitmah commented Jan 10, 2017

@enebo when is 9.1.7.0 release planned?

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 10, 2017

@headius @kares I've added test case

sumitmah commented Jan 10, 2017

@headius @kares I've added test case

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 10, 2017

sumitmah pushed a commit to sumitmah/jruby that referenced this issue Jan 11, 2017

enebo added a commit that referenced this issue Jan 11, 2017

Merge pull request #4428 from sumitmah/4353-fix-dir-glob
[#4353] Ignore consecutive DOUBLE_STAR  in glob_helper.

@kares kares added this to the JRuby 9.1.8.0 milestone Jan 12, 2017

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 12, 2017

Member

should be resolved as #4428 got merged.

Member

kares commented Jan 12, 2017

should be resolved as #4428 got merged.

@kares kares closed this Jan 12, 2017

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