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

Make sure that we have a valid path for DBManager #1610

Closed
wants to merge 1 commit into from

Conversation

guludo
Copy link
Contributor

@guludo guludo commented Apr 5, 2023

Otherwise, we run into problems when we need to use the path attribute and it is None (for example, in DBManager.add_message()).

Getting the path directly from the notmuch config file is not enough, because the user might be using the default location with no explicit entry in the file. As such, it is better to ask it directly to notmuch.

@pazz
Copy link
Owner

pazz commented Apr 6, 2023

Would it not be simpler, for this use case, to hard-code the same default value as notmuch does
as a fallback argument to settings.get_notmuch_setting?
I mean, if that value is not in notmuch's config, the binary will also fall back to a hardc-oded default wouldn't it?

@guludo
Copy link
Contributor Author

guludo commented Apr 6, 2023

Would it not be simpler, for this use case, to hard-code the same default value as notmuch does
as a fallback argument to settings.get_notmuch_setting?
I mean, if that value is not in notmuch's config, the binary will also fall back to a hardc-oded default wouldn't it?

I'm not sure about that, since there isn't exactly simple hardcoded value to use. There is a logic notmuch follows to get the database path. I extracted the following from the notmuch-config(1) manpage:

DATABASE LOCATION
       Notmuch database search order:

       1. Directory specified by NOTMUCH_DATABASE environment variable.

       2. Directory specified by config key database.path.

       3. $XDG_DATA_HOME/notmuch/<profile> where <profile> is defined  by  NOTMUCH_PROFILE  environment  variable  if  set,
          $XDG_DATA_HOME/notmuch/default otherwise.

       4. Directory specified by MAILDIR environment variable.

       5. $HOME/mail

So I think it is rather simpler to just ask it directly via notmuch config instead of reimplementing that logic. And if some version of notmuch changes how it finds the database location, there is no extra work from our part.

@guludo
Copy link
Contributor Author

guludo commented Apr 6, 2023

Oops. I just realized there is an issue with the current patch, forgot to remove the line indexpath = settings.get_notmuch_setting('database', 'path'). Will send a new version with that removed.

Otherwise, we run into problems when we need to use the path attribute
and it is None (for example, in DBManager.add_message()).

Getting the path directly from the notmuch config file is not enough,
because the user might be using the default location with no explicit
entry in the file.

Since there is a bit of logic notmuch does to get the database path (see
section "DATABASE LOCATION" in man page for notmuch-config(1)), it is
better to ask it directly via "notmuch config" instead of reimplementing
that same logic.
@guludo
Copy link
Contributor Author

guludo commented Apr 6, 2023

Oops. I just realized there is an issue with the current patch, forgot to remove the line indexpath = settings.get_notmuch_setting('database', 'path'). Will send a new version with that removed.

Force-pushed with the change above. I also took the opportunity to elaborate more on the reason for using notmuch config in the commit message.

@pazz
Copy link
Owner

pazz commented Apr 13, 2023

It's taken me a while but I am generally on board now with calling the notmuch binary for stuff.
However, I'd much rather have this done systematically and not just hardcoded for this one use-case.

Also, the main UI code as well as the settings manager are already too bloated, and all this could use some refactoring.
I am not sure how best to address this but some other use cases for calling the notmuch binary are:

  • reading notmuch config settings, instead of parsing its config fir directly)
  • making tagging operations; This is sometimes error prone as done now. For example, I keep seeing individual mails in my inbox and alot's untagging mech does not seem to work for those messages. Occasionally I then just call notmuch on the command line. We could simply do this all the time, or at least have this as an option in alot.

@guludo
Copy link
Contributor Author

guludo commented Apr 14, 2023

* reading notmuch config settings, instead of parsing its config fir directly)

Doing this one with #1615.

@guludo
Copy link
Contributor Author

guludo commented Apr 16, 2023

  • making tagging operations; This is sometimes error prone as done now. For example, I keep seeing individual mails in my inbox and alot's untagging mech does not seem to work for those messages. Occasionally I then just call notmuch on the command line. We could simply do this all the time, or at least have this as an option in alot.

Do you know of a way we can reproduce it? Is there an issue already created for this? I'm thinking of closing this PR, since #1615 was merged.

@pazz
Copy link
Owner

pazz commented Apr 17, 2023 via email

@guludo guludo closed this Apr 17, 2023
@guludo guludo deleted the pr/fix-db-path branch April 18, 2023 13:17
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.

None yet

2 participants