bug#43129: 25.2; Typo in lisp/gnus/nnimap.el

bug#43129: 25.2; Typo in lisp/gnus/nnimap.el

[Sean Connor]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

In commit fc9206b73a254a400245578b94542cfe82c68e9c, when IMAP MOVE
extension support was added,

the line

(or (nnimap-find-uid-response "COPYUID" (cadr result))

was replaced with

(or (nnimap-find-uid-response "COPYUID" (caddr result))

which results in a significant slowing of internal-move-group article
movement as the call to nnimap-find-uid-response always fails as caddr
always returns nil, AFAICT based on testing with example server
responses given in IMAP RFCs and those from two different IMAP servers,
leading Gnus to make a slow call to nnimap-find-article-message-id
insead of using the article number provided by the COPYUID response.

Simple patch, which undoes the switch from cadr to caddr:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Simple patch --]
[-- Type: text/diff, Size: 696 bytes --]

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index be8ad9a672..baf90d38ad 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -986,7 +986,7 @@ nnimap-request-move-article
                 (when (and (car result) (not can-move))
                   (nnimap-delete-article article))
                 (cons internal-move-group
-                      (or (nnimap-find-uid-response "COPYUID" (caddr result))
+                      (or (nnimap-find-uid-response "COPYUID" (cadr result))
                           (nnimap-find-article-by-message-id
                            internal-move-group server message-id
                            nnimap-request-articles-find-limit)))))

[-- Attachment #3: Type: text/plain, Size: 89 bytes --]


Cautious patch, which would handle cases where caddr is appropriate, if
there are any:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Cautious patch --]
[-- Type: text/diff, Size: 775 bytes --]

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index be8ad9a672..cea8988f81 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -986,7 +986,8 @@ nnimap-request-move-article
                 (when (and (car result) (not can-move))
                   (nnimap-delete-article article))
                 (cons internal-move-group
-                      (or (nnimap-find-uid-response "COPYUID" (caddr result))
+                      (or (nnimap-find-uid-response "COPYUID" (cadr result))
+                          (nnimap-find-uid-response "COPYUID" (caddr result))
                           (nnimap-find-article-by-message-id
                            internal-move-group server message-id
                            nnimap-request-articles-find-limit)))))

[-- Attachment #5: Type: text/plain, Size: 6 bytes --]


sean

bug#43129: 25.2; Typo in lisp/gnus/nnimap.el

[Eric Abrahamsen]

Sean Connor <sconnor005@allyinics.org> writes:

> In commit fc9206b73a254a400245578b94542cfe82c68e9c, when IMAP MOVE
> extension support was added,
>
> the line
>
> (or (nnimap-find-uid-response "COPYUID" (cadr result))
>
> was replaced with
>
> (or (nnimap-find-uid-response "COPYUID" (caddr result))
>
> which results in a significant slowing of internal-move-group article
> movement as the call to nnimap-find-uid-response always fails as caddr
> always returns nil, AFAICT based on testing with example server
> responses given in IMAP RFCs and those from two different IMAP servers,
> leading Gnus to make a slow call to nnimap-find-article-message-id
> insead of using the article number provided by the COPYUID response.
>
> Simple patch, which undoes the switch from cadr to caddr:
>
> diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
> index be8ad9a672..baf90d38ad 100644
> --- a/lisp/gnus/nnimap.el
> +++ b/lisp/gnus/nnimap.el
> @@ -986,7 +986,7 @@ nnimap-request-move-article
>                  (when (and (car result) (not can-move))
>                    (nnimap-delete-article article))
>                  (cons internal-move-group
> -                      (or (nnimap-find-uid-response "COPYUID" (caddr result))
> +                      (or (nnimap-find-uid-response "COPYUID" (cadr result))
>                            (nnimap-find-article-by-message-id
>                             internal-move-group server message-id
>                             nnimap-request-articles-find-limit)))))
>
>
> Cautious patch, which would handle cases where caddr is appropriate, if
> there are any:
>
> diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
> index be8ad9a672..cea8988f81 100644
> --- a/lisp/gnus/nnimap.el
> +++ b/lisp/gnus/nnimap.el
> @@ -986,7 +986,8 @@ nnimap-request-move-article
>                  (when (and (car result) (not can-move))
>                    (nnimap-delete-article article))
>                  (cons internal-move-group
> -                      (or (nnimap-find-uid-response "COPYUID" (caddr result))
> +                      (or (nnimap-find-uid-response "COPYUID" (cadr result))
> +                          (nnimap-find-uid-response "COPYUID" (caddr result))
>                            (nnimap-find-article-by-message-id
>                             internal-move-group server message-id
>                             nnimap-request-articles-find-limit)))))
>

Thanks for this report! Can you tell us which IMAP servers you've
tested this on? I just tried Dovecot, and the "(cadr result)" fix works
properly there. Unless we know there are some servers where "(caddr
result)" is appropriate (I wonder what server Nikolaus was using), I'm
inclined to put the simpler fix in.

bug#43129: 25.2; Typo in lisp/gnus/nnimap.el

[Sean Connor]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Thanks for this report! Can you tell us which IMAP servers you've
> tested this on? I just tried Dovecot, and the "(cadr result)" fix works
> properly there. Unless we know there are some servers where "(caddr
> result)" is appropriate (I wonder what server Nikolaus was using), I'm
> inclined to put the simpler fix in.

I was a bit mistaken.  My initial tests were on an old server that
didn't support MOVE, so I overlooked something important, the reason for
the change: the COPYUID is given as an "untagged response" for MOVE but
a "tagged response" for COPY [0].  IOW, caddr is what to use for getting
the COPYUID from a response to a MOVE command.

The COPYUID response is given by both the COPY and MOVE commands.  I'd
only been testing the COPY command, oops.

The cautious patch seems to handle both cases, according to this test
code:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test code --]
[-- Type: text/lisp, Size: 524 bytes --]

(mapcar (lambda (imap-response)
    (with-temp-buffer
      (insert imap-response)
      (or
       (nnimap-find-uid-response
	"COPYUID" (cadr
		   ;; simulate a call to nnimap-command.
		   (cons t (nnimap-parse-response))))
       (nnimap-find-uid-response
	"COPYUID" (caddr (cons t (nnimap-parse-response)))))))
 '(
   ;; MOVE result
   "* OK [COPYUID 1598849953 2 3] Moved UIDs.\r
* 1 EXPUNGE\r
1 OK Move completed (0.015 + 0.000 + 0.014 secs).\r"
   ;; COPY result
   "6 OK [COPYUID 1395967160 10 3] Copy completed."))

[-- Attachment #3: Type: text/plain, Size: 407 bytes --]


Sorry for the confusion.  This problem is only going to affect those
whose IMAP servers don't support the MOVE extension, which is probably
why it was overlooked.

I tested with courier, dovecot, RFC sample sessions and gmail IMAP
transcripts, FWIW.

sean
[0]
tagged in COPY response
https://tools.ietf.org/html/rfc2359#section-4.3
untagged in MOVE response
https://tools.ietf.org/html/rfc6851#section-4.3

bug#43129: 25.2; Typo in lisp/gnus/nnimap.el

[Eric Abrahamsen]

Sean Connor <sconnor@allyinics.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Thanks for this report! Can you tell us which IMAP servers you've
>> tested this on? I just tried Dovecot, and the "(cadr result)" fix works
>> properly there. Unless we know there are some servers where "(caddr
>> result)" is appropriate (I wonder what server Nikolaus was using), I'm
>> inclined to put the simpler fix in.
>
> I was a bit mistaken.  My initial tests were on an old server that
> didn't support MOVE, so I overlooked something important, the reason for
> the change: the COPYUID is given as an "untagged response" for MOVE but
> a "tagged response" for COPY [0].  IOW, caddr is what to use for getting
> the COPYUID from a response to a MOVE command.
>
> The COPYUID response is given by both the COPY and MOVE commands.  I'd
> only been testing the COPY command, oops.

And my dovecot test was done in a clean environment where, by default,
Gnus doesn't let dovecot use MOVE, so in effect I was only testing COPY
as well. Looks like cautious is the way to go!

> The cautious patch seems to handle both cases, according to this test
> code:
>
> (mapcar (lambda (imap-response)
>     (with-temp-buffer
>       (insert imap-response)
>       (or
>        (nnimap-find-uid-response
> 	"COPYUID" (cadr
> 		   ;; simulate a call to nnimap-command.
> 		   (cons t (nnimap-parse-response))))
>        (nnimap-find-uid-response
> 	"COPYUID" (caddr (cons t (nnimap-parse-response)))))))
>  '(
>    ;; MOVE result
>    "* OK [COPYUID 1598849953 2 3] Moved UIDs.\r
> * 1 EXPUNGE\r
> 1 OK Move completed (0.015 + 0.000 + 0.014 secs).\r"
>    ;; COPY result
>    "6 OK [COPYUID 1395967160 10 3] Copy completed."))

Thanks, this is helpful. I feel like the "outer edges" of execution is
the wrong place to be checking this stuff -- if the response parsing
process handled the differences, this would never have been an issue in
the first place. And ugh, this can-move thing is all over the place, and
begging to be refactored...

But! We will not be drawn down that rabbit hole. It seems to me that
changing the code to read:

(cons internal-move-group
      (or (nnimap-find-uid-response "COPYUID"
				    (if can-move
					(caddr result)
				      (cadr result)))
          (nnimap-find-article-by-message-id
           internal-move-group server message-id
           nnimap-request-articles-find-limit)))

Should do the trick here. What do you think?

> Sorry for the confusion. This problem is only going to affect those
> whose IMAP servers don't support the MOVE extension, which is probably
> why it was overlooked.
>
> I tested with courier, dovecot, RFC sample sessions and gmail IMAP
> transcripts, FWIW.

That's good to know! That gives us some confidence.

bug#43129: 25.2; Typo in lisp/gnus/nnimap.el

[Eric Abrahamsen]

On 09/02/20 01:38 AM, Sean Connor wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> And my dovecot test was done in a clean environment where, by default,
>> Gnus doesn't let dovecot use MOVE, so in effect I was only testing
>> COPY as well. Looks like cautious is the way to go!
>
> Yes.
>
> [...]
>
>> Thanks, this is helpful. I feel like the "outer edges" of execution is
>> the wrong place to be checking this stuff -- if the response parsing
>> process handled the differences, this would never have been an issue in
>> the first place. And ugh, this can-move thing is all over the place, and
>> begging to be refactored...
>
> I don't know.
>
>> But! We will not be drawn down that rabbit hole. It seems to me that
>> changing the code to read:
>>
>
> [code]
>
> Nice.  That's much clearer.
>
> [...]
>
>>> I tested with courier, dovecot, RFC sample sessions and gmail IMAP
>>> transcripts, FWIW.
>>
>> That's good to know! That gives us some confidence.
>
> Yep.
>
> Thank you.

In it goes! Thanks for the report.

Eric