Add a Jekyll doctor warning for URLs that only differ by case #3171
Conversation
Those URLs are problematic on case-insensitive file systems because one of the URLs is overwritten by the other. Fixes jekyll#3035
| urls[dest.downcase] << dest | ||
| else | ||
| urls[dest.downcase] = [dest] | ||
| end |
There was a problem hiding this comment.
This if block can be simplified to:
(urls[dest.downcase] ||= []) << destThere was a problem hiding this comment.
Sure, but this would make it inconsistent with collect_urls() above. I'd say consistency is more important than short code? Or should I change the other method too?
There was a problem hiding this comment.
Consistency is not important to me here. Theoretically every method should do entirely different things :)
Is it safe to always assume that the current system is the ultimate deployment target? |
|
This is true! Ok, cool. @mattr- / @alfredxing, what do you think about this? Thanks for writing this up, @akoeplinger! |
|
👍 This is definitely useful, thanks for the PR! |
| urls[dest.downcase] = [dest] | ||
| end | ||
| end | ||
| urls |
There was a problem hiding this comment.
How about
things.inject(urls) do |memo, thing|
dest = thing.destination(destination)
(memo[dest.downcase] ||= []) << dest
memo
endThere was a problem hiding this comment.
Looks good, pushed it.
|
Is there anything else here I need to do? |
|
Looks good to me! @alfredxing? Thoughts? |
|
I'm a bit concerned about performance but that's it. |
|
Want to write a couple quick unit tests for this? |
I tested on our website with ~2200 files and I'll take a look at adding some unit tests. |
|
Looks good! I was a bit concerned about performance as well but it doesn't seem to bad based on @akoeplinger's stats. |
|
Hey guys, I added some tests here: akoeplinger@34ff0bb I'm not really happy with them as I needed to make these two separate folders for the site source (I couldn't get anything in-memory working, basically cause I have zero insight into how the Jekyll internals work together), would be glad to hear a better approach. |
|
|
||
| should 'return success on a valid site/page' do | ||
| @site = Site.new(Jekyll.configuration({ | ||
| "source" => File.join(source_dir, '/_urls_differ_by_case_valid'), |
There was a problem hiding this comment.
You can use source_dir('_urls_differ_by_case_valid') here – it does all the joining for you :)
|
domain names are case sensitive according to RFC 4353. The rest is up to how the server wants to handle GET requests, but I'd rather eliminate the possibility of any duplicates due to case-sensitivity, so 👍 on this. @akoeplinger It looks like there are some missing tests to add. Would you mind taking care of those so we can merge this? |
👍 |
|
Still interested in seeing this @akoeplinger? |
|
@parkr ahh sorry guys, I totally missed the notifications here and was busy with other stuff. I'll add the tests tomorrow :) |
|
@akoeplinger Any updates? |
|
Merged in fdcd761. |
|
@parkr omg, no idea how this fell off my radar, sorry for the lack of updates 😭 Thanks for merging! |
Those URLs are problematic on case-insensitive file systems because one of the URLs is overwritten by the other. Fixes #3035.
Note: I can't seem to find tests for doctor, I'm assuming there are none?