Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow separate mail and state paths #33
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
base: main
Are you sure you want to change the base?
Allow separate mail and state paths #33
Changes from all commits
1e18d7b
c50bc1e
d27a82c
2e793b4
b77f15e
bba235c
a559001
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user types an absolute path for the
mail_dir
config option but misspells part of the path, causingstrip_prefix
to fail, they could experience some nasty consequences. We should keep/revise theMailDirNotASubdirOfNotmuchRoot
error here in that case instead of swallowing. We can still support the config option being a relative path by checking if itsis_absolute()
returns false and handling that slightly differently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there's a lot we can do really; how do we know if its a error if the path is valid?
The best I can think to do is for a configured
mail_dir
, make sure it exists and bail out if not. Of course its still possible to typo, but at least it won't plow on regardless.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've added a commit that does just that if
mail_dir
is provided (if we compute it, it doesn't have to exist beforehand).The more I think about the more I think this will be rare, because most of the time I expect that people would mess with their config until its set up how they like, and would then walk away, so its unlikely that they're going to get a path wrong in a system they've already set up. Its still good to have a safety for an obviously-wrong situation though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this isn't quite what I meant. I think I understand why you changed it like this now, to specially handle the case for if
mail_dir
and the notmuch root directory are the same, is that right? According to the docs, it seems likestrip_prefix
will yield an emptyPathBuf
rather thanErr
for this case, so there's actually no need to specially handle this case.https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.strip_prefix
But the consequences of removing the
MailDirNotASubdirOfNotmuchRoot
error as this commit currently exists would result in something like the following:~/Maildir
and mujmap is configured so that its mail dir is~/Maildir/mujmap
. So far so good./mnt/maildrive/Maildir
, but forget to update mujmap, which still points to~/Maildir
strip_prefix
fails here, mujmap silently both decides that it owns the entire mail database, and insists on placing mail files in the old~/Maildir
and trying to add them to the notmuch database. Potentially destructive chaos ensues.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really following your scenario, and I'm not sure if that's because its imprecisely described, or if its because we're not quite in agreement about what we're doing here and so talking past a little bit. I will try and write down what I think is happening here, and hopefully I will understand by the end.
I'm seeing
mail_dir
(andstate_dir
andcache_dir
) as a kind of override; if you set it, it will be used over any other value.So in your scenario, if they've set
mail_dir
to something~/Maildir/mujmap
then they get what they ask for, regardless of whether or not its a subdir of the notmuch root. This has to be this way to support "side-by-side" configs (like XDG).If on the other hand they haven't set
mail_dir
explicitly and its implied (by "directory" mode), then I don't see how mujmap could know this. The paths haven't changed, just the files were moved out without its knowledge.Going back to the code, what I think I'm doing is that if the maildir is under the notmuch root, then the query becomes
path:xxx/**
wherexxx
is the path of the maildir relative to the notmuch root, but if its outside the notmuch root, we use the globalpath:**
because what else can we do? The code is effectively converting the emptyPathBuf
andErr
to mean the same thing: use a global query.Ahh, maybe that's it. Are you talking about the case where the user has explicitly set
mail_dir
to be under the notmuch root? And then they move it later, which will change the path prefix. That could be a problem (I have not thought hard about this yet). I hadn't considered that someone could deliberately set things up this way because I only know of yours and my use cases! Hmm.I wonder if that's the real problem here, that the path prefix is kind of baking too much info about the existing paths into the notmuch database, and if they change we don't know how to hook it up properly?
If so I'm not sure we can fix this here. Some possible solutions (off the top of my head):
mail_root
to follow mujmap'smail_dir
so we can use the global path query only (breaks anyone that has multiple maildirs in a single notmuch index)Something else might be to do a sanity check. If there's a bunch of stuff in the index, but none of the paths exist on disk, abort and warn the user. Hmm, and thinking more on that, maybe the answer is that if a message is not on disk, but is in notmuch, and is on the server, re-download it.
I'm not really arguing against the need for
MailDirNotASubdirOfNotmuchRoot
as such, I just don't know when it should fire. But honestly, I still wonder if this is a thing. Is moving things around a common thing that notmuch people like to do? (I am only a new notmuch person myself). If its not, then maybe noticing something and off and forcing a resync is the right way.(I wrote this over the course of a busy and distracted day so I feel like everything has fallen out of my head even more than the original plan fell out of my head in the last couple of months; I'm really struggling. I'll try to think about this more over the weekend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe this is where the confusion stems. notmuch doesn't actually support this configuration; everything has to be a subdirectory of
database.mail_root
.database.path
used to refer to the same thing asdatabase.mail_root
, but is now independent and only refers to the actual database files if both of these options are specified (see here). If you've somehow managed to index files outside of the root, I think that's actually undefined behavior and probably not good. 😕 But it seems like the best way to prevent unintended behavior is to catch this discrepancy between the two configurations as early as possible, henceMailDirNotASubdirOfNotmuchRoot
.My example was mostly just me trying to address what I thought your misconception was rather than your actual example, rather than me trying to necessarily specifically prevent these kinds of database migration issues, but I do agree that a sanity check for a really large unexpected change like this would be a great to have feature.
That's okay, me too 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that's where the confusion comes from; if using
NOTMUCH_PROFILE
these things can all be separated, and indeed, that's a large part of the reason I'm even doing this work!My current notmuch config with profiles, fyi:
And then I run mujmap with this PR as:
env NOTMUCH_PROFILE=home mujmap --path /home/robn/.config/mujmap/home.toml
.mail_dir
is not set there, so its being taken from notmuch.I did consider that maybe the intent of notmuch's profile support was still that the database was under the maildir, and maybe I was indeed getting accidental behaviour. The config doc is not clear on this, though perhaps that's just it not answering a question that no one asked. However, the tests for split-path configs seems to imagine that the database path and mail dir can be in very different locations, as perhaps does the commit that introduced it, and a code read seems to support that too.
I think I don't have any more to add. I (of course) think I'm right about what notmuch is supposed to be able to do here; if you're not persuaded then I'm not sure what we do next.