-
Notifications
You must be signed in to change notification settings - Fork 325
imapserver: Add CONDSTORE extension support #690
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: v2
Are you sure you want to change the base?
Conversation
Ref #687 which contained the previous version of this PR. |
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.
LGTM apart from these comments!
@@ -69,6 +96,19 @@ func (c *Conn) handleSelect(tag string, dec *imapwire.Decoder, readOnly bool) er | |||
} | |||
} | |||
|
|||
shouldSendModSeqStatus := c.enabled.Has(imap.CapIMAP4rev2) || c.server.options.caps().Has(imap.CapCondStore) |
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 don't think IMAP4rev2 has CONDSTORE folded in? Is there a reason to send mod seq when it's enabled?
msg.modSeq = mbox.highestModSeq | ||
mbox.highestModSeq++ |
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.
Shouldn't we bump mbox.highestModSeq
before setting the message's mod seq?
If we bump mbox.highestModSeq
after, then it's not the highest mod seq, it's the next mod seq.
@@ -324,6 +333,10 @@ func (mbox *MailboxView) Fetch(w *imapserver.FetchWriter, numSet imap.NumSet, op | |||
return | |||
} | |||
|
|||
if options.ChangedSince > 0 && msg.modSeq <= options.ChangedSince { |
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.
Shouldn't this be msg.modSeq < options.ChangedSince
? In other words, we want to send the message if CHANGEDSINCE equals MODSEQ.
var modifiedSeqNums []uint32 | ||
var modifiedMsgs []*message | ||
|
||
mbox.mutex.Lock() |
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.
Nit: we can use mbox.forEach
instead of manually locking/unlocking.
mbox.highestModSeq++ | ||
msg.modSeq = mbox.highestModSeq |
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.
Could we handle the mod seq bump inside msg.store
? This would make it so we don't forget bumping it if we call this function from elsewhere.
|
||
for i, seqNum := range modifiedSeqNums { | ||
msg := modifiedMsgs[i] | ||
mbox.Mailbox.tracker.QueueMessageFlags(seqNum, msg.uid, msg.flagList(), mbox.tracker) |
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.
Is there a reason to move the QueueMessageFlags
call outside of the mbox.forEach
loop?
msg.flagList
can only be called with the mailbox mutex locked. QueueMessageFlags
never blocks.
} | ||
|
||
if !flags.Silent && len(modifiedMsgs) > 0 { | ||
for i, seqNum := range modifiedSeqNums { |
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.
Instead of open-coding this, could we accumulate an imap.SeqSet
of modified message sequence numbers in the loop above, and pass that to mbox.Fetch
?
Implementing changes requested