bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

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

When changing grep-command or grep-find-command to e.g. ripgrep matches are not highlighted in the grep buffer. This patches makes the regexp that is used to identify matches customizable and hence possible to adapt it to potential grep replacements.

For example:

change  grep command to 

"rg -n -H -S --no-heading --color always -e "

and grep-match-regexp to

"\033\\[[0-9]*m\033\\[[0-9]*1m\033\\[[0-9]*1m\\(.*?\\)\033\\[[0-9]*0m"

to get correct highlighting with ripgrep.



[-- Attachment #2: 0001-Make-regexp-used-to-highlight-grep-matches-customiza.patch --]
[-- Type: application/octet-stream, Size: 1421 bytes --]

From 0916b4efa6661cd3f6ad9889cfea83580bf17fe9 Mon Sep 17 00:00:00 2001
From: Simon Lang <simon.lang@outlook.com>
Date: Mon, 8 Jun 2020 20:47:08 +0100
Subject: [PATCH] Make regexp used to highlight grep matches customizable

* lisp/progmodes/grep.el
---
 lisp/progmodes/grep.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 7731be5965..08c8ca071b 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -100,6 +100,9 @@ To change the default value, use \\[customize] or call the function
   :set #'grep-apply-setting
   :version "22.1")
 
+(defcustom grep-regexp-match "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
+  "Regex definition to identify grep markers to highlight matches.")
+
 (defcustom grep-scroll-output nil
   "Non-nil to scroll the *grep* buffer window as output appears.
 
@@ -584,7 +587,7 @@ This function is called from `compilation-filter-hook'."
       (when (< (point) end)
         (setq end (copy-marker end))
         ;; Highlight grep matches and delete marking sequences.
-        (while (re-search-forward "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m" end 1)
+        (while (re-search-forward grep-regexp-match end 1)
           (replace-match (propertize (match-string 1)
                                      'face nil 'font-lock-face grep-match-face)
                          t t)
-- 
2.21.0 (Apple Git-122)

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Juri Linkov]

> For example:
>
> change  grep command to
>
> "rg -n -H -S --no-heading --color always -e "
>
> and grep-match-regexp to

Thanks, grep-match-regexp is a good name, but your patch uses some less suitable
name grep-regexp-match.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Dmitry Gutov]

On 08.06.2020 23:25, Simon Lang wrote:
> For example:
> 
> change  grep command to
> 
> "rg -n -H -S --no-heading --color always -e"

I wonder if we can use a similar customization more generally. For 
instance, in my testing Grep searches the full Emacs checkout in ~230ms, 
whereas RipGrep does that in ~40ms. The difference is perceptible.

The obvious idea is to use grep-template, but we're passing -s and -E to 
it. rg would interpret these options differently.

Here's a perftest patch if someone was personally curious about the 
difference:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 5b5fb4bc47..2c6ed4da7d 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1262,7 +1262,7 @@ xref-matches-in-files
         (dir (file-name-directory (car files)))
         (remote-id (file-remote-p dir))
         ;; 'git ls-files' can output broken symlinks.
-       (command (format "xargs -0 grep %s -snHE -e %s"
+       (command (format "xargs -0 rg %s -nH -e %s"
                          (if (and case-fold-search
                                   (isearch-no-upper-case-p regexp t))
                              "-i"
@@ -1275,6 +1275,7 @@ xref-matches-in-files
                         #'tramp-file-local-name
                         #'file-local-name)
                     files)))
+    (setq tt (time-to-seconds))
      (with-current-buffer output
        (erase-buffer)
        (with-temp-buffer
@@ -1289,6 +1290,7 @@ xref-matches-in-files
                                              shell-command-switch
                                              command)))
        (goto-char (point-min))
+      (message "%s" (- (time-to-seconds) tt))
        (when (and (/= (point-min) (point-max))
                   (not (looking-at grep-re))
                   ;; TODO: Show these matches as well somehow?

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

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

Hi,

I changed grep-regexp-match to grep-match-regexp.

Pls note that ripgrep knows about ignore files etc. hence the more fair comparison would probably be git grep (e.g. vc-git-grep). But ripgrep is still considerably faster and does not only work for git repositories.

Thanks!

________________________________________
From: DG <raaahh@gmail.com> on behalf of Dmitry Gutov <dgutov@yandex.ru>
Sent: 09 June 2020 01:44
To: Simon Lang; 41766@debbugs.gnu.org
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

On 08.06.2020 23:25, Simon Lang wrote:
> For example:
>
> change  grep command to
>
> "rg -n -H -S --no-heading --color always -e"

I wonder if we can use a similar customization more generally. For
instance, in my testing Grep searches the full Emacs checkout in ~230ms,
whereas RipGrep does that in ~40ms. The difference is perceptible.

The obvious idea is to use grep-template, but we're passing -s and -E to
it. rg would interpret these options differently.

Here's a perftest patch if someone was personally curious about the
difference:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 5b5fb4bc47..2c6ed4da7d 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1262,7 +1262,7 @@ xref-matches-in-files
         (dir (file-name-directory (car files)))
         (remote-id (file-remote-p dir))
         ;; 'git ls-files' can output broken symlinks.
-       (command (format "xargs -0 grep %s -snHE -e %s"
+       (command (format "xargs -0 rg %s -nH -e %s"
                          (if (and case-fold-search
                                   (isearch-no-upper-case-p regexp t))
                              "-i"
@@ -1275,6 +1275,7 @@ xref-matches-in-files
                         #'tramp-file-local-name
                         #'file-local-name)
                     files)))
+    (setq tt (time-to-seconds))
      (with-current-buffer output
        (erase-buffer)
        (with-temp-buffer
@@ -1289,6 +1290,7 @@ xref-matches-in-files
                                              shell-command-switch
                                              command)))
        (goto-char (point-min))
+      (message "%s" (- (time-to-seconds) tt))
        (when (and (/= (point-min) (point-max))
                   (not (looking-at grep-re))
                   ;; TODO: Show these matches as well somehow?

[-- Attachment #2: 0001-Make-regexp-used-to-highlight-grep-matches-customiza.patch --]
[-- Type: application/octet-stream, Size: 1421 bytes --]

From bc9b736ff20a03e831bc5110283ccf9241127773 Mon Sep 17 00:00:00 2001
From: Simon Lang <simon.lang@outlook.com>
Date: Mon, 8 Jun 2020 20:47:08 +0100
Subject: [PATCH] Make regexp used to highlight grep matches customizable

* lisp/progmodes/grep.el
---
 lisp/progmodes/grep.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 7731be5965..3f0c99f6dc 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -100,6 +100,9 @@ To change the default value, use \\[customize] or call the function
   :set #'grep-apply-setting
   :version "22.1")
 
+(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
+  "Regex definition to identify grep markers to highlight matches.")
+
 (defcustom grep-scroll-output nil
   "Non-nil to scroll the *grep* buffer window as output appears.
 
@@ -584,7 +587,7 @@ This function is called from `compilation-filter-hook'."
       (when (< (point) end)
         (setq end (copy-marker end))
         ;; Highlight grep matches and delete marking sequences.
-        (while (re-search-forward "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m" end 1)
+        (while (re-search-forward grep-match-regexp end 1)
           (replace-match (propertize (match-string 1)
                                      'face nil 'font-lock-face grep-match-face)
                          t t)
-- 
2.21.0 (Apple Git-122)

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Basil L. Contovounesios]

Simon Lang <Simon.lang@outlook.com> writes:

> From bc9b736ff20a03e831bc5110283ccf9241127773 Mon Sep 17 00:00:00 2001
> From: Simon Lang <simon.lang@outlook.com>
> Date: Mon, 8 Jun 2020 20:47:08 +0100
> Subject: [PATCH] Make regexp used to highlight grep matches customizable
>
> * lisp/progmodes/grep.el
> ---
>  lisp/progmodes/grep.el | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
> index 7731be5965..3f0c99f6dc 100644
> --- a/lisp/progmodes/grep.el
> +++ b/lisp/progmodes/grep.el
> @@ -100,6 +100,9 @@ To change the default value, use \\[customize] or call the function
>    :set #'grep-apply-setting
>    :version "22.1")
>  
> +(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
> +  "Regex definition to identify grep markers to highlight matches.")

Nit: How about "Regular expression matching grep markers to highlight."

Every defcustom also needs:

  :type 'regexp
  :version "28.1"

as well as possibly being announced in etc/NEWS.

I wonder, though: the default value matches some quite obscure codes
which aren't (and maybe shouldn't be) documented, so is a defcustom
really suitable for this?  Or would a defvar suffice?  (I don't have
strong feelings either way.)

Thanks,

-- 
Basil

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Dmitry Gutov]

On 09.06.2020 10:58, Simon Lang wrote:
> Pls note that ripgrep knows about ignore files etc.

But not when passed specific file names on the command line, right? 
Which is what xargs does.

Sorry, it's a tangent from your patch's discussion. It looks good to me, 
but I'd rather defer the review to someone else.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

True, I think you're right.

Cheers,

Simon

________________________________________
From: DG <raaahh@gmail.com> on behalf of Dmitry Gutov <dgutov@yandex.ru>
Sent: 09 June 2020 13:15
To: Simon Lang; 41766@debbugs.gnu.org; Juri Linkov
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

On 09.06.2020 10:58, Simon Lang wrote:
> Pls note that ripgrep knows about ignore files etc.

But not when passed specific file names on the command line, right?
Which is what xargs does.

Sorry, it's a tangent from your patch's discussion. It looks good to me,
but I'd rather defer the review to someone else.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

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

I am happy with defvar instead (attached - I changed the commit message accordingly). I admit it would not be straight forward to customize anyway without doing some research.

Thanks!


________________________________________
From: Basil L. Contovounesios <contovob@tcd.ie>
Sent: 09 June 2020 12:55
To: Simon Lang
Cc: Dmitry Gutov; 41766@debbugs.gnu.org; Juri Linkov
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

Simon Lang <Simon.lang@outlook.com> writes:

> From bc9b736ff20a03e831bc5110283ccf9241127773 Mon Sep 17 00:00:00 2001
> From: Simon Lang <simon.lang@outlook.com>
> Date: Mon, 8 Jun 2020 20:47:08 +0100
> Subject: [PATCH] Make regexp used to highlight grep matches customizable
>
> * lisp/progmodes/grep.el
> ---
>  lisp/progmodes/grep.el | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
> index 7731be5965..3f0c99f6dc 100644
> --- a/lisp/progmodes/grep.el
> +++ b/lisp/progmodes/grep.el
> @@ -100,6 +100,9 @@ To change the default value, use \\[customize] or call the function
>    :set #'grep-apply-setting
>    :version "22.1")
>
> +(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
> +  "Regex definition to identify grep markers to highlight matches.")

Nit: How about "Regular expression matching grep markers to highlight."

Every defcustom also needs:

  :type 'regexp
  :version "28.1"

as well as possibly being announced in etc/NEWS.

I wonder, though: the default value matches some quite obscure codes
which aren't (and maybe shouldn't be) documented, so is a defcustom
really suitable for this?  Or would a defvar suffice?  (I don't have
strong feelings either way.)

Thanks,

--
Basil

[-- Attachment #2: 0001-Remove-hardcoding-of-regexp-used-to-highlight-grep-m.patch --]
[-- Type: application/octet-stream, Size: 1429 bytes --]

From 724d83a211aae922b719a28cc4f9cc75d88cc4ee Mon Sep 17 00:00:00 2001
From: Simon Lang <simon.lang@outlook.com>
Date: Tue, 9 Jun 2020 13:38:07 +0100
Subject: [PATCH] Remove hardcoding of regexp used to highlight grep matches.

* lisp/progmodes/grep.el
---
 lisp/progmodes/grep.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 7731be5965..618c2935f0 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -353,6 +353,10 @@ Notice that using \\[next-error] or \\[compile-goto-error] modifies
 (defvar grep-match-face	'match
   "Face name to use for grep matches.")
 
+(defvar grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
+  "Regular expression matching grep markers to highlight, used
+in `grep-filter'.")
+
 ;;;###autoload
 (defconst grep-regexp-alist
   `((,(concat "^\\(?:"
@@ -584,7 +588,7 @@ This function is called from `compilation-filter-hook'."
       (when (< (point) end)
         (setq end (copy-marker end))
         ;; Highlight grep matches and delete marking sequences.
-        (while (re-search-forward "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m" end 1)
+        (while (re-search-forward grep-match-regexp end 1)
           (replace-match (propertize (match-string 1)
                                      'face nil 'font-lock-face grep-match-face)
                          t t)
-- 
2.21.0 (Apple Git-122)

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Eli Zaretskii]

> From: Simon Lang <simon.lang@outlook.com>
> Date: Mon, 8 Jun 2020 20:25:06 +0000
> 
> +(defcustom grep-regexp-match "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
> +  "Regex definition to identify grep markers to highlight matches.")

Please don't forget the :version tag and the NEWS and the manual
updates.

Thanks.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Basil L. Contovounesios]

Simon Lang <Simon.lang@outlook.com> writes:

> I am happy with defvar instead (attached - I changed the commit message
> accordingly). I admit it would not be straight forward to customize anyway
> without doing some research.

Thanks, just some convention tips from me.

> From 724d83a211aae922b719a28cc4f9cc75d88cc4ee Mon Sep 17 00:00:00 2001
> From: Simon Lang <simon.lang@outlook.com>
> Date: Tue, 9 Jun 2020 13:38:07 +0100
> Subject: [PATCH] Remove hardcoding of regexp used to highlight grep matches.
>
> * lisp/progmodes/grep.el

See etc/CONTRIBUTE for commit message conventions.
In particular, it should list the definitions being changed, e.g.:

* lisp/progmodes/grep.el (grep-match-regexp): New variable.
(grep-filter): Use it.

> ---
>  lisp/progmodes/grep.el | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
> index 7731be5965..618c2935f0 100644
> --- a/lisp/progmodes/grep.el
> +++ b/lisp/progmodes/grep.el
> @@ -353,6 +353,10 @@ Notice that using \\[next-error] or \\[compile-goto-error] modifies
>  (defvar grep-match-face	'match
>    "Face name to use for grep matches.")
>  
> +(defvar grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
> +  "Regular expression matching grep markers to highlight, used
> +in `grep-filter'.")

The first sentence of every docstring should fit on one line;
see (info "(elisp) Documentation Tips").  So you could move the mention
of grep-filter into a sentence of its own.

-- 
Basil

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Tue, 09 Jun 2020 12:55:25 +0100
> Cc: "41766@debbugs.gnu.org" <41766@debbugs.gnu.org>,
>  Dmitry Gutov <dgutov@yandex.ru>, Juri Linkov <juri@linkov.net>
> 
> I wonder, though: the default value matches some quite obscure codes
> which aren't (and maybe shouldn't be) documented, so is a defcustom
> really suitable for this?

They are SGR escape sequences that Grep emits to color its output.  We
should probably mention that in the doc string.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

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

Hi,

I now something to the manual, NEWS and changed the doc string + commit msg as advised. I decided on defcustom in the end because this way it is easy for the user to figure out what goes wrong in case he/she modifies grep-command and highlighting is missing. Hope that is fine.

Pls let me know if there is anything else I need to change.

Thanks!

________________________________________
From: Eli Zaretskii <eliz@gnu.org>
Sent: 09 June 2020 15:43
To: Basil L. Contovounesios
Cc: Simon.lang@outlook.com; 41766@debbugs.gnu.org; dgutov@yandex.ru; juri@linkov.net
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Tue, 09 Jun 2020 12:55:25 +0100
> Cc: "41766@debbugs.gnu.org" <41766@debbugs.gnu.org>,
>  Dmitry Gutov <dgutov@yandex.ru>, Juri Linkov <juri@linkov.net>
>
> I wonder, though: the default value matches some quite obscure codes
> which aren't (and maybe shouldn't be) documented, so is a defcustom
> really suitable for this?

They are SGR escape sequences that Grep emits to color its output.  We
should probably mention that in the doc string.

[-- Attachment #2: 0001-lisp-progmodes-grep.el-grep-match-regexp-New-variabl.patch --]
[-- Type: application/octet-stream, Size: 3140 bytes --]

From bcc839e7c47fafb66eac75623d758122b7e9fed4 Mon Sep 17 00:00:00 2001
From: Simon Lang <simon.lang@outlook.com>
Date: Wed, 10 Jun 2020 22:01:02 +0100
Subject: [PATCH] * lisp/progmodes/grep.el (grep-match-regexp): New variable.
 (grep-filter): Use it.

---
 doc/emacs/building.texi | 4 ++++
 etc/NEWS                | 8 ++++++++
 lisp/progmodes/grep.el  | 9 ++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/building.texi b/doc/emacs/building.texi
index 7074bd45d7..fb13738bc4 100644
--- a/doc/emacs/building.texi
+++ b/doc/emacs/building.texi
@@ -432,6 +432,10 @@ markers around matches for the purpose of highlighting.  You can make
 use of this feature by setting @code{grep-highlight-matches} to
 @code{t}.  When displaying a match in the source buffer, the exact
 match will be highlighted, instead of the entire source line.
+Highlighting is provided via matching of ANSI escape sequences emitted
+by grep.  The matching of the sequences is controlled by
+@code{grep-match-regexp}, which can be customize to accommodate
+different grep programs.
 
   As with compilation commands (@pxref{Compilation}), while the grep
 command runs, the mode line shows the running number of matches found
diff --git a/etc/NEWS b/etc/NEWS
index b0c523672e..125b7c7cec 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -176,6 +176,14 @@ was sent.  To restore the original behavior of dating a message
 from when it is first saved or delayed, add the symbol 'Date' back to
 this user option.
 
+** Grep changes:
+
+*** New variable 'grep-match-regexp' matches grep markers to highlight.
+Grep emits SGR ANSI escape sequences to color its output. The new variable
+'grep-match-regexp' holds the regular expression to match the appropriate
+markers in order to provide highlighting in the source buffer. The variable
+can be customized to accommodate other grep-like tools.
+
 ** Help
 
 +++
diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 7731be5965..32b7a597cf 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -100,6 +100,13 @@ To change the default value, use \\[customize] or call the function
   :set #'grep-apply-setting
   :version "22.1")
 
+(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
+  "Regular expression matching grep markers to highlight.
+It matches SGR ANSI escape sequences which are emitted by grep to
+color its output. This variable is used in `grep-filter'."
+  :type 'regexp
+  :version "28.1")
+
 (defcustom grep-scroll-output nil
   "Non-nil to scroll the *grep* buffer window as output appears.
 
@@ -584,7 +591,7 @@ This function is called from `compilation-filter-hook'."
       (when (< (point) end)
         (setq end (copy-marker end))
         ;; Highlight grep matches and delete marking sequences.
-        (while (re-search-forward "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m" end 1)
+        (while (re-search-forward grep-match-regexp end 1)
           (replace-match (propertize (match-string 1)
                                      'face nil 'font-lock-face grep-match-face)
                          t t)
-- 
2.21.0 (Apple Git-122)

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Juri Linkov]

> I now something to the manual, NEWS and changed the doc string + commit msg
> as advised. I decided on defcustom in the end because this way it is easy
> for the user to figure out what goes wrong in case he/she modifies
> grep-command and highlighting is missing. Hope that is fine.

In the long run what should be customizable is not escape sequences,
but a choice of the grep program with a list of such options:

  "GNU grep"
  "ripgrep"
  ...

i.e. the same customization as for the selection of the web browser
in browse-url.el.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Dmitry Gutov]

On 11.06.2020 00:52, Juri Linkov wrote:
> In the long run what should be customizable is not escape sequences,
> but a choice of the grep program with a list of such options:
> 
>    "GNU grep"
>    "ripgrep"
>    ...

That could simply be done by adding :tag to each option value, right?

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Juri Linkov]

>> In the long run what should be customizable is not escape sequences,
>> but a choice of the grep program with a list of such options:
>>    "GNU grep"
>>    "ripgrep"
>>    ...
>
> That could simply be done by adding :tag to each option value, right?

Indeed, the same way as it's already used in

(defcustom grep-find-use-xargs nil
  "How to invoke find and grep.
If `exec', use `find -exec {} ;'.
If `exec-plus' use `find -exec {} +'.
If `gnu', use `find -print0' and `xargs -0'.
If `gnu-sort', use `find -print0', `sort -z' and `xargs -0'.
Any other value means to use `find -print' and `xargs'.

This variable's value takes effect when `grep-compute-defaults' is called."
  :type '(choice (const :tag "find -exec {} ;" exec)
                 (const :tag "find -exec {} +" exec-plus)
                 (const :tag "find -print0 | xargs -0" gnu)
                 (const :tag "find -print0 | sort -z | xargs -0'" gnu-sort)
                 string
		 (const :tag "Not Set" nil))
  :set #'grep-apply-setting
  :version "27.1")

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Dmitry Gutov]

On 11.06.2020 02:10, Juri Linkov wrote:
>>> In the long run what should be customizable is not escape sequences,
>>> but a choice of the grep program with a list of such options:
>>>     "GNU grep"
>>>     "ripgrep"
>>>     ...
>> That could simply be done by adding :tag to each option value, right?
> Indeed, the same way as it's already used in

Then the main thing missing is the alternative value(s).

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Eli Zaretskii]

> From: Simon Lang <Simon.lang@outlook.com>
> CC: "41766@debbugs.gnu.org" <41766@debbugs.gnu.org>, "dgutov@yandex.ru"
> 	<dgutov@yandex.ru>, "juri@linkov.net" <juri@linkov.net>
> Date: Wed, 10 Jun 2020 21:11:09 +0000
> 
> I now something to the manual, NEWS and changed the doc string + commit msg as advised. I decided on defcustom in the end because this way it is easy for the user to figure out what goes wrong in case he/she modifies grep-command and highlighting is missing. Hope that is fine.
> 
> Pls let me know if there is anything else I need to change.

Thanks, a few minor comments below.

> +by grep.  The matching of the sequences is controlled by
> +@code{grep-match-regexp}, which can be customize to accommodate
> +different grep programs.               ^^^^^^^^^

Typo: should be "customized".

Also, "grep" should be "Grep".

> +** Grep changes:
> +
> +*** New variable 'grep-match-regexp' matches grep markers to highlight.
> +Grep emits SGR ANSI escape sequences to color its output. The new variable
> +'grep-match-regexp' holds the regular expression to match the appropriate
> +markers in order to provide highlighting in the source buffer. The variable
> +can be customized to accommodate other grep-like tools.

Please leave 2 spaces between sentences, per our conventions.
> +(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
> +  "Regular expression matching grep markers to highlight.
> +It matches SGR ANSI escape sequences which are emitted by grep to
> +color its output. This variable is used in `grep-filter'."

Likewise here.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

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

Thank you!

Attached pls find the revised patch.

Simon

________________________________________
From: Eli Zaretskii <eliz@gnu.org>
Sent: 13 June 2020 07:50
To: Simon Lang
Cc: contovob@tcd.ie; 41766@debbugs.gnu.org; dgutov@yandex.ru; juri@linkov.net
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

> From: Simon Lang <Simon.lang@outlook.com>
> CC: "41766@debbugs.gnu.org" <41766@debbugs.gnu.org>, "dgutov@yandex.ru"
>       <dgutov@yandex.ru>, "juri@linkov.net" <juri@linkov.net>
> Date: Wed, 10 Jun 2020 21:11:09 +0000
>
> I now something to the manual, NEWS and changed the doc string + commit msg as advised. I decided on defcustom in the end because this way it is easy for the user to figure out what goes wrong in case he/she modifies grep-command and highlighting is missing. Hope that is fine.
>
> Pls let me know if there is anything else I need to change.

Thanks, a few minor comments below.

> +by grep.  The matching of the sequences is controlled by
> +@code{grep-match-regexp}, which can be customize to accommodate
> +different grep programs.               ^^^^^^^^^

Typo: should be "customized".

Also, "grep" should be "Grep".

> +** Grep changes:
> +
> +*** New variable 'grep-match-regexp' matches grep markers to highlight.
> +Grep emits SGR ANSI escape sequences to color its output. The new variable
> +'grep-match-regexp' holds the regular expression to match the appropriate
> +markers in order to provide highlighting in the source buffer. The variable
> +can be customized to accommodate other grep-like tools.

Please leave 2 spaces between sentences, per our conventions.
> +(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
> +  "Regular expression matching grep markers to highlight.
> +It matches SGR ANSI escape sequences which are emitted by grep to
> +color its output. This variable is used in `grep-filter'."

Likewise here.

[-- Attachment #2: 0001-lisp-progmodes-grep.el-grep-match-regexp-New-variabl.patch --]
[-- Type: application/octet-stream, Size: 3144 bytes --]

From 03a375fa07b0c971069557c4cd204d8e284f302d Mon Sep 17 00:00:00 2001
From: Simon Lang <simon.lang@outlook.com>
Date: Wed, 10 Jun 2020 22:01:02 +0100
Subject: [PATCH] * lisp/progmodes/grep.el (grep-match-regexp): New variable.
 (grep-filter): Use it.

---
 doc/emacs/building.texi | 4 ++++
 etc/NEWS                | 8 ++++++++
 lisp/progmodes/grep.el  | 9 ++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/building.texi b/doc/emacs/building.texi
index 7074bd45d7..96af88b861 100644
--- a/doc/emacs/building.texi
+++ b/doc/emacs/building.texi
@@ -432,6 +432,10 @@ markers around matches for the purpose of highlighting.  You can make
 use of this feature by setting @code{grep-highlight-matches} to
 @code{t}.  When displaying a match in the source buffer, the exact
 match will be highlighted, instead of the entire source line.
+Highlighting is provided via matching of ANSI escape sequences emitted
+by Grep.  The matching of the sequences is controlled by
+@code{grep-match-regexp}, which can be customized to accommodate
+different grep programs.
 
   As with compilation commands (@pxref{Compilation}), while the grep
 command runs, the mode line shows the running number of matches found
diff --git a/etc/NEWS b/etc/NEWS
index b0c523672e..d2366fa7c4 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -176,6 +176,14 @@ was sent.  To restore the original behavior of dating a message
 from when it is first saved or delayed, add the symbol 'Date' back to
 this user option.
 
+** Grep changes:
+
+*** New variable 'grep-match-regexp' matches grep markers to highlight.
+Grep emits SGR ANSI escape sequences to color its output.  The new variable
+'grep-match-regexp' holds the regular expression to match the appropriate
+markers in order to provide highlighting in the source buffer.  The variable
+can be customized to accommodate other grep-like tools.
+
 ** Help
 
 +++
diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 7731be5965..8502744a8e 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -100,6 +100,13 @@ To change the default value, use \\[customize] or call the function
   :set #'grep-apply-setting
   :version "22.1")
 
+(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
+  "Regular expression matching grep markers to highlight.
+It matches SGR ANSI escape sequences which are emitted by grep to
+color its output.  This variable is used in `grep-filter'."
+  :type 'regexp
+  :version "28.1")
+
 (defcustom grep-scroll-output nil
   "Non-nil to scroll the *grep* buffer window as output appears.
 
@@ -584,7 +591,7 @@ This function is called from `compilation-filter-hook'."
       (when (< (point) end)
         (setq end (copy-marker end))
         ;; Highlight grep matches and delete marking sequences.
-        (while (re-search-forward "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m" end 1)
+        (while (re-search-forward grep-match-regexp end 1)
           (replace-match (propertize (match-string 1)
                                      'face nil 'font-lock-face grep-match-face)
                          t t)
-- 
2.21.0 (Apple Git-122)

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

Maybe also a way to easily register new search tools? Otherwise one might be locked into the available options again - and if there is a new tool the interface might be not that stable, so there is the danger that it breaks until there is a new emacs release.

Simon

________________________________________
From: Juri Linkov <juri@linkov.net>
Sent: 11 June 2020 00:10
To: Dmitry Gutov
Cc: Simon Lang; Basil L. Contovounesios; 41766@debbugs.gnu.org
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

>> In the long run what should be customizable is not escape sequences,
>> but a choice of the grep program with a list of such options:
>>    "GNU grep"
>>    "ripgrep"
>>    ...
>
> That could simply be done by adding :tag to each option value, right?

Indeed, the same way as it's already used in

(defcustom grep-find-use-xargs nil
  "How to invoke find and grep.
If `exec', use `find -exec {} ;'.
If `exec-plus' use `find -exec {} +'.
If `gnu', use `find -print0' and `xargs -0'.
If `gnu-sort', use `find -print0', `sort -z' and `xargs -0'.
Any other value means to use `find -print' and `xargs'.

This variable's value takes effect when `grep-compute-defaults' is called."
  :type '(choice (const :tag "find -exec {} ;" exec)
                 (const :tag "find -exec {} +" exec-plus)
                 (const :tag "find -print0 | xargs -0" gnu)
                 (const :tag "find -print0 | sort -z | xargs -0'" gnu-sort)
                 string
                 (const :tag "Not Set" nil))
  :set #'grep-apply-setting
  :version "27.1")

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Juri Linkov]

> Maybe also a way to easily register new search tools? Otherwise one might
> be locked into the available options again - and if there is a new tool the
> interface might be not that stable, so there is the danger that it breaks
> until there is a new emacs release.

No problem, you can even dynamically add an option available only
when a grep program is installed:

(defcustom grep-program nil
  "The default grep program for `grep-command' and `grep-find-command'.
This variable's value takes effect when `grep-compute-defaults' is called."
  :type `(choice (const :tag "GNU grep" (purecopy "grep"))
                 ,@(if (executable-find "rg") '((const :tag "ripgrep" "rg")))
                 (string :tag "Other grep program")
                 (const :tag "Not Set" nil))
  :version "28.1")

Or for a completely new tool:

(nconc (get 'grep-program 'custom-type) '((const :tag "ripgrep" "rg")))

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

> Am 13.06.2020 um 23:59 schrieb Juri Linkov <juri@linkov.net>:
> 
> 
>> 
>> Maybe also a way to easily register new search tools? Otherwise one might
>> be locked into the available options again - and if there is a new tool the
>> interface might be not that stable, so there is the danger that it breaks
>> until there is a new emacs release.
> 
> No problem, you can even dynamically add an option available only
> when a grep program is installed:
> 
> (defcustom grep-program nil
>  "The default grep program for `grep-command' and `grep-find-command'.
> This variable's value takes effect when `grep-compute-defaults' is called."
>  :type `(choice (const :tag "GNU grep" (purecopy "grep"))
>                 ,@(if (executable-find "rg") '((const :tag "ripgrep" "rg")))
>                 (string :tag "Other grep program")
>                 (const :tag "Not Set" nil))
>  :version "28.1")
> 
> Or for a completely new tool:
> 
> (nconc (get 'grep-program 'custom-type) '((const :tag "ripgrep" "rg")))

Maybe still fine to merge my patch now and look at this later? One would not contradict the other, I would think? 

@Eli: sorry I hope my revised patch was not buried below this other discussion now. 

Thank you! 

Simon

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Juri Linkov]

>> Or for a completely new tool:
>>
>> (nconc (get 'grep-program 'custom-type) '((const :tag "ripgrep" "rg")))
>
> Maybe still fine to merge my patch now and look at this later?
> One would not contradict the other, I would think?

Yes, your patch is a good starting point, thanks.

A minor worry is that you put back defcustom instead of leaving defvar.
I can't imagine a user who might want to customize it using literal
escape characters in the customization input field.

Maybe this is fine for now, but in the long run it would be better to
allow an alist of mappings from the grep program name to its escape sequences
like

(defvar grep-match-regexp
 '(("grep" . "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m")
   ("ripgrep" . ""\033\\[[0-9]*m\033\\[[0-9]*1m\033\\[[0-9]*1m\\(.*?\\)\033\\[[0-9]*0m"")))

Then you can easily switch between grep and ripgrep without losing
information about their escape sequences.

But of course this could be improved after installing your current patch.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Simon Lang]

I changed it to defcustom because this is how I currently use it and no one had strong feelings about it. 
Do you want me to change it to devfar again? Or is it fine as it is? 

Thanks,

Simon

________________________________________
From: Juri Linkov <juri@linkov.net>
Sent: 15 June 2020 00:08
To: Simon Lang
Cc: Simon Lang; Dmitry Gutov; Basil L. Contovounesios; 41766@debbugs.gnu.org
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

>> Or for a completely new tool:
>>
>> (nconc (get 'grep-program 'custom-type) '((const :tag "ripgrep" "rg")))
>
> Maybe still fine to merge my patch now and look at this later?
> One would not contradict the other, I would think?

Yes, your patch is a good starting point, thanks.

A minor worry is that you put back defcustom instead of leaving defvar.
I can't imagine a user who might want to customize it using literal
escape characters in the customization input field.

Maybe this is fine for now, but in the long run it would be better to
allow an alist of mappings from the grep program name to its escape sequences
like

(defvar grep-match-regexp
 '(("grep" . "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m")
   ("ripgrep" . ""\033\\[[0-9]*m\033\\[[0-9]*1m\033\\[[0-9]*1m\\(.*?\\)\033\\[[0-9]*0m"")))

Then you can easily switch between grep and ripgrep without losing
information about their escape sequences.

But of course this could be improved after installing your current patch.

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Juri Linkov]

> I changed it to defcustom because this is how I currently use it and
> no one had strong feelings about it.
> Do you want me to change it to devfar again? Or is it fine as it is?

If you don't want to change it to devfar in your patch, then for users
there should be an easy way to revert their customization to the default
value.  This would be possible by using such menu of possible values:

(defcustom grep-match-regexp "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m"
  "Regular expression matching grep markers to highlight.
It matches SGR ANSI escape sequences which are emitted by grep to
color its output.  This variable is used in `grep-filter'."
  :type '(choice (regexp :tag "GNU grep" "\033\\[0?1;31m\\(.*?\\)\033\\[[0-9]*m")
		 (regexp :tag "ripgrep" "\033\\[[0-9]*m\033\\[[0-9]*1m\033\\[[0-9]*1m\\(.*?\\)\033\\[[0-9]*0m")
                 (regexp :tag "Other grep programs"))
  :version "28.1")

bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

[Lars Ingebrigtsen]

Simon Lang <Simon.lang@outlook.com> writes:

> Attached pls find the revised patch.

Thanks.  It looks like everybody agreed that this was a good change, but
then nobody applied your patch, so I did that now (to Emacs 28).

There was then some followup discussion about possible tweaks, and those
can now be done, and I'm closing this bug report.

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