On Thu, Oct 20, 2022 at 09:57:56AM -0700, Kevin J. McCarthy wrote: > 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. :-)
Great, I'll take care of it in a follow-on version of the patch set (after giving folks on the list a few more days to respond). > > > 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. Agreed, I think that reasoning is sound. > - 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. Hmmm yeah, the local IMAP server users is a good point. Between rounding 0 -> 60, and applying 0 as it's currently documented / proposed in this patch, it feels like the latter is a more reasonable solution. Local IMAP users could always set $timeout = 1 or something if they want similar behavior (though that doesn't mean that doing so isn't a regression for the users that are expecting $timeout = 0 to have the behavior being "fixed" by this patch). > 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. This all sounds good to me. Let's give it a few days to see if anyone chimes in, and we can make a final decision and/or further discuss at that time. Thanks, David
