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
