bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

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

Hello!

I am trying to solve the following problem: Using nnvirtual groups that
are made up of several nntp newsgroups I would like to limit by
`Newsgroups' when in the nnvirtual. From reading the code, it appears
that this means I cannot use NOV because then the extra headers list
won't be generated. I can set `nov-is-evil' and this works. Except when
I am also using agent mode, in which case it will always use the NOV.

The below patch is a work in progress and doesn't entirely work yet, but
I wanted to share it early before I spend more time on it.

The following questions have come up for me:

1) Is there a different way to solve this problem? Specifically, is
there a better way to limit by newsgroup when in an nnvirtual group?
2) Is it possible to set `nntp-nov-is-evil' as a group parameter? It
looks like header retrieval may be too early for this and I may need to
use a hook?
3) How would this approach interact with the local cache? I have not
looked at this path yet.

Thanks!

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

diff --git a/lisp/gnus/gnus-int.el b/lisp/gnus/gnus-int.el
index 01053797b3..d2d7fd55ae 100644
--- a/lisp/gnus/gnus-int.el
+++ b/lisp/gnus/gnus-int.el
@@ -527,7 +527,11 @@ gnus-retrieve-headers
      ((and gnus-use-cache (numberp (car articles)))
       (gnus-cache-retrieve-headers articles group fetch-old))
      ((and gnus-agent (gnus-online gnus-command-method)
-	   (gnus-agent-method-p gnus-command-method))
+	   (gnus-agent-method-p gnus-command-method)
+	   (not gnus-nov-is-evil)
+	   (not (symbol-value
+		 (intern
+		  (format "%s-nov-is-evil" (car gnus-command-method))))))
       (gnus-agent-retrieve-headers articles group fetch-old))
      (t
       (funcall (gnus-get-function gnus-command-method 'retrieve-headers)

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

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> 2) Is it possible to set `nntp-nov-is-evil' as a group parameter? It
> looks like header retrieval may be too early for this and I may need to
> use a hook?

How about setting it in an early group entry hook depending on the group
name?  And you can also set it to nil in an agent hook.

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

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> 2) Is it possible to set `nntp-nov-is-evil' as a group parameter? It
>> looks like header retrieval may be too early for this and I may need to
>> use a hook?
>
> How about setting it in an early group entry hook depending on the group
> name?  And you can also set it to nil in an agent hook.

I have experimented with this a bit and it looks like it works. Here's
what I did:

(add-hook 'gnus-summary-mode-hook 'nov-is-evil-for-nnvirtual)
(defun nov-is-evil-for-nnvirtual ()
  (if (string-match "^nnvirtual:.*" gnus-newsgroup-name)
      (setq gnus-nov-is-evil t)))

This, combined with the patch I sent in, will cause `Newsgroups' to be
recorded in the .overview files when using the agent for new messages.

Is there any documentation you would like to see before accepting this
change?

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> Is there any documentation you would like to see before accepting this
> change?

I don't think the patch is the right thing to do:

diff --git a/lisp/gnus/gnus-int.el b/lisp/gnus/gnus-int.el
index 01053797b3..d2d7fd55ae 100644
--- a/lisp/gnus/gnus-int.el
+++ b/lisp/gnus/gnus-int.el
@@ -527,7 +527,11 @@ gnus-retrieve-headers
      ((and gnus-use-cache (numberp (car articles)))
       (gnus-cache-retrieve-headers articles group fetch-old))
      ((and gnus-agent (gnus-online gnus-command-method)
-	   (gnus-agent-method-p gnus-command-method))
+	   (gnus-agent-method-p gnus-command-method)
+	   (not gnus-nov-is-evil)

So you're basically disabling the Agent when gnus-nov-is-evil is set,
and while it solves this problem, it's not a good general solution --
the two things are kinda-sort orthogonal.  Instead you should just
exclude the nnvirtual component groups from the Agent.

+	   (not (symbol-value
+		 (intern
+		  (format "%s-nov-is-evil" (car gnus-command-method))))))

(And this isn't correct anyway -- not all backends have a -nov-is-evil
variable).

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

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> Is there any documentation you would like to see before accepting this
>> change?
>
> I don't think the patch is the right thing to do:

I did mention in my initial email that this wasn't fully cooked and I am
happy to accept alternatives :)

> So you're basically disabling the Agent when gnus-nov-is-evil is set,
> and while it solves this problem, it's not a good general solution --
> the two things are kinda-sort orthogonal.  Instead you should just
> exclude the nnvirtual component groups from the Agent.

What I am trying to achieve is:

- Allow limiting and scoring by newsgroup name in an nnvirtual group
- Still use caching and agent mode for the nnvirtual and constituent
  groups

For the second part I simply set `gnus-agent-cache' to non-nil and add
the constituent groups to the agent. I didn't see a need to add the
virtual group to the agent, because as long as all constituent groups
are, I still get caching and agent mode.

If I use a hook on the nnvirtual group to set nov-is-evil, the
constituent groups end up with the extra headers in their .overview
file. That seems to address both points above, except that if I open the
constituent groups directly, I don't get that information. I tried to
set nov-is-evil as a group parameter for the constituent groups, but
that doesn't work when they are in an agent category and was the reason
for the below code.

> +	   (not (symbol-value
> +		 (intern
> +		  (format "%s-nov-is-evil" (car gnus-command-method))))))
>
> (And this isn't correct anyway -- not all backends have a -nov-is-evil
> variable).

I noticed that and ended up defining a nnvirtual-nov-is-evil, but it
never made it to this clause because of my specific setup.

P.S.: Is there a better place to discuss this? gmane.emacs.gnus.general?

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> What I am trying to achieve is:
>
> - Allow limiting and scoring by newsgroup name in an nnvirtual group
> - Still use caching and agent mode for the nnvirtual and constituent
>   groups
>
> For the second part I simply set `gnus-agent-cache' to non-nil and add
> the constituent groups to the agent. I didn't see a need to add the
> virtual group to the agent, because as long as all constituent groups
> are, I still get caching and agent mode.

Hm...  Am I misreading the patch, then?  This bit:

      ((and gnus-agent (gnus-online gnus-command-method)
-	   (gnus-agent-method-p gnus-command-method))
+	   (gnus-agent-method-p gnus-command-method)
+	   (not gnus-nov-is-evil)
..
       (gnus-agent-retrieve-headers articles group fetch-old))

seems to disable the agent then gnus-nov-is-evil is set?

> P.S.: Is there a better place to discuss this? gmane.emacs.gnus.general?

Sure, but it's very low volume these days, so there's probably not that
many more people reading that group than the bug list.  :-/

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

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> What I am trying to achieve is:
>>
>> - Allow limiting and scoring by newsgroup name in an nnvirtual group
>> - Still use caching and agent mode for the nnvirtual and constituent
>>   groups
>>
>> For the second part I simply set `gnus-agent-cache' to non-nil and add
>> the constituent groups to the agent. I didn't see a need to add the
>> virtual group to the agent, because as long as all constituent groups
>> are, I still get caching and agent mode.
>
> Hm...  Am I misreading the patch, then?  This bit:
>
>       ((and gnus-agent (gnus-online gnus-command-method)
> -	   (gnus-agent-method-p gnus-command-method))
> +	   (gnus-agent-method-p gnus-command-method)
> +	   (not gnus-nov-is-evil)
> ..
>        (gnus-agent-retrieve-headers articles group fetch-old))
>
> seems to disable the agent then gnus-nov-is-evil is set?

The scenario in which this change applies is when the agent is plugged
and when the group is in an agent category. In the case where the
nnvirtual is not in the agent, but the constituent groups are, I want to
not use NOV so I can get the extra headers.

I think your concern is that when `gnus-nov-is-evil' is set globally, it
will effectively disable the agent for everything? Maybe this logic
should reside in `gnus-agent-retrieve-headers' instead?

To address the unbound nov-is-evil for nnvirtual issue, this would work.

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

diff --git a/lisp/gnus/gnus-int.el b/lisp/gnus/gnus-int.el
index 01053797b3..3fe69ec471 100644
--- a/lisp/gnus/gnus-int.el
+++ b/lisp/gnus/gnus-int.el
@@ -522,12 +522,18 @@ gnus-close-group
 (defun gnus-retrieve-headers (articles group &optional fetch-old)
   "Request headers for ARTICLES in GROUP.
 If FETCH-OLD, retrieve all headers (or some subset thereof) in the group."
-  (let ((gnus-command-method (gnus-find-method-for-group group)))
+  (let* ((gnus-command-method (gnus-find-method-for-group group))
+         (gnus-method-nov-is-evil (intern
+		                   (format "%s-nov-is-evil"
+                                           (car gnus-command-method)))))
     (cond
      ((and gnus-use-cache (numberp (car articles)))
       (gnus-cache-retrieve-headers articles group fetch-old))
      ((and gnus-agent (gnus-online gnus-command-method)
-	   (gnus-agent-method-p gnus-command-method))
+	   (gnus-agent-method-p gnus-command-method)
+	   (not gnus-nov-is-evil)
+	   (not (and (boundp gnus-method-nov-is-evil)
+                     (symbol-value gnus-method-nov-is-evil))))
       (gnus-agent-retrieve-headers articles group fetch-old))
      (t
       (funcall (gnus-get-function gnus-command-method 'retrieve-headers)

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

>> P.S.: Is there a better place to discuss this? gmane.emacs.gnus.general?
>
> Sure, but it's very low volume these days, so there's probably not that
> many more people reading that group than the bug list.  :-/

That's unfortunate, I'll stick to the bugs list then.

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> I think your concern is that when `gnus-nov-is-evil' is set globally, it
> will effectively disable the agent for everything? Maybe this logic
> should reside in `gnus-agent-retrieve-headers' instead?

Yes, I think that sounds more reasonable.

> +	   (not (and (boundp gnus-method-nov-is-evil)
> +                     (symbol-value gnus-method-nov-is-evil))))

That's bound-and-true-p.  :-)

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

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> I think your concern is that when `gnus-nov-is-evil' is set globally, it
>> will effectively disable the agent for everything? Maybe this logic
>> should reside in `gnus-agent-retrieve-headers' instead?
>
> Yes, I think that sounds more reasonable.

OK, I will keep working on it.

>> +	   (not (and (boundp gnus-method-nov-is-evil)
>> +                     (symbol-value gnus-method-nov-is-evil))))
>
> That's bound-and-true-p.  :-)

Ah, didn't know about that one :)

Thanks!

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> I think your concern is that when `gnus-nov-is-evil' is set globally, it
>> will effectively disable the agent for everything? Maybe this logic
>> should reside in `gnus-agent-retrieve-headers' instead?

I looked some more into it and it comes down to whether or not the
component groups see a `nov-is-evil' during their initial header
retrieval. If they do, the overview files for the agent include the
necessary information and the cache appears to used as well. However, I
don't see a way to set `nntp-nov-is-evil' for a group in a way that
would still be true when it is part of a virtual group.

As an experiment, I globally set `nntp-nov-is-evil' to `t' and disabled
the agent to see if it even works. I ran into a lot of problems where
Gnus lost track of the unread messages for simple NNTP groups. Is this
expected or is this potentially a bug?

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> However, I don't see a way to set `nntp-nov-is-evil' for a group in a
> way that would still be true when it is part of a virtual group.

Perhaps the easiest would be to create a separate select method for
these groups that has nntp-nov-is-evil set as a select method variable.

> As an experiment, I globally set `nntp-nov-is-evil' to `t' and disabled
> the agent to see if it even works. I ran into a lot of problems where
> Gnus lost track of the unread messages for simple NNTP groups. Is this
> expected or is this potentially a bug?

Sounds like a bug to me.

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

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> However, I don't see a way to set `nntp-nov-is-evil' for a group in a
>> way that would still be true when it is part of a virtual group.
>
> Perhaps the easiest would be to create a separate select method for
> these groups that has nntp-nov-is-evil set as a select method variable.

That's awesome, it works! I wasn't even considering this option, but it
makes perfect sense.

How about adding this to the documentation?

2021-06-09  Alex Bochannek  <alex@bochannek.com>

	* doc/misc/gnus.texi (To From Newsgroups): Document extra header
          nnvirtual group scenario


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

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index 32ba5aee92..18c4b091d3 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -5246,6 +5246,13 @@ To From Newsgroups
 Be aware, though, that this will make entering an @acronym{NNTP} group
 much, much slower, so this is not recommended.
 
+One particular scenario in which it can be desirable to not use
+@samp{XOVER} is for @code{nnvirtual} groups in order to support
+limiting by extra headers (e.g., by the newsgroup of its component
+groups.) Because group parameters are not inherited, a separate select
+method for the component groups with the appropriate
+@code{nov-is-evil} set as a method variable is required.
+
 
 @node Summary Buffer Mode Line
 @subsection Summary Buffer Mode Line

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


>> As an experiment, I globally set `nntp-nov-is-evil' to `t' and disabled
>> the agent to see if it even works. I ran into a lot of problems where
>> Gnus lost track of the unread messages for simple NNTP groups. Is this
>> expected or is this potentially a bug?
>
> Sounds like a bug to me.

I must have gotten myself into an undefined state last night, can't
reproduce.

Thank you for your help! Learned a lot about how virtual groups work in
the process of trying to get this to work :)

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Eli Zaretskii]

> From: Alex Bochannek <alex@bochannek.com>
> Date: Wed, 09 Jun 2021 11:05:14 -0700
> Cc: 48801@debbugs.gnu.org
> 
> +One particular scenario in which it can be desirable to not use
> +@samp{XOVER} is for @code{nnvirtual} groups in order to support
> +limiting by extra headers (e.g., by the newsgroup of its component
> +groups.) Because group parameters are not inherited, a separate select
          ^^
Our conventions are to leave 2 spaces between sentences.

Thanks.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Bochannek <alex@bochannek.com>
>> Date: Wed, 09 Jun 2021 11:05:14 -0700
>> Cc: 48801@debbugs.gnu.org
>> 
>> +One particular scenario in which it can be desirable to not use
>> +@samp{XOVER} is for @code{nnvirtual} groups in order to support
>> +limiting by extra headers (e.g., by the newsgroup of its component
>> +groups.) Because group parameters are not inherited, a separate select
>           ^^
> Our conventions are to leave 2 spaces between sentences.

Thanks for pointing that out! Is there a style document for code and
documentation that I could use as a reference?


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

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index 32ba5aee92..e7ced97ef9 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -5246,6 +5246,13 @@ To From Newsgroups
 Be aware, though, that this will make entering an @acronym{NNTP} group
 much, much slower, so this is not recommended.
 
+One particular scenario in which it can be desirable to not use
+@samp{XOVER} is for @code{nnvirtual} groups in order to support
+limiting by extra headers (e.g., by the newsgroup of its component
+groups.)  Because group parameters are not inherited, a separate
+select method for the component groups with the appropriate
+@code{nov-is-evil} set as a method variable is required.
+
 
 @node Summary Buffer Mode Line
 @subsection Summary Buffer Mode Line

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

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Eli Zaretskii]

> From: Alex Bochannek <alex@bochannek.com>
> Cc: larsi@gnus.org,  48801@debbugs.gnu.org
> Date: Wed, 09 Jun 2021 11:47:32 -0700
> 
> > Our conventions are to leave 2 spaces between sentences.
> 
> Thanks for pointing that out! Is there a style document for code and
> documentation that I could use as a reference?

We use US English conventions, both for spelling and for punctuation.
I don't think we spell out these conventions explicitly somewhere, not
in their entirety anyway.  However, CONTRIBUTE does mention this
particular rule.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Bochannek <alex@bochannek.com>
>> Cc: larsi@gnus.org,  48801@debbugs.gnu.org
>> Date: Wed, 09 Jun 2021 11:47:32 -0700
>> 
>> > Our conventions are to leave 2 spaces between sentences.
>> 
>> Thanks for pointing that out! Is there a style document for code and
>> documentation that I could use as a reference?
>
> We use US English conventions, both for spelling and for punctuation.
> I don't think we spell out these conventions explicitly somewhere, not
> in their entirety anyway.  However, CONTRIBUTE does mention this
> particular rule.

I have to admit that I have not read CONTRIBUTE, thank you for
mentioning it. It clears up some other things that I was wondering
about, esp. around commit messages.

Thanks!

-- 
Alex.

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> +One particular scenario in which it can be desirable to not use
> +@samp{XOVER} is for @code{nnvirtual} groups in order to support
> +limiting by extra headers (e.g., by the newsgroup of its component
> +groups.)  Because group parameters are not inherited, a separate
> +select method for the component groups with the appropriate
> +@code{nov-is-evil} set as a method variable is required.

Thanks; applied to Emacs 28.  And I guess this means that the proposed
code changes aren't necessary, so I'm closing this bug report.  (If I
missed something, please pipe up.)

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

bug#48801: 28.0.50; [PATCH] Feature suggestion gnus-retrieve-headers check for nov-is-evil when agent is enabled

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> +One particular scenario in which it can be desirable to not use
>> +@samp{XOVER} is for @code{nnvirtual} groups in order to support
>> +limiting by extra headers (e.g., by the newsgroup of its component
>> +groups.)  Because group parameters are not inherited, a separate
>> +select method for the component groups with the appropriate
>> +@code{nov-is-evil} set as a method variable is required.
>
> Thanks; applied to Emacs 28.  And I guess this means that the proposed
> code changes aren't necessary, so I'm closing this bug report.  (If I
> missed something, please pipe up.)

Correct, thank you, no code changes, just documentation. Thanks again
for pointing me in the direction of select method variables, that solved
it!

-- 
Alex.