Skip to content
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

Conversation

danini-the-panini
Copy link
Contributor

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
Copy link
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?

@danini-the-panini
Copy link
Contributor Author

danini-the-panini 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
Copy link
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
Copy link
Member

headius commented Oct 6, 2016

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

@danini-the-panini
Copy link
Contributor Author

@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.

@danini-the-panini
Copy link
Contributor Author

@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
@enebo
Copy link
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File.dirname interprets backslashes as path separators on Mac OS X
3 participants