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

Support XDG base directory specification #2384

Closed

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Dec 19, 2019

This is a poc implementation for checking $XDG_CONFIG_HOME and loads the $XDG_CONFIG_HOME/ctags/*.ctags (fixes #89). But yes, I know the current implementation is not compliant to the specification (when $XDG_CONFIG_HOME is empty or unset, it should fallback to ~/.config/ctags). Note that we still have to use the .ctags extension (can't use $XDG_CONFIG_HOME/ctags/config) but is reasonable to split the config per languages.

EDIT: Now the implementation is compliant to the XDG base directory specification (I believe).

@coveralls
Copy link

coveralls commented Dec 19, 2019

Coverage Status

Coverage increased (+0.002%) to 86.375% when pulling 78b7a9b on itchyny:support-xdg-config-home into e216bb4 on universal-ctags:master.


return full_path;
return combinePathAndFile(envval, path);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we should not fallback to /ctags when the environment variable is not set.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

I would like to introduce a fix for this important suggestion first.
I made a pull request for the fix. See #2385.

@codecov-io
Copy link

codecov-io commented Dec 19, 2019

Codecov Report

Merging #2384 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2384      +/-   ##
==========================================
+ Coverage   86.25%   86.25%   +<.01%     
==========================================
  Files         176      176              
  Lines       35740    35738       -2     
==========================================
- Hits        30828    30827       -1     
+ Misses       4912     4911       -1
Impacted Files Coverage Δ
main/options.c 82.46% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e216bb4...78b7a9b. Read the comment docs.

@masatake masatake self-assigned this Dec 19, 2019
@masatake masatake self-requested a review December 20, 2019 03:01
masatake added a commit to masatake/ctags that referenced this pull request Dec 23, 2019
… name

combinePathAndFile takes two C strings.
The first one PATH was used in an expression: path [strlen(path) - 1] .
If PATH is an empty, this can cause a buffer overrun.

This change adds a guard-condition for such processing.

Inspired from the comment submitted by @itchyny in universal-ctags#2384.
masatake added a commit to masatake/ctags that referenced this pull request Dec 23, 2019
Suggested by @itchyny in universal-ctags#2384.

When $HOME is set but it is an empty string,
masatake added a commit to masatake/ctags that referenced this pull request Dec 23, 2019
masatake added a commit to masatake/ctags that referenced this pull request Dec 23, 2019
Suggested by @itchyny in universal-ctags#2384.

When $HOME is set but it is an empty string,
masatake added a commit to masatake/ctags that referenced this pull request Dec 23, 2019
…d file name

combinePathAndFile takes two C strings.
The first one PATH was used in an expression: path [strlen(path) - 1] .
If PATH is an empty, this can cause a buffer overrun.

This change adds a guard-condition for such processing.

Inspired from the comment submitted by @itchyny in universal-ctags#2384.
masatake added a commit to masatake/ctags that referenced this pull request Dec 23, 2019
@masatake
Copy link
Member

not compliant to the specification

So how we should do?

A. this change is not compliant to the specification, therefore this pull request is just for discussion, not for merging.
B. this change is not compliant to the specification, however, we should merge this because this change is useful.

For A, I have no strong comment. Simply say, I don't want to take much time for this topic.
For B, we need much more documents for the usefulness. The documents explain introducing this change is much more valuable than the incompliant. I think in the future we will get a bug report "ctags configuration loader isn't compliant to XDG standard". I think I cannot respond to the bug report.
The document may help me.

I want a test case. I can give you hints for writing. Any way I would like to hear the basic direction, A or B.

@itchyny
Copy link
Contributor Author

itchyny commented Dec 24, 2019

I added ~/.config/ctags fallback to follow the specification (I was just unconfident that I can write slash in .path).

@@ -0,0 +1,12 @@
# Copyright: 2017 Masatake YAMATO
Copy link
Member

@masatake masatake Dec 24, 2019

Choose a reason for hiding this comment

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

I didn't write this script.

@masatake
Copy link
Member

masatake commented Dec 24, 2019

Thank you for adding the test case.

Note that we still have to use the .ctags extension (can't use $XDG_CONFIG_HOME/ctags/config) but is reasonable to split the config per languages.

Does this violate the specification? or Is it acceptable in the specification?

I would like you to add minimum code for satisfying the specification.
.ctags loader is already complicated.

@itchyny
Copy link
Contributor Author

itchyny commented Dec 24, 2019

Does this violate the specification?

No. It's just a note for users (including me) who is not used to the behavior of universal-ctags. I believe the current implementation is compliant to the spec.
I'd like to hear some opinions from those who might be interested in XDG config support. @majutsushi @fishman @blueyed @justinmk @cweagans @dsifford (who commented in #89).

@@ -1,12 +1,10 @@
# Copyright: 2017 Masatake YAMATO
Copy link
Member

Choose a reason for hiding this comment

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

Copyright notice is neeed, but is not mine.

@@ -1,11 +1,9 @@
# Copyright: 2017 Masatake YAMATO
Copy link
Member

Choose a reason for hiding this comment

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

Copyright notice is neeed, but is not mine.

@masatake
Copy link
Member

Thank you for updating.

@majutsushi
Copy link

Looks reasonable to me.

@masatake
Copy link
Member

@majutsushi, thank you for veryfing.

This pull request is mostly ready for merging.
However, I would like to keep the history of our repository simple; I don't want to introduce try-and-error commits like 569c087 and 78b7a9b.

So I rearranged the commits in this pull request and made a new pull request. @itchyny, could you look into #2386?

@masatake
Copy link
Member

Merged via #2386. Thank you.

@masatake masatake closed this Dec 28, 2019
@itchyny itchyny deleted the support-xdg-config-home branch December 28, 2019 16:26
davidosomething added a commit to davidosomething/dotfiles that referenced this pull request Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use XDG dirs spec for data/config
5 participants