gnus-agent-expire-group-1 bug?

gnus-agent-expire-group-1 bug?

[Katsumi Yamaoka]

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

Hi,

The following section (prefixed with `-') in the function
`gnus-agent-expire-group-1' seems to do nothing, though I'm not
sure what it is for.  There's no `catch' for `sort-results', and
the `when' always gets nil.

(I found this while trying replacing `mapcar' with `mapc' or
`dolist'.
cf. http://article.gmane.org/gmane.emacs.gnus.general/65429)


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

--- gnus-agent.el~	2007-10-04 21:53:35 +0000
+++ gnus-agent.el	2007-10-22 09:55:41 +0000
@@ -3329,27 +3329,6 @@
 
 		   (setcdr dlist (cddr dlist)))
 	       (setq dlist (cdr dlist)))))
-
-	 ;; Check the order of the entry positions.  They should be in
-	 ;; ascending order.  If they aren't, the positions must be
-	 ;; converted to markers.
-	 (when (let ((dlist dlist)
-		     (prev-pos -1)
-		     pos)
-		 (while dlist
-		   (if (setq pos (nth 3 (pop dlist)))
-		       (if (< pos prev-pos)
-			   (throw 'sort-results 'unsorted)
-			 (setq prev-pos pos)))))
-	   (gnus-message 7 "gnus-agent-expire: Unsorted overview; inserting markers to compensate.")
-	   (mapcar (lambda (entry)
-		     (let ((pos (nth 3 entry)))
-		       (if pos
-			   (setf (nth 3 entry)
-				 (set-marker (make-marker)
-					     pos)))))
-		   dlist))
-
 	 (gnus-message 7 "gnus-agent-expire: Merging entries... Done")
 
 	 (let* ((len (float (length dlist)))

Re: gnus-agent-expire-group-1 bug?

[Reiner Steib]

On Mon, Oct 22 2007, Katsumi Yamaoka wrote:

> Hi,
>
> The following section (prefixed with `-') in the function
> `gnus-agent-expire-group-1' seems to do nothing, though I'm not
> sure what it is for.  There's no `catch' for `sort-results', and
> the `when' always gets nil.
>
> (I found this while trying replacing `mapcar' with `mapc' or
> `dolist'.
> cf. http://article.gmane.org/gmane.emacs.gnus.general/65429)
>
> --- gnus-agent.el~	2007-10-04 21:53:35 +0000
> +++ gnus-agent.el	2007-10-22 09:55:41 +0000
> @@ -3329,27 +3329,6 @@
>
>  		   (setcdr dlist (cddr dlist)))
>  	       (setq dlist (cdr dlist)))))
> -
> -	 ;; Check the order of the entry positions.  They should be in
> -	 ;; ascending order.  If they aren't, the positions must be
> -	 ;; converted to markers.
> -	 (when (let ((dlist dlist)
> -		     (prev-pos -1)
> -		     pos)
> -		 (while dlist
> -		   (if (setq pos (nth 3 (pop dlist)))
> -		       (if (< pos prev-pos)
> -			   (throw 'sort-results 'unsorted)
> -			 (setq prev-pos pos)))))
> -	   (gnus-message 7 "gnus-agent-expire: Unsorted overview; inserting markers to compensate.")
> -	   (mapcar (lambda (entry)
> -		     (let ((pos (nth 3 entry)))
> -		       (if pos
> -			   (setf (nth 3 entry)
> -				 (set-marker (make-marker)
> -					     pos)))))
> -		   dlist))
> -
>  	 (gnus-message 7 "gnus-agent-expire: Merging entries... Done")
>
>  	 (let* ((len (float (length dlist)))

Kevin, could you please comment on this?  The relevant log entry is

,----
| 2005-03-06  Kevin Greiner  <kevin.greiner@compsol.cc>
| [...]
| 	* gnus-agent.el [...]
| 	(gnus-agent-expire-group-1): Avoid using markers when the overview
| 	is in ascending order; greatly improves performance.
`----

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: gnus-agent-expire-group-1 bug?

[Kevin Greiner]

Reiner Steib wrote:
> On Mon, Oct 22 2007, Katsumi Yamaoka wrote:
>
>   
>> Hi,
>>
>> The following section (prefixed with `-') in the function
>> `gnus-agent-expire-group-1' seems to do nothing, though I'm not
>> sure what it is for.  There's no `catch' for `sort-results', and
>> the `when' always gets nil.
>>
>> (I found this while trying replacing `mapcar' with `mapc' or
>> `dolist'.
>> cf. http://article.gmane.org/gmane.emacs.gnus.general/65429)
>>
>> --- gnus-agent.el~	2007-10-04 21:53:35 +0000
>> +++ gnus-agent.el	2007-10-22 09:55:41 +0000
>> @@ -3329,27 +3329,6 @@
>>
>>  		   (setcdr dlist (cddr dlist)))
>>  	       (setq dlist (cdr dlist)))))
>> -
>> -	 ;; Check the order of the entry positions.  They should be in
>> -	 ;; ascending order.  If they aren't, the positions must be
>> -	 ;; converted to markers.
>> -	 (when (let ((dlist dlist)
>> -		     (prev-pos -1)
>> -		     pos)
>> -		 (while dlist
>> -		   (if (setq pos (nth 3 (pop dlist)))
>> -		       (if (< pos prev-pos)
>> -			   (throw 'sort-results 'unsorted)
>> -			 (setq prev-pos pos)))))
>> -	   (gnus-message 7 "gnus-agent-expire: Unsorted overview; inserting markers to compensate.")
>> -	   (mapcar (lambda (entry)
>> -		     (let ((pos (nth 3 entry)))
>> -		       (if pos
>> -			   (setf (nth 3 entry)
>> -				 (set-marker (make-marker)
>> -					     pos)))))
>> -		   dlist))
>> -
>>  	 (gnus-message 7 "gnus-agent-expire: Merging entries... Done")
>>
>>  	 (let* ((len (float (length dlist)))
>>     
>
> Kevin, could you please comment on this?  The relevant log entry is
>
> ,----
> | 2005-03-06  Kevin Greiner  <kevin.greiner@compsol.cc>
> | [...]
> | 	* gnus-agent.el [...]
> | 	(gnus-agent-expire-group-1): Avoid using markers when the overview
> | 	is in ascending order; greatly improves performance.
> `----
>
> Bye, Reiner.
>   
I've added the missing catch form to the when function.  The code now 
looks like:

     (when (catch 'sort-results
         (let ((dlist dlist)
               (prev-pos -1)
               pos)
           (while dlist
             (if (setq pos (nth 3 (pop dlist)))
             (if (< pos prev-pos)
                 (throw 'sort-results 'unsorted)
               (setq prev-pos pos))))))
        ...)

For those interested, if any, there was a time when updates to the 
agent's overview file resulted in it having entries that were out of 
order.  The errors were proving difficult to isolate so the original 
gnus-agent-expire-group-1 function used markers to record the position 
of each entry.  Then, as the function edited the buffer, emacs kept the 
markers updated.  It was all very simple and yet horribly inefficient.  
By the time of the 2005-03-06 check-in, I was fairly confident that the 
overview file was always ordered.  That implied that 
gnus-agent-expire-group-1 no longer needed to use markers which greatly 
improved performance.  Still, I wasn't absolutely sure that overview 
would always be sorted so I added an integrity check and fall-back code 
to convert positions into markers when the file proved to be unsorted.  
Apparently, the integrity check has always been broken which implies 
that people either never use gnus-agent-expire-group or that the 
overview buffer is finally sorted.

Kevin