Re: [PATCH] Use IMAP MOVE extension if available

[Eric Abrahamsen <eric@ericabrahamsen.net>]

Nikolaus Rath <Nikolaus@rath.org> writes:

> On Aug 02 2015, Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>> Nikolaus Rath <Nikolaus@rath.org> writes:
>>
>>> On Jul 27 2015, Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>>>> diff --git a/lisp/nnimap.el b/lisp/nnimap.el
>>>> index 161a6b4..f8e6e3e 100644
>>>> --- a/lisp/nnimap.el
>>>> +++ b/lisp/nnimap.el
>>>> @@ -489,11 +489,18 @@ textual parts.")
>>>>  		      (when (functionp (nth 2 credentials))
>>>>  			(funcall (nth 2 credentials)))
>>>>  		      ;; See if CAPABILITY is set as part of login
>>>> -		      ;; response.
>>>> -		      (dolist (response (cddr login-result))
>>>> -			(when (string= "CAPABILITY" (upcase (car response)))
>>>> -			  (setf (nnimap-capabilities nnimap-object)
>>>> -				(mapcar #'upcase (cdr response))))))
>>>> +		      ;; response.  If it wasn't, specifically request
>>>> +		      ;; it.
>>>> +		      (when
>>>> +			  (catch 'caps
>>>> +			    (dolist (response (cddr login-result))
>>>> +			      (when (string= "CAPABILITY" (upcase (car response)))
>>>> +				(throw 'caps (setq capabilities (cdr response)))))
>>>> +			    (let ((req (nnimap-command "CAPABILITY")))
>>>> +			      (when (car req)
>>>> +				(throw 'caps (setq capabilities (cdaddr req))))))
>>>> +			(setf (nnimap-capabilities nnimap-object)
>>>> +			      (mapcar #'upcase capabilities))))
>>>>  		  ;; If the login failed, then forget the credentials
>>>>  		  ;; that are now possibly cached.
>>>>  		  (dolist (host (list (nnoo-current-server 'nnimap)
>>>
>>>
>>> This looks a lot more complex, but as far as I can tell it still
>>> needlessy asks for capabilities if they have been supplied in the
>>> response code of OK response instead of a in separate, untagged
>>> response. So is it really worth the extra complexity, if in many cases
>>> we still issue an explicit CAPABILITY command?
>>>
>>> (For everyone unfamiliar with RFC3501, what I mean is that this:
>>>
>>> * CAPABILITY IMAP4rev1 AND SOME MORE \r\n
>>> 1 OK Logged in\r\n
>>>
>>> will be parsed correcly but this:
>>>
>>> 1 OK [CAPABILITY IMAP4rev1 AND SOME MORE] Logged in\r\n
>>>
>>> will still cause Gnus to issue a separate CAPABILITY command).
>>
>> Okay, I pushed the CAPABILITY patch, and the MOVE patch right after
>> that. Sorry this is all going so slowly!
>
> Don't worry, I'm not in a rush. Luckily I can run the patches locally
> until they applied upstream :-).
>
>> I still think that we'll be able to come up with something that doesn't
>> unconditionally issue an extra CAPABILITY command. My patch was more
>> complex, but at least it avoided redundancy in *some* cases -- now we're
>> always doing the extra command.
>
> I don't understand this worry about one additional back and forth during
> *login*. For me, starting Gnus takes about 2 seconds, and issuing the
> CAPABILITY command takes about 160 ms. Is it really worth optimizing
> this?

No, I guess you're right. It just irks, a little.

>> Are you sure that `nnimap-parse-response' and `nnimap-parse-line' can't
>> handle the bracket case above?
>
> Depends what you mean. The data does end up in the list that's returned
> by nnimap-parse-response, because (as the comment says), it just
> "lightly" converts the response to a list of strings and lists of
> string. The problem is the "lightly" - apparently this function
> "succeeds" no matter what data you give it, even if this data completely
> fails the assumptions that went into writing that function. Looking at
> the code, I am not able to tell (in general) at which position the
> response code is going to be. So the best I can do is just use the
> location at which it ends up with my server - but I don't think that's a
> good idea.
>
> That's also the reason why I did not use nnimap-parse-response at all
> for parsing the NAMESPACE command (in my other patch). In that case,
> "parsing" the return value of nnimap-parse-response is actually a lot
> harder than parsing the IMAP response directly. For example, the
> nnimap-parse-response return value contains strings with opening parens,
> but no strings with the corresponding closing parens.
>
>> I haven't had time to test yet, but looking at the code it seems like
>> it ought to be able to handle it. If there are multiple commonly-used
>> formats for IMAP server responses, it seems like we really ought to be
>> handling them at the base level, when parsing responses.
>
> Making nnimap-parse-response into a proper parser would certainly be a
> good idea, but that's probably better left as a separate patch.

Oh certainly -- I'm not in any hurry to get stuck into that! But at some
point, yes, it would probably be nice.

>> I'll admit I only started learning about IMAP about six months ago, so
>> I'm happy to take other people's word on how all this works. Out of
>> curiosity, is there a particular server that returns the kind of
>> response you note above? I suppose we should be making a list of
>> known-good IMAP servers that Gnus can interact happily with.
>
> This is mail.messagingengine.com, the IMAP server from
> www.fastmail.com. IIRC they are using Cyrus.

That's pretty mainstream stuff, and I've heard Cyrus referred to as one
of the "sane" IMAP servers, so we should definitely be trying to play
nice with it...

Thanks,
Eric