Fwd: Patch for a bug in imap-open

Fwd: Patch for a bug in imap-open

[Ramkumar R]

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

Hi!

I had sent a patch to Lars (plz. see details in the conv. below). He
has asked me to send it to this group for its perusal...so here's the
patch...

Ramkumar.

---------- Forwarded message ----------
From: Lars Magne Ingebrigtsen <larsi@gnus.org>
Date: Aug 25, 2005 10:16 PM
Subject: Re: Patch for a bug in imap-open
To: Ramkumar R <andyetitmoves@gmail.com>


Ramkumar R <andyetitmoves@gmail.com> writes:

> I encountered an error when opening my inbox over imap with starttls.
> There is an error thrown up which gets swallowed by a condition-case
> somewhere higher up and I get an error message and a yes-or-no-p
> asking me if I want to continue. Traced the problem to imap-open and I
> have enclosed a patch. I program in lisp, but have no idea about the
> gnus source. So, it is likely that the solution is wrong or hackish.
> Works for me though :)

:-)

The patch looks OK to me, but I'm not all that familiar with the imap
side of things.  Could you send the patch to the ding@gnus.org
mailing list and see whether anybody has any comments?

--
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen


-- 
The day Microsoft makes something that doesn't suck
is probably the day they start making vacuum cleaners.
                                               -Ernst Jan Plugge

[-- Attachment #2: gnus-patch --]
[-- Type: application/octet-stream, Size: 766 bytes --]

Index: lisp/imap.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/imap.el,v
retrieving revision 7.21
diff -r7.21 imap.el
1094,1095c1094,1097
< 		      (kill-buffer buffer)
< 		      (rename-buffer buffer)
---
> 		      (let ((bname (buffer-name buffer)))
> 			(kill-buffer buffer)
> 			(setq buffer (current-buffer))
> 			(rename-buffer bname))
Index: lisp/mail-source.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/mail-source.el,v
retrieving revision 7.11
diff -r7.11 mail-source.el
993c993
<       (if (and (imap-open server port stream authentication buf)
---
>       (if (and (setq buf (imap-open server port stream authentication buf))

Re: Fwd: Patch for a bug in imap-open

[Simon Josefsson]

Ramkumar R <andyetitmoves@gmail.com> writes:

> Hi!
>
> I had sent a patch to Lars (plz. see details in the conv. below). He
> has asked me to send it to this group for its perusal...so here's the
> patch...
...
>> I encountered an error when opening my inbox over imap with starttls.
>> There is an error thrown up which gets swallowed by a condition-case
>> somewhere higher up and I get an error message and a yes-or-no-p
>> asking me if I want to continue. Traced the problem to imap-open and I
>> have enclosed a patch. I program in lisp, but have no idea about the
>> gnus source. So, it is likely that the solution is wrong or hackish.
>> Works for me though :)
...
> Index: lisp/imap.el
> ===================================================================
> RCS file: /usr/local/cvsroot/gnus/lisp/imap.el,v
> retrieving revision 7.21
> diff -r7.21 imap.el
> 1094,1095c1094,1097
> < 		      (kill-buffer buffer)
> < 		      (rename-buffer buffer)
> ---
>> 		      (let ((bname (buffer-name buffer)))
>> 			(kill-buffer buffer)
>> 			(setq buffer (current-buffer))
>> 			(rename-buffer bname))

There is something wrong with this patch; imap-open should make sure
the returned process is inside the buffer provided in the BUFFER
argument.  Your patch changes this interface, causing all callers of
imap-open to be updated as follows:

> Index: lisp/mail-source.el
> ===================================================================
> RCS file: /usr/local/cvsroot/gnus/lisp/mail-source.el,v
> retrieving revision 7.11
> diff -r7.11 mail-source.el
> 993c993
> <       (if (and (imap-open server port stream authentication buf)
> ---
>>       (if (and (setq buf (imap-open server port stream authentication buf))

Doing that is complicated inside nnimap.el, and I'd rather not change
the API in this way if possible.

Exactly what problem did you see?  Use (setq debug-on-signal t) to
find errors that are swallowed by condition-case.

Re: Fwd: Patch for a bug in imap-open

[Ramkumar R]

> There is something wrong with this patch; imap-open should make sure
> the returned process is inside the buffer provided in the BUFFER
> argument.  Your patch changes this interface, causing all callers of
> imap-open to be updated as follows:

yeah....i realised that. from what i could gather from the function,
the function tries to create a new buffer when tls is supported and
fills in the contents. If we have to use the same buffer, we would
need to reinitialize the buffer to a clean state, and use it instead
of a new buffer. as i said, i am not familiar with the gnus
source...so i didn't know if copying buffer contents after possibly a
kill-all-local-variables would be safe...(the older buffer also has a
process) or is it required that we selectively tweak some
variables....this seemed the easiest way out...if u can figure that
out, then perhaps we can manage with the same buffer..

> Exactly what problem did you see?  Use (setq debug-on-signal t) to
> find errors that are swallowed by condition-case.

well...debug-on-signal springs up a lot of other spurious errors in
other packages in my setup. For that matter, there are many cases
where condition-case's are genuine, unlike in the case i described,
where it was an error which was covered up for the user's convenience.

Regards,
Ramkumar.

-- 
The day Microsoft makes something that doesn't suck
is probably the day they start making vacuum cleaners.
                                               -Ernst Jan Plugge

Re: Fwd: Patch for a bug in imap-open

[Simon Josefsson]

Ramkumar R <andyetitmoves@gmail.com> writes:

>> There is something wrong with this patch; imap-open should make sure
>> the returned process is inside the buffer provided in the BUFFER
>> argument.  Your patch changes this interface, causing all callers of
>> imap-open to be updated as follows:
>
> yeah....i realised that. from what i could gather from the function,
> the function tries to create a new buffer when tls is supported and
> fills in the contents. If we have to use the same buffer, we would
> need to reinitialize the buffer to a clean state, and use it instead
> of a new buffer. as i said, i am not familiar with the gnus
> source...so i didn't know if copying buffer contents after possibly a
> kill-all-local-variables would be safe...(the older buffer also has a
> process) or is it required that we selectively tweak some
> variables....this seemed the easiest way out...if u can figure that
> out, then perhaps we can manage with the same buffer..

But doing that is exactly what the current code attempts to do.  It
create a temporary buffer for the TLS/GSSAPI/whatever, and if that
connection succeed, it close the first connection (in BUFFER), kill
that buffer and rename the new temporary buffer to the old BUFFER:

		    (message "imap: Reconnecting with stream `%s'..." stream)
		    (if (null (let ((imap-stream stream))
				(imap-open-1 (current-buffer))))
			(progn
			  (kill-buffer (current-buffer))
			  (message
			   "imap: Reconnecting with stream `%s'...failed"
			   stream))
		      ;; We're done, kill the first connection
		      (imap-close buffer)
		      (kill-buffer buffer)
		      (rename-buffer buffer)
		      (message "imap: Reconnecting with stream `%s'...done"
			       stream)

>> Exactly what problem did you see?  Use (setq debug-on-signal t) to
>> find errors that are swallowed by condition-case.
>
> well...debug-on-signal springs up a lot of other spurious errors in
> other packages in my setup. For that matter, there are many cases
> where condition-case's are genuine, unlike in the case i described,
> where it was an error which was covered up for the user's convenience.

You can press `c' to continue passed spurious errors.

I think we should go back to what your original problem was instead of
looking at possible solutions.  I haven't understood what problem you
want to solve.

Re: Fwd: Patch for a bug in imap-open

[Ramkumar R]

> I think we should go back to what your original problem was instead of
> looking at possible solutions.  I haven't understood what problem you
> want to solve.

This is my mail-sources:

      '((file)
	(imap :server "10.6.5.215" :user "ramk"
	      :mailbox "mbox" :fetchflag "\\Seen")))

I get the following error:

Mail source (imap :server 10.6.5.215 :user ramk :mailbox mbox
:fetchflag \Seen) error (stringp).  Continue? (yes or no)

I trace the error to rename-buffer which should be taking a buffer
name rather than a buffer. And that we are doing a kill buffer just
before that. So, I modified imap.el as it is in the patch.

Now, I get this error:

Mail source (imap :server 10.6.5.215 :user ramk :mailbox mbox
:fetchflag \Seen) error (Selecting deleted buffer).  Continue? (yes or
no)

So, I realise that the caller has been trying to use `buf' with which
imap-open was called, and `buf' was actually killed. What we have now
is a different buffer with the same buffer name. So, I updated buf in
the caller.

Hope this clarifies.

Ramkumar.

-- 
We should have a great many fewer disputes in the world if words were taken
for what they are, the signs of our ideas, and not for things themselves.
                                     -John Locke, philosopher (1632-1704)