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

Do not traverse symlinks in heuristics #3946

Merged
merged 1 commit into from
Dec 12, 2017
Merged

Conversation

kivikakk
Copy link
Contributor

This is part of what should be a correct fix for github/markup#1133. Changes are required in markup itself and dotcom to pass the symlink info through from the repository.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

@kivikakk kivikakk merged commit d4c2d83 into master Dec 12, 2017
@kivikakk kivikakk deleted the kivikakk/no-traverse-symlink branch December 12, 2017 10:53
def symlink?
return @symlink if !@symlink.nil?
@symlink = (File.symlink?(@fullpath) rescue false)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kivikakk I just noticed this is reporting a bunch of warnings in the tests: https://travis-ci.org/github/linguist/jobs/323823193

/home/travis/build/github/linguist/lib/linguist/file_blob.rb:30: warning: instance variable @symlink not initialized
/home/travis/build/github/linguist/lib/linguist/file_blob.rb:30: warning: instance variable @symlink not initialized
/home/travis/build/github/linguist/lib/linguist/file_blob.rb:30: warning: instance variable @symlink not initialized
/home/travis/build/github/linguist/lib/linguist/file_blob.rb:30: warning: instance variable @symlink not initialized

They're not failures hence we missed this before, but I think it'd be best to clean these up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also just realised this PR didn't intro any tests to verify the symlink behaviour. Sorry I missed this before.

Copy link
Contributor Author

@kivikakk kivikakk Jan 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, my bad. I've cleaned up the uninitialised instance var messages in 1673284 post-rebase: 067b8cc. Will add some tests in that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a test in 3eabf45.

@lildude lildude mentioned this pull request Jan 26, 2018
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants