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

.editorconfig files with syntax errors can cause property lookup to fail, even if the file is in a directory beneath the "root" config #55

Open
craigbarnes opened this issue Apr 30, 2019 · 2 comments
Assignees
Labels

Comments

@craigbarnes
Copy link

craigbarnes commented Apr 30, 2019

The current logic in the ini_handler() function seems to be to check every directory beneath the target directory for an .editorconfig file, from /.editorconfig upwards. If it encounters a config with root=true it clears the previous values and continues the traversal. If it encounters a file with INI syntax errors, it stops traversing at that directory and returns an error code.

This logic allows an .editorconfig file that shouldn't have any effect at all on the return value (because it's in a directory beneath the root config) to effectively prevent all property lookups in every directory above it.

I realize this is a weird edge case and fairly unlikely to happen, but I think it's worth mentioning anyway.

@xuhdev xuhdev self-assigned this May 4, 2019
@xuhdev xuhdev added the bug label May 4, 2019
@xuhdev
Copy link
Member

xuhdev commented May 4, 2019

This is indeed an issue that should be fixed. Do you have a suggestion for an elegant fix?

@craigbarnes
Copy link
Author

craigbarnes commented May 6, 2019

The following options crossed my mind when I first noticed the issue:

  1. Keep track of errors and do the full bottom-up traversal without any early return, then work out if there was an error above the "root" level and return appropriately.
  2. Traverse the directories twice -- first backwards to find the "root", then forwards (from that point) to accumulate the properties.
  3. Just ignore the lines that aren't valid INI sections or properties.

I opted for option 3 in the dte implementation, although that may mean that it won't be able to pass the full test suite when I eventually hook it up...?

There seems to be plenty of precedent for just ignoring invalid lines though -- .gitconfig files, *.desktop files, systemd *.service files, etc. all ignore lines that don't contain a valid section/property and most parser libs (including inih) continue parsing past invalid lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants