Subject: bug#75305: 31.0.50; gnus-refer-thread-use-search isn't exact enough about how the current group is searched

From: Björn Bidar <[email-masked]>
Date: Mon, 24 Feb 2025 18:07:48 +0200

Andrew Cohen <[email-masked]> writes:

>>>>>> "BB" == Björn Bidar <[email-masked]> writes:
>
>     BB> Andrew Cohen <[email-masked]> writes:
>     >>>>>>> "EZ" == Eli Zaretskii <[email-masked]> writes:
>     >> 
>     EZ> Ping! Can we please make some progress here?
>     >> 
>     >> All set to commit, I think. I have been running with the
>     >> following in my personal tree for the past few weeks without
>     >> issue. If no objections we can commit.
>
>     BB> I would have hoped there would be a response to my mail. Further
>     BB> I don't mind to refactor the contribution to the issues you
>     BB> mentioned but I don't think it is great to just take over IMHO.
>
>
> I sincerely apologize. It was never my intent to take over and I
> definitely don't want to make you feel disenfranchised. I am perfectly
> happy to let you continue to manage this issue.

No problem for me it only about the communication of this. I.e. someone
contributes and an existing developer comes with a counter patch that
incorporates the work from themselves.

> Eli contacted me about this since I wrote and maintain the code that is
> implicated in this change. In the end I thought that the tiny change
> (along with the defcustom and documentation changes that you suggested)
> in gnus-sum.el was the best way to introduce the feature that you
> suggested.

Well your patch effectively does the same minus including 'current as a
possible value for gnus-refer-thread-use-search without any other values
and slightly different documentation.
As long as the bug is fixed I am fine, there are other bugs there to
fix/patches to review for Gnus.

Acknowledgment/credit helps. It also helpful to label the reviewers
and/or those who suggested a change or reviewed a patch with commit
message trailer see: git-interpret-trailers(1) (the man page in context
is for the helper tool which isn't a requirement to use these but it
does explain the concept).

> And again I apologize for not responding to your previous email.
>
>>In think this would be potentially to slow. That's what's happening in
>>nnselect refer article which can cause that it searches on the whole
>>server e.g. an imap server which means it could take hours to finish
>>searching.
>
> This seems like something might be broken? I routinely search imap
> servers with 100 groups and it finishes in a few seconds. Does your imap
> server maintain a searchable index? All of them should, and then the
> searching is basically instantaneous, aside from the cost of the network
> round trip and the changing of groups.

You mean like as in full-text search? I don't think I have that enabled.
Good pointer.

>>>From my point of view when talking about a non-virtual group it does
>>makes sense to refer to the "current" as in the group that the user is
>>in when pressing A T.
>>However I get the point of that not being idea for nnselect groups. To
>>be honest I did not think of that when writing the patch. I agree using
>>artgroup is better in this context. This also aligns with the original
>>behavior I wanted to align with when writing the patch i.e. to include
>>the groups listed in gnus-refer-thread-use-search but also include the
>>articles which would be found if gnus-refer-thread-use-search was nil.
>
> Although I originally suggested this, I think I was mistaken for doing
> so. The nnselect search routine does indeed collect the component groups
> and search them. So this aligns the behavior of 'current with
> gnus-refer-thread-use-search set to nil.

Oh that matches in retrospect what I experienced to when trying to limit
searches in nnselect-ephemeral groups to the groups the articles come
from.  

>>>From my personal point of the just that was the goal so I could e.g. put
>>the send group into the list of groups searched when building the thread
>>including the current group.
>
> Agreed. 
>
> FWIW here is the patch with Eli's suggested fixes incorporated. Feel
> free to discard---I won't be offended. 
>
> From 13d4062453bd8de5718d1781a701f26e17248061 Mon Sep 17 00:00:00 2001
> From: Andrew G Cohen <[email-masked]>
> Date: Sun, 23 Feb 2025 11:18:37 +0800
> Subject: [PATCH] Allow 'current in gnus-refer-thread-use-search
>
> The variable `gnus-refer-thread-use-search' controls which groups are
> searched when finding articles in a thread.  The symbol `current' can be
> used to include the current group in those searched.
>
> * doc/misc/gnus.texi: Document the use of `current' in
> `gnus-refer-thread-use-search'.
> * lisp/gnus/gnus-sum.el (gnus-summary-refer-thread): Resolve the symbol
> `current' to the group from which `gnus-summary-refer-thread' was
> invoked.
> ---
>  doc/misc/gnus.texi    | 25 ++++++++++++++-------
>  lisp/gnus/gnus-sum.el | 51 +++++++++++++++++++++++++------------------
>  2 files changed, 47 insertions(+), 29 deletions(-)
>
> diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
> index bdfab7a8718..b09c059cf9a 100644
> --- a/doc/misc/gnus.texi
> +++ b/doc/misc/gnus.texi
> @@ -10546,8 +10546,7 @@ Finding the Parent
>  @findex gnus-summary-refer-thread
>  @kindex A T @r{(Summary)}
>  Display the full thread where the current article appears
> -(@code{gnus-summary-refer-thread}).  By default this command looks for
> -articles only in the current group.  If the group belongs to a backend
> +(@code{gnus-summary-refer-thread}).  If the group belongs to a backend
>  that has an associated search engine, articles are found by searching.
>  In other cases each header in the current group must be fetched and
>  examined, so it usually takes a while.  If you do it often, you may
> @@ -10557,12 +10556,22 @@ Finding the Parent
>  course, it'll make group entry somewhat slow.
>  
>  @vindex gnus-refer-thread-use-search
> -If @code{gnus-refer-thread-use-search} is @code{nil} (the default)
> -then thread-referral only looks for articles in the current group.  If
> -this variable is @code{t} the server to which the current group
> -belongs is searched (provided that searching is available for the
> -server's backend).  If this variable is a list of servers, each server
> -in the list is searched.
> +If @code{gnus-refer-thread-use-search} is @code{nil} (the default), then
> +thread-referral only looks for articles in the current group.
> +
> +If this variable is @code{t}, and the server to which the current group
> +belongs has an associated search engine, then thread-referral searches
> +this server.

It might be helpful to either link to the relevant Info node or to
`gnus-search-default-engines' to provide context.

> +
> +This variable may also be a list of servers and groups to search.  Each
> +element of the list is of the form @w{@code{(@var{server} @var{group1}
> +@var{group2}@dots{})}}, where @var{server} is the server and
> +@var{group1}, @var{group2}@dots{} are the groups belonging to this
> +server that are to be searched.  If the @var{group}s are absent the
> +entire server is searched.
> +
> +This list may also contain the special symbol @code{current}, which will
> +add the current group to the list of groups to be searched.
>  
>  @vindex gnus-refer-thread-limit
>  The @code{gnus-refer-thread-limit} variable says how many old (i.e.,
> diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
> index 6294825b400..672e4e1a71d 100644
> --- a/lisp/gnus/gnus-sum.el
> +++ b/lisp/gnus/gnus-sum.el
> @@ -145,15 +145,21 @@ 'gnus-refer-thread-use-nnir
>  (defcustom gnus-refer-thread-use-search nil
>    "Specify where to find articles when referring threads.
>  A nil value restricts searches for thread-related articles to the
> -current group; a value of t searches all groups on the server; a
> -list of servers and groups (where each element is a list whose
> -car is the server and whose cdr is a list of groups on this
> -server or nil to search the entire server) searches these
> -server/groups.  This may usefully be set as a group parameter."

You removed the suggestion here. I suggest to move that to the end of
the docstring. 

> -  :version "28.1"
> +current group; a value of t searches all groups on the server to which
> +the group belongs; a list of servers and groups (where each element is a
> +list whose car is the server and whose cdr is a list of groups on this
> +server, or nil to search the entire server) searches just these
> +server/groups.
> +
> +The list of server/groups may contain the symbol `current' which
> +includes the current group in the list of those searched."

Is server/groups or server and groups better? Personally I think using
and instead of / is easier to read.

> +  :version "31.1"
>    :group 'gnus-thread
> -  :type '(restricted-sexp :match-alternatives
> -                          (listp 't 'nil)))
> +  :type '(choice (const :tag "Current group" nil)
> +                 (const :tag "All groups" t)
> +                 (repeat :tag "Server and groups"
> +                         (choice (const :tag "Current Group" current)
> +                                 (repeat :tag "Server and groups" string)))))
>  
>  (defcustom gnus-refer-thread-limit-to-thread nil
>    "If non-nil referring a thread will limit the summary buffer to
> @@ -9014,25 +9020,28 @@ gnus-delete-duplicate-headers
>  
>  (defun gnus-summary-refer-thread (&optional limit)
>    "Fetch all articles in the current thread.
> -A non-numeric prefix arg will search the entire server; without a
> -prefix arg only the current group is searched.  If the variable
> -`gnus-refer-thread-use-search' is t the prefix arg has the
> -reverse meaning.  If searching is not enabled for the current
> -group, fetch LIMIT (the numerical prefix) old headers.  If LIMIT
> -is non-numeric or nil fetch the number specified by the
> -`gnus-refer-thread-limit' variable."
> +Find articles in the current group by default.  With a non-numeric
> +prefix arg, search all groups on the current server.  If the variable
> +`gnus-refer-thread-use-search' is t, invert the effect of the prefix
> +arg; if it is a list, search only those groups in the list.  If the
> +current group does not have a search engine enabled, find articles by
> +fetching LIMIT (the numerical prefix) old headers; if LIMIT is
> +non-numeric or nil fetch the number specified by
> +`gnus-refer-thread-limit'."
>    (interactive "P" gnus-summary-mode)
>    (let* ((group gnus-newsgroup-name)
> +         (server (gnus-method-to-server (gnus-find-method-for-group group)))
>           (header (gnus-summary-article-header))
>           (id (mail-header-id header))
>           (gnus-inhibit-demon t)
>           (gnus-summary-ignore-duplicates t)
>           (gnus-refer-thread-use-search
> -          (if (or (null limit) (numberp limit))
> -              gnus-refer-thread-use-search
> -            (if (booleanp gnus-refer-thread-use-search)
> -                (not gnus-refer-thread-use-search)
> -              gnus-refer-thread-use-search))))
> +          (if (listp gnus-refer-thread-use-search)
> +              (cl-subst (list server group) 'current
> +                        gnus-refer-thread-use-search)
> +            (if (or (null limit) (numberp limit))
> +                gnus-refer-thread-use-search
> +              (not gnus-refer-thread-use-search)))))
>      (gnus-summary-insert-articles
>       (cond
>        ;; If there is a backend-specific method, use it.
> @@ -9041,7 +9050,7 @@ gnus-summary-refer-thread
>         (gnus-request-thread header group))
>        ;; If a search engine is configured, use it.
>        ((ignore-errors
> -         (gnus-search-server-to-engine (gnus-group-server group)))
> +         (gnus-search-server-to-engine server))
>         (gnus-search-thread header))
>        ;; Otherwise just retrieve some headers.
>        (t
> -- 
> 2.47.2