Proposed changes to gnus-dup.el

Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-lisp-gnus-gnus-dup.el-Use-lexical-binding.patch --]
[-- Type: text/x-diff, Size: 3911 bytes --]

From b360958263800408081fc5b7a92fb219c46a43a5 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 25 Mar 2019 02:22:58 +0000
Subject: [PATCH] * lisp/gnus/gnus-dup.el: Use lexical-binding

(gnus-dup-list-dirty): Add docstring.
(gnus-dup-open): Allocate gnus-dup-hashtb more conservatively now
that it is no longer an obarray.
(gnus-dup-enter-articles): Fix off-by-one error and De Morgan.
(gnus-dup-suppress-articles): DRY.
---
 lisp/gnus/gnus-dup.el | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/lisp/gnus/gnus-dup.el b/lisp/gnus/gnus-dup.el
index 8b876489e1..e2de5d0c8e 100644
--- a/lisp/gnus/gnus-dup.el
+++ b/lisp/gnus/gnus-dup.el
@@ -1,4 +1,4 @@
-;;; gnus-dup.el --- suppression of duplicate articles in Gnus
+;;; gnus-dup.el --- suppression of duplicate articles in Gnus  -*- lexical-binding: t -*-
 
 ;; Copyright (C) 1996-2019 Free Software Foundation, Inc.
 
@@ -57,10 +57,12 @@ gnus-duplicate-file
 
 (defvar gnus-dup-list nil
   "List of seen message IDs, as strings.")
+
 (defvar gnus-dup-hashtb nil
   "Hash table of seen message IDs, for fast lookup.")
 
-(defvar gnus-dup-list-dirty nil)
+(defvar gnus-dup-list-dirty nil
+  "Non-nil if `gnus-dup-list' needs to be saved.")
 
 ;;;
 ;;; Starting and stopping
@@ -80,7 +82,7 @@ gnus-dup-open
   (if gnus-save-duplicate-list
       (gnus-dup-read)
     (setq gnus-dup-list nil))
-  (setq gnus-dup-hashtb (gnus-make-hashtable gnus-duplicate-list-length))
+  (setq gnus-dup-hashtb (gnus-make-hashtable))
   ;; Enter all Message-IDs into the hash table.
   (dolist (g gnus-dup-list)
     (puthash g t gnus-dup-hashtb)))
@@ -112,20 +114,22 @@ gnus-dup-enter-articles
     ;; Enter the Message-IDs of all read articles into the list
     ;; and hash table.
     (dolist (datum gnus-newsgroup-data)
-      (when (and (not (gnus-data-pseudo-p datum))
-		 (> (gnus-data-number datum) 0)
-		 (not (memq (gnus-data-number datum) gnus-newsgroup-unreads))
-		 (not (= (gnus-data-mark datum) gnus-canceled-mark))
-		 (setq msgid (mail-header-id (gnus-data-header datum)))
-		 (not (nnheader-fake-message-id-p msgid))
-		 (not (gethash msgid gnus-dup-hashtb)))
+      (unless (or (gnus-data-pseudo-p datum)
+                  (<= (gnus-data-number datum) 0)
+                  (memq (gnus-data-number datum) gnus-newsgroup-unreads)
+                  (= (gnus-data-mark datum) gnus-canceled-mark)
+                  (not (setq msgid (mail-header-id (gnus-data-header datum))))
+                  (nnheader-fake-message-id-p msgid)
+                  (gethash msgid gnus-dup-hashtb))
 	(push msgid gnus-dup-list)
 	(puthash msgid t gnus-dup-hashtb))))
-  ;; Chop off excess Message-IDs from the list.
-  (let ((end (nthcdr gnus-duplicate-list-length gnus-dup-list)))
+  ;; Remove excess Message-IDs from the list and hash table.
+  (let* ((dups (cons nil gnus-dup-list))
+         (end  (nthcdr gnus-duplicate-list-length dups)))
     (when end
       (mapc (lambda (id) (remhash id gnus-dup-hashtb)) (cdr end))
-      (setcdr end nil))))
+      (setcdr end nil))
+    (setq gnus-dup-list (cdr dups))))
 
 (defun gnus-dup-suppress-articles ()
   "Mark duplicate articles as read."
@@ -137,10 +141,9 @@ gnus-dup-suppress-articles
 	number)
     (dolist (header gnus-newsgroup-headers)
       (when (and (gethash (mail-header-id header) gnus-dup-hashtb)
-		 (gnus-summary-article-unread-p (mail-header-number header)))
-	(setq gnus-newsgroup-unreads
-	      (delq (setq number (mail-header-number header))
-		    gnus-newsgroup-unreads))
+                 (setq number (mail-header-number header))
+                 (gnus-summary-article-unread-p number))
+        (setq gnus-newsgroup-unreads (delq number gnus-newsgroup-unreads))
 	(if (not auto)
 	    (push (cons number gnus-duplicate-mark) gnus-newsgroup-reads)
 	  (push number gnus-newsgroup-expirable)
-- 
2.20.1


[-- Attachment #2: Type: text/plain, Size: 182 bytes --]


While looking into bug#34973, bug#34974, and bug#34987, I accumulated
the attached changes to lisp/gnus/gnus-dup.el which I would like to push
to master.  WDYT?

Thanks,

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Andy Moreton]

On Mon 25 Mar 2019, Basil L. Contovounesios wrote:

> While looking into bug#34973, bug#34974, and bug#34987, I accumulated
> the attached changes to lisp/gnus/gnus-dup.el which I would like to push
> to master.  WDYT?

Given the games Gnus plays with bindings (see `defvoo' and `deffoo') the
switch to lexical binding for gnus may prove to be complex.

It would be better to avoid switching to lexical binding for a single
file in gnus: rather, do that as part of a larger effort to switch all
of gnus over to lexical binding.

    AndyM

Re: Proposed changes to gnus-dup.el

[Stefan Monnier]

>> While looking into bug#34973, bug#34974, and bug#34987, I accumulated
>> the attached changes to lisp/gnus/gnus-dup.el which I would like to push
>> to master.  WDYT?
>
> Given the games Gnus plays with bindings (see `defvoo' and `deffoo') the
> switch to lexical binding for gnus may prove to be complex.
>
> It would be better to avoid switching to lexical binding for a single
> file in gnus: rather, do that as part of a larger effort to switch all
> of gnus over to lexical binding.

FWIW, I've been compiling Gnus using lexical-binding for many years now,
and it's the MUA I use to spam this mailing-list, so at least the main
"read&write messages" works fine for the nnimap and nntp backends.

I think the def[fv]oo hacks don't get in the way of
lexical-binding at all.


        Stefan

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Mon 25 Mar 2019, Basil L. Contovounesios wrote:
>
>> While looking into bug#34973, bug#34974, and bug#34987, I accumulated
>> the attached changes to lisp/gnus/gnus-dup.el which I would like to push
>> to master.  WDYT?
>
> Given the games Gnus plays with bindings (see `defvoo' and `deffoo') the
> switch to lexical binding for gnus may prove to be complex.

There are no such binding constructs in gnus-dup.el, and switching to
lexical-binding in this file was simple, as the patch demonstrates.

> It would be better to avoid switching to lexical binding for a single
> file in gnus: rather, do that as part of a larger effort to switch all
> of gnus over to lexical binding.

Why would that be better?

Enabling lexical-binding in a small, simple file carries little risk of
fallout while providing the (admittedly tiny) benefit of bringing us one
increment closer to lexical Gnus.

Sweeping changes across the whole Gnus system, OTOH, are far more risky,
as the recent switch to hash-tables has taught us.

I would welcome such a wholesale switch to lexical-binding, but I see no
reason or benefit to hold back on one file in the meantime.

Thanks,

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> While looking into bug#34973, bug#34974, and bug#34987, I accumulated
>>> the attached changes to lisp/gnus/gnus-dup.el which I would like to push
>>> to master.  WDYT?
>>
>> Given the games Gnus plays with bindings (see `defvoo' and `deffoo') the
>> switch to lexical binding for gnus may prove to be complex.
>>
>> It would be better to avoid switching to lexical binding for a single
>> file in gnus: rather, do that as part of a larger effort to switch all
>> of gnus over to lexical binding.
>
> FWIW, I've been compiling Gnus using lexical-binding for many years now,
> and it's the MUA I use to spam this mailing-list, so at least the main
> "read&write messages" works fine for the nnimap and nntp backends.
>
> I think the def[fv]oo hacks don't get in the way of
> lexical-binding at all.

Thank you for sharing your experience; it's reassuring.

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> +      (unless (or (gnus-data-pseudo-p datum)
> +                  (<= (gnus-data-number datum) 0)

I'm against using `unless' on complex logic -- many people get confused
when reading it in my experience.

The rest looks OK.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Proposed changes to gnus-dup.el

[Tadeus Prastowo]

On Mon, Mar 25, 2019 at 6:34 PM Basil L. Contovounesios <contovob@tcd.ie> wrote:
> Sweeping changes across the whole Gnus system, OTOH, are far more risky,
> as the recent switch to hash-tables has taught us.

Would you mind giving me the URL(s) to the thread(s) that contain(s)
the lesson, please?

Thank you very much.

--
Best regards,
Tadeus

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> +      (unless (or (gnus-data-pseudo-p datum)
>> +                  (<= (gnus-data-number datum) 0)
>
> I'm against using `unless' on complex logic -- many people get confused
> when reading it in my experience.

Fair enough.

> The rest looks OK.

Thanks.  I will wait a few more days for people to comment and push the
rest of the patch if there are no serious objections.

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Eric Abrahamsen]

Tadeus Prastowo <tadeus.prastowo@unitn.it> writes:

> On Mon, Mar 25, 2019 at 6:34 PM Basil L. Contovounesios <contovob@tcd.ie> wrote:
>> Sweeping changes across the whole Gnus system, OTOH, are far more risky,
>> as the recent switch to hash-tables has taught us.
>
> Would you mind giving me the URL(s) to the thread(s) that contain(s)
> the lesson, please?

See:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34973
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34987
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34974
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34983
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34981
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33653


:(

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Tadeus Prastowo <tadeus.prastowo@unitn.it> writes:

> On Mon, Mar 25, 2019 at 6:34 PM Basil L. Contovounesios <contovob@tcd.ie> wrote:
>> Sweeping changes across the whole Gnus system, OTOH, are far more risky,
>> as the recent switch to hash-tables has taught us.
>
> Would you mind giving me the URL(s) to the thread(s) that contain(s)
> the lesson, please?

See bugs 33653, 34973, 34974, 34981, 34983, and 34987.

https://debbugs.gnu.org/33653
https://debbugs.gnu.org/34973
https://debbugs.gnu.org/34974
https://debbugs.gnu.org/34981
https://debbugs.gnu.org/34983
https://debbugs.gnu.org/34987

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Eric Abrahamsen]

On 03/25/19 18:13 PM, Basil L. Contovounesios wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>>
>>> +      (unless (or (gnus-data-pseudo-p datum)
>>> +                  (<= (gnus-data-number datum) 0)
>>
>> I'm against using `unless' on complex logic -- many people get confused
>> when reading it in my experience.
>
> Fair enough.
>
>> The rest looks OK.
>
> Thanks.  I will wait a few more days for people to comment and push the
> rest of the patch if there are no serious objections.

If I end up backing out the hash-table changes, will this patch still
apply correctly?

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 03/25/19 18:13 PM, Basil L. Contovounesios wrote:
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>
>>> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>>>
>>>> +      (unless (or (gnus-data-pseudo-p datum)
>>>> +                  (<= (gnus-data-number datum) 0)
>>>
>>> I'm against using `unless' on complex logic -- many people get confused
>>> when reading it in my experience.
>>
>> Fair enough.
>>
>>> The rest looks OK.
>>
>> Thanks.  I will wait a few more days for people to comment and push the
>> rest of the patch if there are no serious objections.
>
> If I end up backing out the hash-table changes, will this patch still
> apply correctly?

The original patch I sent will conflict with the hunks where you added
docstrings to gnus-dup-{list,hashtb} and switched from obarray to
hash-table operations.

It would be easy for me to resolve these minor conflicts, but if that's
going to get in your way of more pressing work, then I'll hold off.
(That's not to say I think you should revert the hash-table changes.)

Thanks,

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Andy Moreton]

On Mon 25 Mar 2019, Basil L. Contovounesios wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> While looking into bug#34973, bug#34974, and bug#34987, I accumulated
>>>> the attached changes to lisp/gnus/gnus-dup.el which I would like to push
>>>> to master.  WDYT?
>>>
>>> Given the games Gnus plays with bindings (see `defvoo' and `deffoo') the
>>> switch to lexical binding for gnus may prove to be complex.
>>>
>>> It would be better to avoid switching to lexical binding for a single
>>> file in gnus: rather, do that as part of a larger effort to switch all
>>> of gnus over to lexical binding.
>>
>> FWIW, I've been compiling Gnus using lexical-binding for many years now,
>> and it's the MUA I use to spam this mailing-list, so at least the main
>> "read&write messages" works fine for the nnimap and nntp backends.
>>
>> I think the def[fv]oo hacks don't get in the way of
>> lexical-binding at all.
>
> Thank you for sharing your experience; it's reassuring.

I am glad to hear that lexical binding is not problematic after all.
Perhaps you can coordinate with Stephan to update all of Gnus to use
lexical binding ?

Thanks for working on Gnus,

    AndyM

Re: Proposed changes to gnus-dup.el

[Stefan Monnier]

> I am glad to hear that lexical binding is not problematic after all.
> Perhaps you can coordinate with Stephan to update all of Gnus to use
> lexical binding ?

Oh, I do have to apply some patches, but nothing too serious and nothing
anywhere near the def[vf]oo business.


        Stefan "who compiles *all* his files with lexical-binding"

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> I am glad to hear that lexical binding is not problematic after all.
> Perhaps you can coordinate with Stephan to update all of Gnus to use
> lexical binding ?

I don't intend to make such a systematic effort any time soon, but I'll
be sure to post any changes I accumulate for review.

Thanks,

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> On 03/25/19 18:13 PM, Basil L. Contovounesios wrote:
>>>
>>> I will wait a few more days for people to comment and push the
>>> rest of the patch if there are no serious objections.
>>
>> If I end up backing out the hash-table changes, will this patch still
>> apply correctly?
>
> The original patch I sent will conflict with the hunks where you added
> docstrings to gnus-dup-{list,hashtb} and switched from obarray to
> hash-table operations.
>
> It would be easy for me to resolve these minor conflicts, but if that's
> going to get in your way of more pressing work, then I'll hold off.
> (That's not to say I think you should revert the hash-table changes.)

Eric, just to be clear: may I push these minor changes, or would you
rather I held off?

Thanks,

-- 
Basil

Re: Proposed changes to gnus-dup.el

[Eric Abrahamsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> On 03/25/19 18:13 PM, Basil L. Contovounesios wrote:
>>>>
>>>> I will wait a few more days for people to comment and push the
>>>> rest of the patch if there are no serious objections.
>>>
>>> If I end up backing out the hash-table changes, will this patch still
>>> apply correctly?
>>
>> The original patch I sent will conflict with the hunks where you added
>> docstrings to gnus-dup-{list,hashtb} and switched from obarray to
>> hash-table operations.
>>
>> It would be easy for me to resolve these minor conflicts, but if that's
>> going to get in your way of more pressing work, then I'll hold off.
>> (That's not to say I think you should revert the hash-table changes.)
>
> Eric, just to be clear: may I push these minor changes, or would you
> rather I held off?

Oh, sorry -- I'd assumed you'd already pushed them! They're good to go
as far as I'm concerned: in for a penny, in for a pound.

Thanks,
Eric

Re: Proposed changes to gnus-dup.el

[Basil L. Contovounesios]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>>
>>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>>
>>>> On 03/25/19 18:13 PM, Basil L. Contovounesios wrote:
>>>>>
>>>>> I will wait a few more days for people to comment and push the
>>>>> rest of the patch if there are no serious objections.
>>>>
>>>> If I end up backing out the hash-table changes, will this patch still
>>>> apply correctly?
>>>
>>> The original patch I sent will conflict with the hunks where you added
>>> docstrings to gnus-dup-{list,hashtb} and switched from obarray to
>>> hash-table operations.
>>>
>>> It would be easy for me to resolve these minor conflicts, but if that's
>>> going to get in your way of more pressing work, then I'll hold off.
>>> (That's not to say I think you should revert the hash-table changes.)
>>
>> Eric, just to be clear: may I push these minor changes, or would you
>> rather I held off?
>
> Oh, sorry -- I'd assumed you'd already pushed them! They're good to go
> as far as I'm concerned: in for a penny, in for a pound.

Thanks, pushed.

[1: 2bd3e48404]: * lisp/gnus/gnus-dup.el: Use lexical-binding
  2019-04-03 02:55:41 +0100
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=2bd3e484041b2b7ea47c236b86f59610d971b609

-- 
Basil