bug#44981: 28.0.50; Restore nnimap-split-download-body?

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Eric Abrahamsen]

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


When splitting IMAP messages in Gnus' nnimap, there is a defvar called
`nnimap-split-download-body-default', which is consulted to decide
whether to download the whole message body during splitting, or only
consider its headers.

It appears that there used to be a defcustom called
`nnimap-split-download-body', which was advertised to the user, and this
defvar was used to hold... its default? It's odd.

In 2010, in commit 20a673b2d, a bunch of external Gnus code was merged
in, changing a lot of nnimap.el, and in the course of that the defcustom
was deleted. The defvar still remains, and is still effective. The
manual still refers to the customization option.

This looks unintentional, and I'd like to apply the attached diff,
restoring `nnimap-split-download-body' as a user option, and getting rid
of the default variable, which seems unnecessary. If this seems okay
I'll do up a proper commit, and make sure the documentation is all
accurate.

The same user who reported this also expressed a desire to be able to
download message bodies conditionally, based on the headers, only if
a split rule required it. I'm not at all sure about the feasibility of
that, but wanted to see if anyone else had an opinion.

Eric



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: nnimap-split-body.diff --]
[-- Type: text/x-patch, Size: 968 bytes --]

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index 7984998d21..1a1e13a23a 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -146,13 +146,16 @@ nnimap-request-articles-find-limit
   :version "24.4"
   :group 'nnimap)
 
+(defcustom nnimap-split-download-body nil
+  "If non-nil, splitting may also consider message bodies.
+This requires downloading the full message from the IMAP server
+during splitting, which may be slow."
+  :type 'boolean)
+
 (defvar nnimap-process nil)
 
 (defvar nnimap-status-string "")
 
-(defvar nnimap-split-download-body-default nil
-  "Internal variable with default value for `nnimap-split-download-body'.")
-
 (defvar nnimap-keepalive-timer nil)
 (defvar nnimap-process-buffers nil)
 
@@ -2100,7 +2103,7 @@ nnimap-fetch-inbox
 		 "BODY.PEEK"
 	       "RFC822.PEEK"))
 	    (cond
-	     (nnimap-split-download-body-default
+	     (nnimap-split-download-body
 	      "[]")
 	     ((nnimap-ver4-p)
 	      "[HEADER]")

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> This looks unintentional, and I'd like to apply the attached diff,
> restoring `nnimap-split-download-body' as a user option, and getting rid
> of the default variable, which seems unnecessary. If this seems okay
> I'll do up a proper commit, and make sure the documentation is all
> accurate.

I think it's unintentional, yes, so that sounds like a good plan.  You
can't get rid of the variable, though (there'll be external packages
relying on it), but you can make it obsolete.

> The same user who reported this also expressed a desire to be able to
> download message bodies conditionally, based on the headers, only if
> a split rule required it. I'm not at all sure about the feasibility of
> that, but wanted to see if anyone else had an opinion.

I don't see any way to do that without making splitting unbearably slow.

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

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> You can't get rid of the variable, though (there'll be external
> packages relying on it), but you can make it obsolete.

Of course, though in fairness its docstring starts with
"Internal variable..."

-- 
Basil

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Eric Abrahamsen]

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

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> You can't get rid of the variable, though (there'll be external
>> packages relying on it), but you can make it obsolete.
>
> Of course, though in fairness its docstring starts with
> "Internal variable..."

I also can't picture any way in which an external library would actually
make use of this. On the other hand, typing
`define-obsolete-variable-alias' is very little work.

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I also can't picture any way in which an external library would actually
> make use of this. On the other hand, typing
> `define-obsolete-variable-alias' is very little work.

It's mentioned in spam.el, at least, so it's probably out there.

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

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Eric Abrahamsen]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> This looks unintentional, and I'd like to apply the attached diff,
>> restoring `nnimap-split-download-body' as a user option, and getting rid
>> of the default variable, which seems unnecessary. If this seems okay
>> I'll do up a proper commit, and make sure the documentation is all
>> accurate.
>
> I think it's unintentional, yes, so that sounds like a good plan.  You
> can't get rid of the variable, though (there'll be external packages
> relying on it), but you can make it obsolete.
>
>> The same user who reported this also expressed a desire to be able to
>> download message bodies conditionally, based on the headers, only if
>> a split rule required it. I'm not at all sure about the feasibility of
>> that, but wanted to see if anyone else had an opinion.
>
> I don't see any way to do that without making splitting unbearably slow.

No, me neither.

Here's a proper commit. I moved the bulk of the documentation to the
section on IMAP client-side splitting. Originally it was only mentioned
in the docs for the Spam package, with a link to IMAP splitting, but
then not actually mentioned in the IMAP splitting section.

Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-nnimap-split-download-body-as-a-customizatio.patch --]
[-- Type: text/x-patch, Size: 3394 bytes --]

From 668f59da7b58e6c24b3bac7f6b5e02103d32363a Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Wed, 2 Dec 2020 15:52:25 -0800
Subject: [PATCH] Restore nnimap-split-download-body as a customization option

* lisp/gnus/nnimap.el (nnimap-split-download-body): And add an
obsolete alias for `nnimap-split-download-body-default'.
(nnimap-fetch-inbox): Check the value of the option, not the variable.
* doc/misc/gnus.texi (Client-Side IMAP Splitting): Move the primary
documentation of this option to this section.
---
 doc/misc/gnus.texi  | 16 +++++++++-------
 lisp/gnus/nnimap.el | 15 +++++++++++----
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index 81ce13945c..4aa07ce388 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -14521,6 +14521,14 @@ Client-Side IMAP Splitting
 
 @end table
 
+@vindex nnimap-split-download-body
+By default, the nnimap back end only retrieves the message headers;
+the option @code{nnimap-split-download-body} (which is a regular
+customization option, not a server variable) tells it to retrieve the
+message bodies as well.  We don't set this by default because it will
+slow @acronym{IMAP} down, and that is not an appropriate decision to
+make on behalf of the user.
+
 Here's a complete example @code{nnimap} backend with a client-side
 ``fancy'' splitting method:
 
@@ -24622,13 +24630,7 @@ Filtering Incoming Mail
 Note for IMAP users: if you use the @code{spam-check-bogofilter},
 @code{spam-check-ifile}, and @code{spam-check-stat} spam back ends,
 you should also set the variable @code{nnimap-split-download-body} to
-@code{t}.  These spam back ends are most useful when they can ``scan''
-the full message body.  By default, the nnimap back end only retrieves
-the message headers; @code{nnimap-split-download-body} tells it to
-retrieve the message bodies as well.  We don't set this by default
-because it will slow @acronym{IMAP} down, and that is not an
-appropriate decision to make on behalf of the user.  @xref{Client-Side
-IMAP Splitting}.
+@code{t}.  @xref{Client-Side IMAP Splitting}.
 
 You have to specify one or more spam back ends for @code{spam-split}
 to use, by setting the @code{spam-use-*} variables.  @xref{Spam Back
diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index 7984998d21..e7f9451986 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -146,13 +146,20 @@ nnimap-request-articles-find-limit
   :version "24.4"
   :group 'nnimap)
 
+(defcustom nnimap-split-download-body nil
+  "If non-nil, make message bodies available for consideration during splitting.
+This requires downloading the full message from the IMAP server
+during splitting, which may be slow."
+  :type 'boolean)
+
+(define-obsolete-variable-alias
+  'nnimap-split-download-body-default 'nnimap-split-download-body
+  "28.1")
+
 (defvar nnimap-process nil)
 
 (defvar nnimap-status-string "")
 
-(defvar nnimap-split-download-body-default nil
-  "Internal variable with default value for `nnimap-split-download-body'.")
-
 (defvar nnimap-keepalive-timer nil)
 (defvar nnimap-process-buffers nil)
 
@@ -2100,7 +2107,7 @@ nnimap-fetch-inbox
 		 "BODY.PEEK"
 	       "RFC822.PEEK"))
 	    (cond
-	     (nnimap-split-download-body-default
+	     (nnimap-split-download-body
 	      "[]")
 	     ((nnimap-ver4-p)
 	      "[HEADER]")
-- 
2.29.2

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Here's a proper commit.

Looks good to me.

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

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Here's a proper commit.
>
> Looks good to me.

Thanks, done.

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Basil L. Contovounesios]

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

Thanks for cleaning this up, Eric.  Could someone please look into the
following byte-compilation warning?

  In spam-setup-widening:
  gnus/spam.el:1234:11: Warning: ‘nnimap-split-download-body-default’ is an
      obsolete variable (as of 28.1); use ‘nnimap-split-download-body’ instead.

I was thinking of the following change:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: spam.diff --]
[-- Type: text/x-diff, Size: 895 bytes --]

diff --git a/lisp/gnus/spam.el b/lisp/gnus/spam.el
index 96a7da2313..8634fa680d 100644
--- a/lisp/gnus/spam.el
+++ b/lisp/gnus/spam.el
@@ -44,12 +44,9 @@
 ;;; for the definitions of group content classification and spam processors
 (require 'gnus)
 
-(eval-when-compile (require 'hashcash))
-
-;; for nnimap-split-download-body-default
-(eval-when-compile (require 'nnimap))
-
-(eval-when-compile (require 'cl-lib))
+(eval-when-compile
+  (require 'cl-lib)
+  (require 'hashcash))
 
 ;; autoload query-dig
 (autoload 'query-dig "dig")
@@ -1230,8 +1227,9 @@ spam-generic-score
 
 ;;; set up IMAP widening if it's necessary
 (defun spam-setup-widening ()
+  (defvar nnimap-split-download-body)
   (when (spam-widening-needed-p)
-    (setq nnimap-split-download-body-default t)))
+    (setq nnimap-split-download-body t)))
 
 (defun spam-widening-needed-p (&optional force-symbols)
   (let (found)

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


But Ted explicitly changed spam-setup-widening in 2003 to use
nnimap-split-download-body-default instead of
nnimap-split-download-body, to avoid modifying a user option.

I don't know what spam-setup-widening is meant to do, and by extension I
don't know what TRT to do here is.  Either way, loading nnimap in
eval-when-compile "for a variable" is definitely dubious.  Ideas?

Thanks,

-- 
Basil

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Eric Abrahamsen]

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

> Thanks for cleaning this up, Eric.  Could someone please look into the
> following byte-compilation warning?
>
>   In spam-setup-widening:
>   gnus/spam.el:1234:11: Warning: ‘nnimap-split-download-body-default’ is an
>       obsolete variable (as of 28.1); use ‘nnimap-split-download-body’ instead.
>
> I was thinking of the following change:
>
> diff --git a/lisp/gnus/spam.el b/lisp/gnus/spam.el
> index 96a7da2313..8634fa680d 100644
> --- a/lisp/gnus/spam.el
> +++ b/lisp/gnus/spam.el
> @@ -44,12 +44,9 @@
>  ;;; for the definitions of group content classification and spam processors
>  (require 'gnus)
>  
> -(eval-when-compile (require 'hashcash))
> -
> -;; for nnimap-split-download-body-default
> -(eval-when-compile (require 'nnimap))
> -
> -(eval-when-compile (require 'cl-lib))
> +(eval-when-compile
> +  (require 'cl-lib)
> +  (require 'hashcash))
>  
>  ;; autoload query-dig
>  (autoload 'query-dig "dig")
> @@ -1230,8 +1227,9 @@ spam-generic-score
>  
>  ;;; set up IMAP widening if it's necessary
>  (defun spam-setup-widening ()
> +  (defvar nnimap-split-download-body)
>    (when (spam-widening-needed-p)
> -    (setq nnimap-split-download-body-default t)))
> +    (setq nnimap-split-download-body t)))
>  
>  (defun spam-widening-needed-p (&optional force-symbols)
>    (let (found)
>
>
> But Ted explicitly changed spam-setup-widening in 2003 to use
> nnimap-split-download-body-default instead of
> nnimap-split-download-body, to avoid modifying a user option.
>
> I don't know what spam-setup-widening is meant to do, and by extension I
> don't know what TRT to do here is.  Either way, loading nnimap in
> eval-when-compile "for a variable" is definitely dubious.  Ideas?

I should have known it wouldn't be this simple!

It looks like widening is used if the spam backend in use does
statistical "learning", e.g. a Bayesian system that you can train on
spam/ham data sets. In that case the backend needs access to the full
message text in order to train properly. For local mail, all that's
needed is to "widen" the message buffer so that the body is available
for training, not just the headers. For IMAP the message body isn't
present, so "widening" takes on the additional meaning of "download the
message body and widen to it".

I'm not sure why spam.el would need to work on the default value of
`nnimap-split-download-body'. If spam.el determines that widening is
needed, it's going to download message bodies across the board anyway.
The only reason to preserve the user's own customization would be if the
user later unloaded spam.el -- then it should restore the previous
value. But how often would that actually be useful?

In principle, TRT would be to have spam.el let-bind a variable around
the splitting process. But I don't think the code is set up that way:
all the spam.el stuff happens "inside" the splitting process, not around
it.

Eric

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Basil L. Contovounesios]

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

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I'm not sure why spam.el would need to work on the default value of
> `nnimap-split-download-body'. If spam.el determines that widening is
> needed, it's going to download message bodies across the board anyway.
> The only reason to preserve the user's own customization would be if the
> user later unloaded spam.el -- then it should restore the previous
> value. But how often would that actually be useful?
>
> In principle, TRT would be to have spam.el let-bind a variable around
> the splitting process. But I don't think the code is set up that way:
> all the spam.el stuff happens "inside" the splitting process, not around
> it.

How about the attached kludgy but conservative dance?

-- 
Basil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Stop-using-deprecated-nnimap-variable-in-spam.el.patch --]
[-- Type: text/x-diff, Size: 3427 bytes --]

From 9792b97faf4e5b1dc22e58d99806ec17a9fd7985 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 5 Dec 2020 15:28:35 +0000
Subject: [PATCH] Stop using deprecated nnimap variable in spam.el

* lisp/gnus/nnimap.el (nnimap--split-download-body): New variable.
(nnimap-fetch-inbox): Use it in conjunction with
nnimap-split-download-body.
* lisp/gnus/spam.el: Don't load nnimap.el at compile time for a
dynamic variable.
(spam-setup-widening): Rather than unconditionally set deprecated
nnimap-split-download-body-default, set nnimap--split-download-body
to recognizable non-nil value only if it is nil (bug#44981).
(spam-teardown-widening): Undo this if nnimap--split-download-body
still holds the recognizable value.
(spam-unload-hook): Call spam-teardown-widening to revert any change
to the value of nnimap--split-download-body.
---
 lisp/gnus/nnimap.el |  6 +++++-
 lisp/gnus/spam.el   | 26 +++++++++++++++++---------
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index a860333066..c8b700ebcc 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -157,6 +157,9 @@ nnimap-split-download-body
   :version "28.1"
   :type 'boolean)
 
+(defvar nnimap--split-download-body nil
+  "Like `nnimap-split-download-body', but for internal use.")
+
 (defvar nnimap-process nil)
 
 (defvar nnimap-status-string "")
@@ -2108,7 +2111,8 @@ nnimap-fetch-inbox
 		 "BODY.PEEK"
 	       "RFC822.PEEK"))
 	    (cond
-	     (nnimap-split-download-body
+             ((or nnimap-split-download-body
+                  nnimap--split-download-body)
 	      "[]")
 	     ((nnimap-ver4-p)
 	      "[HEADER]")
diff --git a/lisp/gnus/spam.el b/lisp/gnus/spam.el
index 96a7da2313..e74aef3efe 100644
--- a/lisp/gnus/spam.el
+++ b/lisp/gnus/spam.el
@@ -44,12 +44,9 @@
 ;;; for the definitions of group content classification and spam processors
 (require 'gnus)
 
-(eval-when-compile (require 'hashcash))
-
-;; for nnimap-split-download-body-default
-(eval-when-compile (require 'nnimap))
-
-(eval-when-compile (require 'cl-lib))
+(eval-when-compile
+  (require 'cl-lib)
+  (require 'hashcash))
 
 ;; autoload query-dig
 (autoload 'query-dig "dig")
@@ -1228,10 +1225,20 @@ spam-generic-score
 
 ;;{{{ set up widening, processor checks
 
-;;; set up IMAP widening if it's necessary
+(defconst spam--widened (list ())
+  "Unique value identifying changes to `nnimap--split-download-body'.")
+
 (defun spam-setup-widening ()
-  (when (spam-widening-needed-p)
-    (setq nnimap-split-download-body-default t)))
+  "Set up IMAP widening if it's necessary."
+  (and (boundp 'nnimap--split-download-body)
+       (not nnimap--split-download-body)
+       (spam-widening-needed-p)
+       (setq nnimap--split-download-body spam--widened)))
+
+(defun spam-teardown-widening ()
+  "Tear down IMAP widening."
+  (when (eq (bound-and-true-p nnimap--split-download-body) spam--widened)
+    (setq nnimap--split-download-body nil)))
 
 (defun spam-widening-needed-p (&optional force-symbols)
   (let (found)
@@ -2865,6 +2872,7 @@ spam-initialize
 (defun spam-unload-hook ()
   "Uninstall the spam.el hooks."
   (interactive)
+  (spam-teardown-widening)
   (remove-hook 'gnus-save-newsrc-hook 'spam-maybe-spam-stat-save)
   (remove-hook 'gnus-get-top-new-news-hook 'spam-maybe-spam-stat-load)
   (remove-hook 'gnus-startup-hook 'spam-maybe-spam-stat-load)
-- 
2.29.2

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Eric Abrahamsen]

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

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I'm not sure why spam.el would need to work on the default value of
>> `nnimap-split-download-body'. If spam.el determines that widening is
>> needed, it's going to download message bodies across the board anyway.
>> The only reason to preserve the user's own customization would be if the
>> user later unloaded spam.el -- then it should restore the previous
>> value. But how often would that actually be useful?
>>
>> In principle, TRT would be to have spam.el let-bind a variable around
>> the splitting process. But I don't think the code is set up that way:
>> all the spam.el stuff happens "inside" the splitting process, not around
>> it.
>
> How about the attached kludgy but conservative dance?

It seems like a lot of work for something no one's likely to even
notice! But since you've already done the work I wouldn't object :)

bug#44981: 28.0.50; Restore nnimap-split-download-body?

[Basil L. Contovounesios]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> How about the attached kludgy but conservative dance?
>
> It seems like a lot of work for something no one's likely to even
> notice! But since you've already done the work I wouldn't object :)

Thanks, done.  :)

Avoid modifying nnimap user option in spam.el
e84a1ffde9 2020-12-06 11:45:52 +0000
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=e84a1ffde9047c1ca0acb9abcd6d31e3bfba457d

-- 
Basil