Skip to content

Feat/respect xdg spec on all os #2627

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
Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Feat/respect xdg spec on all os #2627

wants to merge 15 commits into from
+124 −32

Conversation

KlassyKat
Copy link

This Pull Request fixes/closes #1498.

It changes the following:

  • Unifies OS logic for loading config.
  • Adds a ordered list of locations to search for config.
  • List includes XDG_CONFIG_HOME as the first priority.
  • Implements the same logic for getting the cache_path with XDG_CACHE_HOME as the top priority
  • No longer creates an empty gitui config folder by default.

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@KlassyKat
Initial Implementation: Create a set of potential config locations to…
beb71fb 
… search for
@KlassyKat
Apply the same logic to get_app_cache_path
8e307d3
@KlassyKat
Refactor to make logic more reusable and testable
fb935b7
@KlassyKat
Realize I don't know how to spell candidates
819b4ca
@KlassyKat
refactor to be a little cleaner
cbc388d
@KlassyKat
don't create an empty config folder
299015a 
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.
@KlassyKat
Add unit tests
9ea0d0d
@KlassyKat
Update docs to reflect new behavior
cde05b1
@KlassyKat
Add changelog item
e8440c2
@KlassyKat
Copy link
Author

KlassyKat commented Apr 29, 2025 •
edited
Loading

No longer creates an empty gitui config folder by default.

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 gitui config folder to exist for any users that don't feel any need to configure gitui. I see this as an extremely annoying UX for anyone that doesn't want/feel the need for any configuration of gitui and likes to keep their dotfiles clean.

For an app as functional as gitui out of the box I don't see the portion of users who don't feel the need for any additional config as a particularly small group.

KlassyKat
KlassyKat commented Apr 29, 2025
View reviewed changes
@KlassyKat
Copy link
Author

KlassyKat commented Apr 29, 2025 •
edited
Loading

Could also add a $GITUI_CONFIG_DIR env var to look for at the top of the priority list, but I see that as maybe a bit overkill for what is a relatively simple config structure.

naseschwarz
naseschwarz suggested changes Apr 30, 2025
View reviewed changes
Copy link
Contributor

@naseschwarz naseschwarz left a comment

Choose a reason for hiding this comment

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

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),
Copy link
Contributor

Choose a reason for hiding this comment

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

At least according to spec, XDG_CONFIG_HOME should only be considered if it's absolute. (Same for _CACHE...)

src/args.rs Outdated
];

get_path_from_candidates(config_dir_candidates)
.map_err(|_| anyhow!("failed to find os config dir."))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really the "os config dir" we were looking for, right?

naseschwarz
naseschwarz suggested changes Apr 30, 2025
View reviewed changes
src/args.rs
Comment on lines +157 to +158
for potential_dir in
candidates.into_iter().flatten().filter(|p| p.is_dir())
Copy link
Contributor

@naseschwarz naseschwarz Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

@KlassyKat KlassyKat marked this pull request as draft May 2, 2025 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naseschwarz naseschwarz naseschwarz requested changes

Assignees
No one assigned
Labels
needs_work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Follow XDG spec on windows
2 participants
@KlassyKat @naseschwarz