Add support for XDG_CONFIG_HOME and AppData on Windows #3671
Conversation
samcoe
force-pushed
the
xdg-config
branch
2 times, most recently
from
c000bf6
to
6461354
Compare
There was a problem hiding this comment.
This is heading in a great direction. Thanks for working on this!
Left some comments about tightening up the migration logic and ensuring that it's entirely exercised in tests.
Note that if we advertise support for XDG spec, then we have to adhere to it as much as we can. That means avoiding storing non-config files under XDG_CONFIG_HOME. Currently, our only non-config file is state.yml, so that one should be migrated to XDG_STATE_HOME accordingly. #554 (comment)
|
@mislav thanks for the thorough review. I addressed your comments. Regarding, |
samcoe
force-pushed
the
xdg-config
branch
2 times, most recently
from
f34727f
to
99a31ff
Compare
| return false | ||
| } | ||
|
|
||
| var migrateConfigDir = func(oldPath, newPath string) { |
There was a problem hiding this comment.
I wish that this wouldn't be an overridable function anymore, but I understand that other pieces of our codebase are coupled to ConfigDir in tests and that they could inadvertently cause changes to the user's home directory.
There was a problem hiding this comment.
Yeah, unfortunately that is the case now. Hopefully in the future we can clean that up. I did add tests for this directly though so it isn't stubbed out in all tests.
| } | ||
|
|
||
| _ = os.MkdirAll(filepath.Dir(newPath), 0755) | ||
| _ = os.Rename(oldPath, newPath) |
There was a problem hiding this comment.
Should we also remove the orphaned ~/.config directory after the migration of ~/.config/gh to the new location?
We could simply try to os.Remove the parent. If it errors out, we ignore it.
There was a problem hiding this comment.
I don't think this is a good idea since it could remove files users want to keep. I actually have git, kitty, and nvim configuration files in ~/.config.
There was a problem hiding this comment.
os.Remove, like the rm command, is only able to remove empty directories. It would error out otherwise.
samcoe
force-pushed
the
xdg-config
branch
3 times, most recently
from
b002d4c
to
6d8c6c0
Compare
|
🎉 thanks @samcoe !! |
This PR adds support for storing
ghconfig files inXDG_CONFIG_HOMEas well asAppDataon Windows.cc #2470
closes #1944
closes #554
supersedes #2080