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

[profiled] Config dir change from .profiled to .config/profiled JB#9225 #3

Merged
merged 1 commit into from Jan 31, 2023

Conversation

dsuni
Copy link

@dsuni dsuni commented Jan 16, 2023

Also adds a oneshot script that moves the old files to the new directory.

database.c Outdated
if (snprintf(path, sizeof(path), "%s/.config/profiled", getenv("HOME")) < 0)
{
log_err("Path %s/.config/profiled is too long. Max size %lu\n", getenv("HOME"), sizeof(path));
snprintf(path, sizeof(path), "/tmp");

Choose a reason for hiding this comment

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

This goes all wrong now...

Old code: protected against side effects of NULL $HOME and dontcare if $HOME is bonkers long (under assumption that it will be checked later on or something).

New code: removes NULL protection, adds incorrect "path too long" check (snprintf returns number of chars it would have written if there were space, negative values are something else) and superfluous parentheses for sizeof...

Copy link

Choose a reason for hiding this comment

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

Please don't hardcode $HOME/.config but use $XDG_CONFIG_HOME

rpm/move-profiled-config.sh Outdated Show resolved Hide resolved
Copy link

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

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

Please follow the XDG spec properly.

database.c Outdated
if (snprintf(path, sizeof(path), "%s/.config/profiled", getenv("HOME")) < 0)
{
log_err("Path %s/.config/profiled is too long. Max size %lu\n", getenv("HOME"), sizeof(path));
snprintf(path, sizeof(path), "/tmp");
Copy link

Choose a reason for hiding this comment

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

Please don't hardcode $HOME/.config but use $XDG_CONFIG_HOME

rpm/move-profiled-config.sh Outdated Show resolved Hide resolved
database.h Outdated
@@ -85,6 +85,8 @@ void database_reload(void);

void database_set_restart_request_cb(void (*cb)(void));

void migrate_configs (const char *legacy, const char *current);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to expose in the header?

database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
Copy link

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

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

Mostly only what @pvuorela already but also use get_home_dir instead of detecting if $HOME is set your self.

I would also prefer if the naming of the old and legacy path is the same all around.

database.c Outdated
{
static char path[256] = "";

if( *path == 0 )
{
snprintf(path, sizeof path, "%s/.profiled", getenv("HOME") ?: "/tmp");
snprintf(path, sizeof(path), "%s/.profiled", getenv("HOME") ?: "/tmp");
Copy link

Choose a reason for hiding this comment

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

Please use GLib.get_home_dir instead.

database.c Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.h Outdated Show resolved Hide resolved
Copy link

@spiiroin spiiroin left a comment

Choose a reason for hiding this comment

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

There are comments about style issues and midfunction returns that make my eyes sore but... those do not affect functionality -> feel free to ignore.

But do add that "old dir exists" check as condition before attempting migration.

database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
Copy link

@spiiroin spiiroin left a comment

Choose a reason for hiding this comment

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

Couple of cosmetic nits, approving already.

database.c Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
database.c Outdated Show resolved Hide resolved
If the config files can not be found in the new location, it will attempt to
move the files from the old location.
@pvuorela pvuorela merged commit 97eff15 into sailfishos:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants