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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

return correct file in dir if dir has same name as file #6569

Merged
merged 5 commits into from Nov 24, 2017

Conversation

@Crunch09
Member

Crunch09 commented Nov 19, 2017

  • bar.html
  • bar
    |- baz.html

GET /bar/
Before: Returns bar.html
After: Returns bar.html

GET /bar/baz
Before: Returns bar.html
After: Returns bar/baz.html

GET /bar/whatever
Before: Returns bar.html
After: Returns 404

This fixes #6475. Thanks @janpio for the great reproduction script! 馃憤

cc: @jekyll/build

return correct file in dir if dir has same name as file
- bar.html
- bar
  |- baz.html

_GET /bar/_
_Before:_ Returns bar.html
_After:_  Returns bar.html

_GET /bar/baz_
_Before:_ Returns bar.html
_After:_  Returns bar/baz.html

_GET /bar/whatever_
_Before:_ Returns bar.html
_After:_  Returns 404

This fixes #6475

@Crunch09 Crunch09 requested a review from pathawks Nov 19, 2017

@jekyllbot jekyllbot requested review from ayastreb, mattr- and parkr Nov 19, 2017

@DirtyF DirtyF added the fix label Nov 19, 2017

@DirtyF DirtyF added this to the v3.7.0 milestone Nov 19, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 20, 2017

Member

@Crunch09 Does this address #6222 as well (resolved via #6231). I think you should test for that case as well, if this doesn't already..

Member

ashmaroli commented Nov 20, 2017

@Crunch09 Does this address #6222 as well (resolved via #6231). I think you should test for that case as well, if this doesn't already..

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 20, 2017

Member

@ashmaroli Yes, this is checked by this test.

Member

Crunch09 commented Nov 20, 2017

@ashmaroli Yes, this is checked by this test.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 20, 2017

Member

Have you verified this against GitHub Pages? Ideally we'd keep this server behaviour the same, especially around an edge case like this which is confusing to users.

Member

parkr commented Nov 20, 2017

Have you verified this against GitHub Pages? Ideally we'd keep this server behaviour the same, especially around an edge case like this which is confusing to users.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 21, 2017

Member

@parkr Checked the current Github-Pages behavior with https://janpio.github.io/jekyll-path-test/ (provided by @janpio ). After this PR, webrick should have the same behavior.

Member

Crunch09 commented Nov 21, 2017

@parkr Checked the current Github-Pages behavior with https://janpio.github.io/jekyll-path-test/ (provided by @janpio ). After this PR, webrick should have the same behavior.

@oe

oe approved these changes Nov 23, 2017

@DirtyF

DirtyF approved these changes Nov 23, 2017

LGTM 馃憤

@parkr

parkr approved these changes Nov 24, 2017

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 24, 2017

Member

@jekyllbot: merge +bug

Member

DirtyF commented Nov 24, 2017

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 368fa1f into jekyll:master Nov 24, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP ready for review
Details

@jekyllbot jekyllbot added bug fix labels Nov 24, 2017

@Crunch09 Crunch09 deleted the Crunch09:directory_file_mix_up branch Nov 24, 2017

DirtyF added a commit that referenced this pull request Dec 7, 2017

DirtyF added a commit that referenced this pull request Dec 7, 2017

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