On Wed, Oct 19, 2022 at 09:42:55PM -0500, David Vernet wrote:
On Wed, Oct 19, 2022 at 04:20:22PM -0700, Kevin J. McCarthy wrote:That's fine, or I'll be happy to touch it up myself after this is resolved.Whatever will cause less churn / work for you is fine with me. I wouldn't mind throwing my name on a mutt commit, but if it will cause less back and forth for you to just write it how you'd like that's fine as well. If you do, please feel free to ping me for a review.
In that case, please do feel free to update the documentation too. It wasn't my intention to make it harder for you to have your name on these fixes and updates. :-)
It also may have been more correct to set i to INT_MAX in km_dokey() for the 0/negative case. We could do that, but I'd argue we have the reverse problem now: the behavior has been as-is for 20+ years. Even though the doc's say it doesn't timeout, it does, and it does end up checking current mailbox/buffy about once a minute.Understood, being a Linux kernel contributor I'm used to this constraint. We must reward our loyal users with the expectation that we won't pull the rug out from under them, even when "fixing" a bug to match what's documented :-) It is what it is, and honestly, fixing the documentation to describe the behavior and its implications is probably sufficient given the available workaround of setting a large timeout anyways. Perhaps adding a comment in the code is warranted as well.
Sounds fine, to at least acknowledge that precedence is overriding a more "correct" implementation.
I still think it would be better to just change the $timeout doc to say a 0/negative value will default to 60, and just change imap_check_mailbox() to do the same, to make minimal behavior changes right now.If you feel this is the best / lightest touch, I'm happy to send out a patch that does it. For my own clarification though, what would we gain from doing this if keepalive is also sending the noops? Wouldn't this just result in the same behavior, expect that we may even be sending multiple, redundant NOOPs?
I've been thinking about this quite a bit, and honestly I've gone back and forth.
My strongest resistance was worrying about regressions:- At first I forgot about the keepalive NOOPs being sent, but with those at least the connection won't be closed and will at least occasionally guarantee updates/expunges are received.
- The only other non-force imap_check_mailbox() is in imap_sync_mailbox(). But since $timeout can also be set very high I don't see how making "0" skip the NOOP should cause a problem here.
- Then there are the (IMAP using) folks who currently have $timeout set to 0. If there is any latency they've probably given up on that by now, but perhaps some with a local IMAP server have it set. Right now, they are seeing "immediate" updates whenever they scroll around. Changing to 60 will slow this down, but probably not as noticeably as waiting for $imap_keepalive.
The biggest problem is that Mutt has obfuscated the way IMAP current mailbox polling works, using a variable ($timeout) that it perhaps shouldn't have, without documenting it. Either way we ought to change this for <=0, and so it will be a "change in behavior".
After thinking it through, I've changed my mind and am not opposed to having it skip the NOOP in imap_check_mailbox(). I think making "a change" is justifiable given the horrible current behavior, and given that we document the change and reasoning well.
However, let's give a few days to see if anyone else on the list has feedback.
-- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
