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.

Sign up for GitHub

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

Jump to bottom

[READY] Resolve symlinks in extra conf glob pattern #814

Merged
merged 2 commits into from
Aug 12, 2017
Merged

[READY] Resolve symlinks in extra conf glob pattern #814

merged 2 commits into from
Aug 12, 2017

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Aug 12, 2017 •
edited by Valloric
Loading

When a .ycm_extra_conf.py file is found, ycmd determines if this file should be white/blacklisted by checking that its path matches one of the patterns from the extra_conf_globlist option. This is done by converting the .ycm_extra_conf.py path and the patterns to absolute paths through the os.path.abspath function. However, this function doesn't resolve symbolic links. This means that if a pattern in extra_conf_globlist contains a symbolic link, any .ycm_extra_conf.py file that should be matched by this pattern if the symlink was resolved, won't be white/blacklisted.

This is fixed on UNIX by replacing os.path.abspath with the os.path.realpath function. Windows symlinks are not supported because os.path.realpath doesn't resolve symlinks on this platform and writing our own implementation of os.path.realpath supporting Windows symlinks is not worth the trouble.

This change is Reviewable

@micbou
Add test for symlink in extra conf glob pattern
3acf969 
Disable test on Windows since symlink support from the Python standard library
is nonexistent or incomplete (depending on the version of Python) on this
platform.
@micbou
Resolve symlinks in extra conf glob pattern
bfe0db9
@codecov-io
Copy link

codecov-io commented Aug 12, 2017 •
edited
Loading

Codecov Report

Merging #814 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #814   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files          79       79           
  Lines        5373     5373           
  Branches      170      170           
=======================================
  Hits         5093     5093           
  Misses        233      233           
  Partials       47       47

@bstaletic
Copy link
Collaborator

:lgtm:

So that's the difference between `abspath()` and `realpath()`! Thanks.

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@Valloric
Copy link
Member

A fantastic PR, as is customary from @micbou. :)

Thanks!

@zzbot r=bstaletic

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Aug 12, 2017

📌 Commit bfe0db9 has been approved by bstaletic

@zzbot
Copy link
Contributor

zzbot commented Aug 12, 2017

⌛ Testing commit bfe0db9 with merge 0a8f633...

zzbot added a commit that referenced this pull request Aug 12, 2017
@zzbot
Auto merge of #814 - micbou:resolve-symlink-extra-conf-globlist, r=bs…
0a8f633 
…taletic

[READY] Resolve symlinks in extra conf glob pattern

When a `.ycm_extra_conf.py` file is found, ycmd determines if this file should be white/blacklisted by checking that its path matches one of the patterns from [the `extra_conf_globlist` option](https://github.com/Valloric/YouCompleteMe#the-gycm_extra_conf_globlist-option). This is done by converting the `.ycm_extra_conf.py` path and the patterns to absolute paths through [the `os.path.abspath` function](https://docs.python.org/2/library/os.path.html#os.path.abspath). However, this function doesn't resolve symbolic links. This means that if a pattern in `extra_conf_globlist` contains a symbolic link, any `.ycm_extra_conf.py` file that should be matched by this pattern if the symlink was resolved, won't be white/blacklisted.

This is fixed on UNIX by replacing `os.path.abspath` with [the `os.path.realpath` function](https://docs.python.org/2/library/os.path.html#os.path.realpath). Windows symlinks are not supported because [`os.path.realpath` doesn't resolve symlinks on this platform](https://bugs.python.org/issue9949) and writing our own implementation of `os.path.realpath` supporting Windows symlinks is not worth the trouble.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/814)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Aug 12, 2017

☀️ Test successful - status-travis
Approved by: bstaletic
Pushing 0a8f633 to master...

@zzbot zzbot merged commit bfe0db9 into ycm-core:master Aug 12, 2017
@micbou micbou deleted the resolve-symlink-extra-conf-globlist branch August 29, 2017 09:36
@micbou micbou mentioned this pull request Sep 10, 2017
Merged
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Sep 10, 2017
@zzbot
Auto merge of #2768 - micbou:update-ycmd, r=puremourning
fd84aba 
[READY] Update ycmd

This new version of ycmd includes the following changes:

 - PR ycm-core/ycmd#795: add option to make relative paths in flags from extra conf absolute;
 - PR ycm-core/ycmd#802: fix compilation on Haiku;
 - PR ycm-core/ycmd#804: add libclang detection on FreeBSD;
 - PR ycm-core/ycmd#808: write python used during build before installing completers;
 - PR ycm-core/ycmd#810: support unknown languages from tags;
 - PR ycm-core/ycmd#811: update Universal Ctags languages list;
 - PR ycm-core/ycmd#814: resolve symlinks in extra conf glob patterns;
 - PR ycm-core/ycmd#815: update JediHTTP;
 - PR ycm-core/ycmd#816: update Boost to 1.65.0;
 - PR ycm-core/ycmd#819: filter and sort candidates when query is empty;
 - PR ycm-core/ycmd#820: improve LLVM root path search for prebuilt binaries;
 - PR ycm-core/ycmd#822: inline critical utility functions;
 - PR ycm-core/ycmd#824: do not sort header paths in filename completer;
 - PR ycm-core/ycmd#825: implement partial sorting;
 - PR ycm-core/ycmd#830: add max_num_candidates option;
 - PR ycm-core/ycmd#831: fix multiline comments and strings issues;
 - PR ycm-core/ycmd#832: update Clang to 5.0.0.

The `g:ycm_max_num_candidates` and `g:ycm_max_num_identifier_candidates` options are added to the documentation.

The link to ycmd extra conf is updated.

Fixes #2562.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2768)
<!-- Reviewable:end -->
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Sep 10, 2017
@zzbot
Auto merge of #2768 - micbou:update-ycmd, r=micbou
9ca755a 
[READY] Update ycmd

This new version of ycmd includes the following changes:

 - PR ycm-core/ycmd#795: add option to make relative paths in flags from extra conf absolute;
 - PR ycm-core/ycmd#802: fix compilation on Haiku;
 - PR ycm-core/ycmd#804: add libclang detection on FreeBSD;
 - PR ycm-core/ycmd#808: write python used during build before installing completers;
 - PR ycm-core/ycmd#810: support unknown languages from tags;
 - PR ycm-core/ycmd#811: update Universal Ctags languages list;
 - PR ycm-core/ycmd#814: resolve symlinks in extra conf glob patterns;
 - PR ycm-core/ycmd#815: update JediHTTP;
 - PR ycm-core/ycmd#816: update Boost to 1.65.0;
 - PR ycm-core/ycmd#819: filter and sort candidates when query is empty;
 - PR ycm-core/ycmd#820: improve LLVM root path search for prebuilt binaries;
 - PR ycm-core/ycmd#822: inline critical utility functions;
 - PR ycm-core/ycmd#824: do not sort header paths in filename completer;
 - PR ycm-core/ycmd#825: implement partial sorting;
 - PR ycm-core/ycmd#830: add max_num_candidates option;
 - PR ycm-core/ycmd#831: fix multiline comments and strings issues;
 - PR ycm-core/ycmd#832: update Clang to 5.0.0.

The `g:ycm_max_num_candidates` and `g:ycm_max_num_identifier_candidates` options are added to the documentation.

The link to ycmd extra conf is updated.

Fixes #2562.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2768)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@micbou @codecov-io @bstaletic @Valloric @zzbot