Feat/respect xdg spec on all os #2627
Conversation
I am a bit hesitant about this change since I am unsure if any logic depends on a config folder needing to exist. But as far as I can tell nothing does and creating a empty config folder in someones dotfiles when they don't want any configuration for gitui seems like terrible UX.
I'm a little trepidatious about this particular change since I am not certain there is no logic relying on a config folder existing; though as far as I can tell there isn't. If it doesn't present any issues (or even if it does present minor issues) I am a strong advocate for not forcing an empty For an app as functional as |
|
Could also add a |
There was a problem hiding this comment.
Thank you for your PR!
I'm not a regular Windows user, so I didn't test the new behavior. The following is just based on code review.
I'm a little trepidatious about this particular change since I am not certain there is no logic relying on a config folder existing; though as far as I can tell there isn't.
I agree with that. There was a feature that created a null theme file a while back, but that has been removed in #1652. No need to create an empty directory.
I have a few remarks regarding the implementation.
src/args.rs
Outdated
| dirs::config_dir() | ||
| // List of potential config directories in order of priority | ||
| let config_dir_candidates = [ | ||
| env::var_os("XDG_CONFIG_HOME").map(PathBuf::from), |
There was a problem hiding this comment.
At least according to spec, XDG_CONFIG_HOME should only be considered if it's absolute. (Same for _CACHE...)
| for potential_dir in | ||
| candidates.into_iter().flatten().filter(|p| p.is_dir()) |
There was a problem hiding this comment.
Sorry, one more remark: At least for XDG, Path::is_dir isn't a precondition for considering that path. If it doesn't exist, create it (with permissions 0700)
There was a problem hiding this comment.
If, when attempting to write a file, the destination directory is non-existent an attempt should be made to create it with permission 0700.
This only applies when attempting to write a file.
When attempting to read a file, if for any reason a file in a certain directory is unaccessible, e.g. because the directory is non-existent, the file is non-existent or the user is not authorized to open the file, then the processing of the file in that directory should be skipped.
So believe this point is fairly moot when it comes to XDG_CONFIG_HOME.
That being said it does have some consideration for XDG_CACHE_HOME, but I think there is still some room for interpretation. I think you could consider that line to be referring to the creation of an app subdirectory and not to create the root directory itself. In which case that is already being done. Furthermore when it comes to creating good UX a user may have an XDG_CACHE_HOME defined which would result in that being the location to write to by default. But say the user wanted to keep XDG_CACHE_HOME only having the logs of particular apps creating a gitui folder in the fallback cache directory would allow them to filter it out.
I don't really imagine anyone wanting to do that, but it seems superior to have the option than not to.
There was a problem hiding this comment.
Yes, for XDG_CONFIG_HOME it does not make a difference as of now, as we don't create things there. For XDG_CACHE_HOME it does, though.
It worked before and does not work now. I'm sorry to say, but I don't think I'd want to approve this. If this were to fix a huge issue, okay, but this only addresses a (very) peculiar concern on windows.
Might remove this after further scrutinizing the XDG spec
Co-authored-by: Johannes Agricola <[email protected]>
This Pull Request fixes/closes #1498.
It changes the following:
XDG_CONFIG_HOMEas the first priority.XDG_CACHE_HOMEas the top prioritygituiconfig folder by default.I followed the checklist:
make checkwithout errors