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

[Sean Connor]

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:

[Eric Abrahamsen]

Post by Sean Connor
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.
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
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.

[Sean Connor]

Post by Eric Abrahamsen
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:

[Eric Abrahamsen]

Post by Sean Connor

Post by Eric Abrahamsen
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!

Post by Sean Connor
The cautious patch seems to handle both cases, according to this test
(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?

Post by Sean Connor
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.

[Eric Abrahamsen]

Post by Eric Abrahamsen
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.
[...]

Post by Eric Abrahamsen
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.

Post by Eric Abrahamsen
But! We will not be drawn down that rabbit hole. It seems to me that

[code]
Nice. That's much clearer.
[...]

Post by Eric Abrahamsen

Post by Sean Connor
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

[Eric Abrahamsen]

Post by Eric Abrahamsen
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.
[...]

Post by Eric Abrahamsen
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.

Post by Eric Abrahamsen
But! We will not be drawn down that rabbit hole. It seems to me that

[code]
Nice. That's much clearer.
[...]

Post by Eric Abrahamsen

Post by Sean Connor
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