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

Fix issues with File.dirname #4177

Merged
merged 6 commits into from Nov 6, 2016

Conversation

Projects
None yet
3 participants
@jellymann
Contributor

jellymann commented Sep 24, 2016

This fixes #4116

e.g. The result of File.dirname('/foo/bar\baz') on Windows is "/foo/bar"
while on Unix it is "/foo", since "bar\baz" is a single file name (\ is not a path separator in Unix)

As an added bonus, this also fixes a number of inconsistencies with MRI with regard to Windows UNC paths, notably:

  1. A typical UNC path is made up of "//<host>/<share>/path/to/file.ext", where the "root" is "//<host>/<share>"

    This means that, while File.dirname('//foo/bar/baz') is "//foo/bar", File.dirname('//foo/bar') is still "//foo/bar" (since "//foo/bar" is the "root").

  2. Extraneous "leading" separators are removed, e.g. File.dirname('////foo/bar/baz') is "//foo/bar"

  3. This logic must not be confused with Unix paths, where there should be no more than 1 separator at the beginning of the string.

    e.g. File.dirname('/foo/bar') should still be "/foo", on both Unix and Windows.

  4. For all of the above, / and \ are completely interchangeable at all times on Windows.

  5. Windows UNC paths are not recognised at all on Unix, e.g. File.dirname('//foo/bar') is still "/foo".

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 28, 2016

Member

@jellymann this PR nearly works but test_split for File#split uses dirname internally and these changes caused to specs to fail. Any chance you can take a look?

Member

enebo commented Sep 28, 2016

@jellymann this PR nearly works but test_split for File#split uses dirname internally and these changes caused to specs to fail. Any chance you can take a look?

@jellymann

This comment has been minimized.

Show comment
Hide comment
@jellymann

jellymann Oct 1, 2016

Contributor

@enebo I've had a look at the spec. Is there any reason why it specifically dictates that File.split deviates from MRI?

If File.split is to be "fixed" (i.e. made to pass current spec), it will no longer be consistent with File.dirname, which is not following POLA.

If File.split must pass the current spec, and we wish to remain consistent with File.dirname, then the deviation should be added to File.dirname. However, #4116 will not be resolved.

Removing the deviation from the File.split spec is, in my opinion, the most "correct" way to resolve the issue. However, this may be considered a breaking change, and it would depend heavily on the reason for the deviation in the first place.

It seems this deviation has been in there since the beginning of RubySpec

Contributor

jellymann commented Oct 1, 2016

@enebo I've had a look at the spec. Is there any reason why it specifically dictates that File.split deviates from MRI?

If File.split is to be "fixed" (i.e. made to pass current spec), it will no longer be consistent with File.dirname, which is not following POLA.

If File.split must pass the current spec, and we wish to remain consistent with File.dirname, then the deviation should be added to File.dirname. However, #4116 will not be resolved.

Removing the deviation from the File.split spec is, in my opinion, the most "correct" way to resolve the issue. However, this may be considered a breaking change, and it would depend heavily on the reason for the deviation in the first place.

It seems this deviation has been in there since the beginning of RubySpec

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 3, 2016

Member

@jellymann ok that is interesting. I have seen bad specs. If so we can tag it out in our tags file with Windows: (vs Fail: or Error:) then it will skip testing that spec on windows. I guess ultimately ruby/spec should get fixed then.

Member

enebo commented Oct 3, 2016

@jellymann ok that is interesting. I have seen bad specs. If so we can tag it out in our tags file with Windows: (vs Fail: or Error:) then it will skip testing that spec on windows. I guess ultimately ruby/spec should get fixed then.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 6, 2016

Member

We (or you) can fix the spec directly in our repo; @eregon periodically merges those changes across.

Member

headius commented Oct 6, 2016

We (or you) can fix the spec directly in our repo; @eregon periodically merges those changes across.

@jellymann

This comment has been minimized.

Show comment
Hide comment
@jellymann

jellymann Oct 8, 2016

Contributor

@headius @enebo I took out the deviation in File.spec, and it is now failing because File.basename is also not taking system-specific path separators into account (it does the same as File.dirname did, just replacing all \ with / before processing)

I will investigate MRI's behaviour and write tests for basename, and then fix the implementation.

Contributor

jellymann commented Oct 8, 2016

@headius @enebo I took out the deviation in File.spec, and it is now failing because File.basename is also not taking system-specific path separators into account (it does the same as File.dirname did, just replacing all \ with / before processing)

I will investigate MRI's behaviour and write tests for basename, and then fix the implementation.

@jellymann

This comment has been minimized.

Show comment
Hide comment
@jellymann

jellymann Oct 23, 2016

Contributor

@headius @enebo I've fixed basename (and consequently File.split as well) so that it behaves consistently with MRI.

Sorry it took a while, I had a family crisis and was out of town for a bit.

Contributor

jellymann commented Oct 23, 2016

@headius @enebo I've fixed basename (and consequently File.split as well) so that it behaves consistently with MRI.

Sorry it took a while, I had a family crisis and was out of town for a bit.

@enebo enebo merged commit bc7036a into jruby:master Nov 6, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 6, 2016

Member

@jellymann sorry I did not merge this a long time ago. We shall see how it works. At the time you submitted this our CI was pretty messed up so I might ping you if something else breaks as part of fallout.

Member

enebo commented Nov 6, 2016

@jellymann sorry I did not merge this a long time ago. We shall see how it works. At the time you submitted this our CI was pretty messed up so I might ping you if something else breaks as part of fallout.

@enebo enebo added this to the JRuby 9.1.6.0 milestone Nov 6, 2016

@enebo enebo added the windows label Nov 6, 2016

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