Skip to content

Conversation

aharpervc
Copy link

@aharpervc aharpervc commented May 2, 2016

Fix for #18660 for rails 5 (based on master branch, assuming that's the right place for rails 5 fixes).

I went ahead and made pr's for previous versions... not sure how you guys handle backporting fixes, if extra pr's is noise they can just be closed. However, backporting this fix is important either way.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @chancancode (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.

@maclover7
Copy link
Contributor

maclover7 commented May 2, 2016

4.0 is unmaintained for bug fixes -- going to close #24824. Can you look into adding a regression test here? [edit: 4.1 is still available for bug fixes]

@aharpervc
Copy link
Author

Ohhh, not being able to accept a backport for this is kind of a problem. In my personal case, the backport to 4.0 is more important than this fix on master. Is that a hard/fixed policy? Otherwise I'm basically going to need to run a fork, which I'd prefer not to.

@maclover7
Copy link
Contributor

Yep, 4.0.z receives no maintenance per our maintenance policy listed here

@aharpervc
Copy link
Author

Working on a test case... I might need some assistance on that, as helpers tests are a bit intimidating... I'll add a commit to this pr tomorrow.

@maclover7
Copy link
Contributor

👍 thanks!

@maclover7
Copy link
Contributor

Since the fix appears to be uniform across the branch PRs, we can reopen the backport PRs (if it is decided that the fix should be backported) once the master branch PR is merged in.

@aharpervc
Copy link
Author

I think this test is not actually verifying what I need it to verify... not sure... still thinking about this problem.

@xn
Copy link

xn commented May 4, 2016

So we have:

A) case sensitive systems
vs
B) case insensitive systems

::::

  1. _path is cased differently than some result from Dir[]
    vs
  2. _path is cased the same as some result from Dir[]

::::

What we need to figure out is if we have a match based off of those situations.
A1 -> not a match
A2 -> match

B1 -> match
B2 -> match

::::

So how do you determine case sensitivity?
Ruby gives you:

File::FNM_SYSCASE

But I get the same result, 0 on my case insensitive OSX filesystem as I do on my case sensitive Ubuntu server, so looking further, HAVE_HFS exists in Dir.c, etc, which is set by the existance of __APPLE__ and is used for these kinds of checks, but is not available outside of that as far as I can see. We could have

(File::FNM_SYSCASE > 0) || ((RUBY_PLATFORM =~ /darwin/).to_i > 0)

Which could be tucked in it's own method and then stubbed in a test.

sound good?

@aharpervc
Copy link
Author

Seems plausible to me... I'll investigate the implementation next week

@PascalPixel
Copy link

PascalPixel commented Jun 23, 2017

Heads up; macOS 10.13 ships with a conversion to Case-Sensitive APFS.
This PR fixes the issue.

@gmcgibbon
Copy link
Member

Looks like this issue was fixed by @matthewd a few days ago in #33704. I believe this can be closed now.

@matthewd matthewd closed this Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants