bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[jflack--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="=-=-=", Size: 2383 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

- --=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


The new gnus-search-indexed-parse-output doesn't properly quote group
names before using them as regexes meaning a group name containing
meta-characters (other than . or \ because of the current replacement)
won't be properly matched in the search results later.

I have attached a diff that fixes this by first quoting the group name
before performing the replacement; which is now constructed with RX
to save \\ soup.

=2D-=20
Thanks,
Jai

- --=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment; filename=gnus-search-regex.diff
Content-Transfer-Encoding: quoted-printable

diff --git a/lisp/gnus/gnus-search.el b/lisp/gnus/gnus-search.el
index 44f43b073c..54603d8792 100644
=2D-- a/lisp/gnus/gnus-search.el
+++ b/lisp/gnus/gnus-search.el
@@ -82,6 +82,7 @@
 (require 'gnus-util)
 (require 'eieio)
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'rx))
 (autoload 'eieio-build-class-alist "eieio-opt")
 (autoload 'nnmaildir-base-name-to-article-number "nnmaildir")
=20
@@ -1380,8 +1381,8 @@ gnus-search-indexed-parse-output
 			 (lambda (x)
 			   (replace-regexp-in-string
 			    ;; Accept any of [.\/] as path separators.
=2D			    "[.\\/]" "[.\\\\/]"
=2D			    (gnus-group-real-name x)))
+			    (rx (or "\\." "\\\\" "/")) "[.\\\\/]"
+			    (regexp-quote (gnus-group-real-name x))))
 			 groups "\\|")))
 	artlist vectors article group)
     (goto-char (point-min))

- --=-=-=--
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEuAb6YdjwXI5dBirisAc6s2XQgH0FAmAapdQACgkQsAc6s2XQ
gH1aehAAjdDYFs7rxva8vZQCHO4l6S84Ghgo3kqNyP3CXoitc7wPDAIZhvNYTnz3
/LBH/mtfJZA2TYhQkH6i3iNeF7PpJ5OhxJ1p3/in83yYHrhxsy94O61iBvElA3z0
TM8J5i3vuCc7xogze7vfGjXHe+gD7sCr/7EEFRN3Pan4bZGatJRRM3u5oKYbWmyM
dcQ+y+IzTODJ/ayx3nCX0H9r5grn7Iqh6W413bWEILr2YqTOOm37HJZ3LeDeegIv
0MitBjReUH3ePRhZtnOtyY9adbCX5AX4R65U8vLpIVTmunQ/xogHxg0L962RDlbt
4bSKrYHACbipGsBNvQ1ul76johoFRkCS/POkCAdt284VfxxwqXMxPXTJiy6ZEXjW
/4PpTZR0gGpoGMXgHjS0qT35gwmoF4rQoxGl6H4GDJIgHlg55wCgteSIANvqTEWl
8HHJ6S7pR3tbHTj4UYK+CrkrovAoW/eENtlueeuEFacvZUDfieCKwsS6SY7AdaMG
U2yetfZbLq42ZyVhK96h2pFNhOVUfn/gnLcZy1ghMlJgqMZ0S0836ql3OQGyLaJV
xwXrVJCLqz91f58TisjThMcgm7wm46fnhZ246CA3L+nZnyDQCLYtAQChDCco8O5f
ifG/JGqZqdsWGtXZGtGjj+1orCA4R+v+vxNNuytB5Wa+s8BzCCI=
=QS0M
-----END PGP SIGNATURE-----

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[Basil L. Contovounesios]

jflack--- via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> diff --git a/lisp/gnus/gnus-search.el b/lisp/gnus/gnus-search.el
> index 44f43b073c..54603d8792 100644
> =2D-- a/lisp/gnus/gnus-search.el
> +++ b/lisp/gnus/gnus-search.el
> @@ -82,6 +82,7 @@
>  (require 'gnus-util)
>  (require 'eieio)
>  (eval-when-compile (require 'cl-lib))
> +(eval-when-compile (require 'rx))

No need for this; rx is preloaded.

>  (autoload 'eieio-build-class-alist "eieio-opt")
>  (autoload 'nnmaildir-base-name-to-article-number "nnmaildir")
> =20
> @@ -1380,8 +1381,8 @@ gnus-search-indexed-parse-output
>  			 (lambda (x)
>  			   (replace-regexp-in-string
>  			    ;; Accept any of [.\/] as path separators.
> =2D			    "[.\\/]" "[.\\\\/]"
> =2D			    (gnus-group-real-name x)))
> +			    (rx (or "\\." "\\\\" "/")) "[.\\\\/]"
> +			    (regexp-quote (gnus-group-real-name x))))
>  			 groups "\\|")))
>  	artlist vectors article group)
>      (goto-char (point-min))

BTW, your mail content seems to be mangled somewhere:
https://debbugs.gnu.org/46271.

-- 
Basil

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[Eric Abrahamsen]

jflack--- via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> - --=-=-=
> Content-Type: text/plain
> Content-Transfer-Encoding: quoted-printable
>
>
> The new gnus-search-indexed-parse-output doesn't properly quote group
> names before using them as regexes meaning a group name containing
> meta-characters (other than . or \ because of the current replacement)
> won't be properly matched in the search results later.
>
> I have attached a diff that fixes this by first quoting the group name
> before performing the replacement; which is now constructed with RX
> to save \\ soup.
>
> =2D-=20
> Thanks,
> Jai
>
> - --=-=-=
> Content-Type: text/x-diff
> Content-Disposition: attachment; filename=gnus-search-regex.diff
> Content-Transfer-Encoding: quoted-printable
>
> diff --git a/lisp/gnus/gnus-search.el b/lisp/gnus/gnus-search.el
> index 44f43b073c..54603d8792 100644
> =2D-- a/lisp/gnus/gnus-search.el
> +++ b/lisp/gnus/gnus-search.el
> @@ -82,6 +82,7 @@
>  (require 'gnus-util)
>  (require 'eieio)
>  (eval-when-compile (require 'cl-lib))
> +(eval-when-compile (require 'rx))
>  (autoload 'eieio-build-class-alist "eieio-opt")
>  (autoload 'nnmaildir-base-name-to-article-number "nnmaildir")
> =20
> @@ -1380,8 +1381,8 @@ gnus-search-indexed-parse-output
>  			 (lambda (x)
>  			   (replace-regexp-in-string
>  			    ;; Accept any of [.\/] as path separators.
> =2D			    "[.\\/]" "[.\\\\/]"
> =2D			    (gnus-group-real-name x)))
> +			    (rx (or "\\." "\\\\" "/")) "[.\\\\/]"
> +			    (regexp-quote (gnus-group-real-name x))))
>  			 groups "\\|")))
>  	artlist vectors article group)
>      (goto-char (point-min))

Thanks very much for the patch! Let me do some local testing over the
next couple of days, but at first glance this looks like it will do the
trick.

Thanks,
Eric

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[jflack--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> No need for this; rx is preloaded.
Apologies if this is off topic but; is it loaded before the start of
byte-compilation or somewhere else? I can find it (require 'rx)'d before
the start of tests but nowhere else.

> BTW, your mail content seems to be mangled somewhere:
> https://debbugs.gnu.org/46271.
Thanks for pointing it out, seems to be from giving a signed mail to
`message-send-and-exit'.

-- 
Thanks,
Jai

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[Basil L. Contovounesios]

Jai Flack <jflack@disroot.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> No need for this; rx is preloaded.
> Apologies if this is off topic but; is it loaded before the start of
> byte-compilation or somewhere else? I can find it (require 'rx)'d before
> the start of tests but nowhere else.

Sorry, I meant autoloaded, not preloaded.

There is no need to (require 'rx) unless using internal rx.el
definitions (needed in rx-tests.el, for example).  Most other cases of
(require 'rx), regardless of whether in normal libraries or test files,
can be removed.

-- 
Basil

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> jflack--- via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs@gnu.org> writes:
>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA512
>>
>> - --=-=-=
>> Content-Type: text/plain
>> Content-Transfer-Encoding: quoted-printable
>>
>>
>> The new gnus-search-indexed-parse-output doesn't properly quote group
>> names before using them as regexes meaning a group name containing
>> meta-characters (other than . or \ because of the current replacement)
>> won't be properly matched in the search results later.
>>
>> I have attached a diff that fixes this by first quoting the group name
>> before performing the replacement; which is now constructed with RX
>> to save \\ soup.
>>
>> =2D-=20
>> Thanks,
>> Jai
>>
>> - --=-=-=
>> Content-Type: text/x-diff
>> Content-Disposition: attachment; filename=gnus-search-regex.diff
>> Content-Transfer-Encoding: quoted-printable
>>
>> diff --git a/lisp/gnus/gnus-search.el b/lisp/gnus/gnus-search.el
>> index 44f43b073c..54603d8792 100644
>> =2D-- a/lisp/gnus/gnus-search.el
>> +++ b/lisp/gnus/gnus-search.el
>> @@ -82,6 +82,7 @@
>>  (require 'gnus-util)
>>  (require 'eieio)
>>  (eval-when-compile (require 'cl-lib))
>> +(eval-when-compile (require 'rx))
>>  (autoload 'eieio-build-class-alist "eieio-opt")
>>  (autoload 'nnmaildir-base-name-to-article-number "nnmaildir")
>> =20
>> @@ -1380,8 +1381,8 @@ gnus-search-indexed-parse-output
>>  			 (lambda (x)
>>  			   (replace-regexp-in-string
>>  			    ;; Accept any of [.\/] as path separators.
>> =2D			    "[.\\/]" "[.\\\\/]"
>> =2D			    (gnus-group-real-name x)))
>> +			    (rx (or "\\." "\\\\" "/")) "[.\\\\/]"
>> +			    (regexp-quote (gnus-group-real-name x))))
>>  			 groups "\\|")))
>>  	artlist vectors article group)
>>      (goto-char (point-min))
>
> Thanks very much for the patch! Let me do some local testing over the
> next couple of days, but at first glance this looks like it will do the
> trick.

Finally getting to this...

This doesn't quite do what it intends to do, as the
`replace-regexp-in-string' interferes with the `regexp-quote'. To wit:

(let ((group-name "mail+"))
  (replace-regexp-in-string
   "[.\\/]" "[.\\\\/]"
   (regexp-quote (gnus-group-real-name group-name))))
-> "mail[.\\/]+"

But what we wanted was:

"mail\\+"

So I think it has to get messier:

(let ((groups '("mail+" "gnus.gener+al" "archive.2007.10")))
  (mapconcat
   #'identity
   (mapcar
    (lambda (group-name)
      (mapconcat #'regexp-quote
		 (split-string
		  (gnus-group-real-name group-name)
		  "[.\\/]")
		 "[.\\\\/]"))
    groups)
   "\\|"))

-> "mail\\+\\|gnus[.\\\\/]gener\\+al\\|archive[.\\\\/]2007[.\\\\/]10"

This looks right to me. WDYT?

Also, I'd prefer not to use rx in this case, simply because this:

"[.\\/]"

turns into this:

(rx (or "\\." "\\\\" "/"))

Which seems like a loss of readability, though in general I think rx is
a very good idea.

Thanks again for the report!

Eric

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[Basil L. Contovounesios]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> So I think it has to get messier:
>
> (let ((groups '("mail+" "gnus.gener+al" "archive.2007.10")))
>   (mapconcat
>    #'identity
>    (mapcar
>     (lambda (group-name)
>       (mapconcat #'regexp-quote
> 		 (split-string
> 		  (gnus-group-real-name group-name)
> 		  "[.\\/]")
> 		 "[.\\\\/]"))
>     groups)
>    "\\|"))

AKA:

  (let ((groups '("mail+" "gnus.gener+al" "archive.2007.10")))
    (mapconcat
     (lambda (group-name)
       (mapconcat #'regexp-quote
                  (split-string
                   (gnus-group-real-name group-name)
                   "[.\\/]")
                  "[.\\\\/]"))
     groups
     "\\|"))

> Also, I'd prefer not to use rx in this case, simply because this:
>
> "[.\\/]"
>
> turns into this:
>
> (rx (or "\\." "\\\\" "/"))

Or (rx (in "./\\")), or (rx (in ?. ?/ ?\\)), or...

-- 
Basil

bug#46271: 28.0.50; [PATCH] Properly quote group names for gnus-search

[Eric Abrahamsen]

On 02/08/21 19:11 PM, Basil L. Contovounesios wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> So I think it has to get messier:
>>
>> (let ((groups '("mail+" "gnus.gener+al" "archive.2007.10")))
>>   (mapconcat
>>    #'identity
>>    (mapcar
>>     (lambda (group-name)
>>       (mapconcat #'regexp-quote
>> 		 (split-string
>> 		  (gnus-group-real-name group-name)
>> 		  "[.\\/]")
>> 		 "[.\\\\/]"))
>>     groups)
>>    "\\|"))
>
> AKA:
>
>   (let ((groups '("mail+" "gnus.gener+al" "archive.2007.10")))
>     (mapconcat
>      (lambda (group-name)
>        (mapconcat #'regexp-quote
>                   (split-string
>                    (gnus-group-real-name group-name)
>                    "[.\\/]")
>                   "[.\\\\/]"))
>      groups
>      "\\|"))

Ha! That was a brain malfunction.

>> Also, I'd prefer not to use rx in this case, simply because this:
>>
>> "[.\\/]"
>>
>> turns into this:
>>
>> (rx (or "\\." "\\\\" "/"))
>
> Or (rx (in "./\\")), or (rx (in ?. ?/ ?\\)), or...

That first one's not bad, but I still don't feel like it's a real
improvement over the plain regexp.

Anyway, thanks for the tuneup. I think we can just go with this.

Eric