Skip to content

Comments

Respect notmuch-config(1) FILES specs.#33

Open
ferdinandyb wants to merge 1 commit intoaperezdc:masterfrom
ferdinandyb:master
Open

Respect notmuch-config(1) FILES specs.#33
ferdinandyb wants to merge 1 commit intoaperezdc:masterfrom
ferdinandyb:master

Conversation

@ferdinandyb
Copy link
Contributor

WIP

Questions and comments:

  • I would make --config supersede any environment variables (the same as the notmuch command itself)
  • To actually conform to the specs, I'd need to check at each stage whether the file actually exists and go to the next one if it doesn't, otherwise we'll never get to $HOME/.notmuch-config. I did a quick search of how you do this in C, but as there are multiple ways and nothing really popped out as appropriate, what do you prefer for that? In the end I need a function that returns a true or false :)
  • I'd probably add a G_OPTION_ARG_NONE type --debug flag which prints which config file / database is actually used in the end, or maybe at that point maybe the "check file exists" func could print each file it tries, otherwise I think it might get confusing if things don't work as expected.

Copy link
Owner

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

@ferdinandyb I agree that following the same steps as Notmuch to find the condiguration file would be great, thanks for looking into this. That being said, this patch needs a bit of work before we can merge it. I have written a couple of comments with some hints on how to continue.

Looking forward to an updated patch ⚒️

else if(!notmuch_config_path_lookupvar)
config_path = g_strdup_printf ("%s/.notmuch-config", g_get_home_dir ());
else if (g_getenv ("NOTMUCH_PROFILE")){
if (g_getenv("XDG_CONFIG_HOME")){
Copy link
Owner

Choose a reason for hiding this comment

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

You should use g_get_user_config_dir() which will work even if the XDG_CONFIG_HOME environment variable is undefined. Then you don't need to check if g_getenv() returned a non-NULL value.

To check whether the file exists you can use g_file_new_build_filename() to calculate the path and create a GFile for it at the same time, then use g_file_query_exists(). Finally, if the file is present, its full path can be obtained with g_file_get_path().

}
else
config_path = notmuch_config_path_lookupvar;
config_path = g_strdup_printf ("%s/.notmuch-config", g_get_home_dir ());
Copy link
Owner

Choose a reason for hiding this comment

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

I know the path was already being calculated in this way, but it would be better to use g_build_filename() instead:

Suggested change
config_path = g_strdup_printf ("%s/.notmuch-config", g_get_home_dir ());
config_path = g_build_filename (g_get_home_dir (), ".notmuch-config", NULL);

@ferdinandyb
Copy link
Contributor Author

Thanks for the followup! It is indeed just half-done. Could you also check my question about the config override and the debug flag?

@aperezdc aperezdc force-pushed the master branch 2 times, most recently from 8efabc7 to 569becc Compare October 13, 2022 15:01
@aperezdc
Copy link
Owner

Thanks for the followup! It is indeed just half-done. Could you also check my question about the config override and the debug flag?

My two cents:

  • Yes, it makes sense that --config is more important and takes precedence.
  • I think we do not need a --debug option, you can use g_debug() and by default debug messages are not shown, and setting e.g. G_MESSAGES_DEBUG=all in the environment can then be used to have them shown. As a bonus, GLib will handle adding timestamps, process identifier, and a few other goodies.

@ferdinandyb
Copy link
Contributor Author

perfect, I'll likely take a week or two to follow up

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.

2 participants