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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Simon Lang
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.



0001-Make-regexp-used-to-highlight-grep-matches-customiza.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Dmitry Gutov
In reply to this post by Simon Lang
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?



Reply | Threaded
Open this post in threaded view
|

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

Simon Lang
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 <[hidden email]> on behalf of Dmitry Gutov <[hidden email]>
Sent: 09 June 2020 01:44
To: Simon Lang; [hidden email]
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?

0001-Make-regexp-used-to-highlight-grep-matches-customiza.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Simon Lang <[hidden email]> writes:

> From bc9b736ff20a03e831bc5110283ccf9241127773 Mon Sep 17 00:00:00 2001
> From: Simon Lang <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Dmitry Gutov
In reply to this post by Simon Lang
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.



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> on behalf of Dmitry Gutov <[hidden email]>
Sent: 09 June 2020 13:15
To: Simon Lang; [hidden email]; 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.



Reply | Threaded
Open this post in threaded view
|

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

Simon Lang
In reply to this post by Basil L. Contovounesios
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 <[hidden email]>
Sent: 09 June 2020 12:55
To: Simon Lang
Cc: Dmitry Gutov; [hidden email]; Juri Linkov
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

Simon Lang <[hidden email]> writes:

> From bc9b736ff20a03e831bc5110283ccf9241127773 Mon Sep 17 00:00:00 2001
> From: Simon Lang <[hidden email]>
> 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

0001-Remove-hardcoding-of-regexp-used-to-highlight-grep-m.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Simon Lang
> From: Simon Lang <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
In reply to this post by Simon Lang
Simon Lang <[hidden email]> 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 <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Basil L. Contovounesios
> From: "Basil L. Contovounesios" <[hidden email]>
> Date: Tue, 09 Jun 2020 12:55:25 +0100
> Cc: "[hidden email]" <[hidden email]>,
>  Dmitry Gutov <[hidden email]>, Juri Linkov <[hidden email]>
>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Simon Lang
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 <[hidden email]>
Sent: 09 June 2020 15:43
To: Basil L. Contovounesios
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

> From: "Basil L. Contovounesios" <[hidden email]>
> Date: Tue, 09 Jun 2020 12:55:25 +0100
> Cc: "[hidden email]" <[hidden email]>,
>  Dmitry Gutov <[hidden email]>, Juri Linkov <[hidden email]>
>
> 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.

0001-lisp-progmodes-grep.el-grep-match-regexp-New-variabl.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
> 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.



Reply | Threaded
Open this post in threaded view
|

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?



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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")



Reply | Threaded
Open this post in threaded view
|

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).



Reply | Threaded
Open this post in threaded view
|

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

Simon Lang
In reply to this post by Simon Lang
Thank you!

Attached pls find the revised patch.

Simon

________________________________________
From: Eli Zaretskii <[hidden email]>
Sent: 13 June 2020 07:50
To: Simon Lang
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
Subject: Re: bug#41766: Make it possible to change regexp to identify and highlight grep matches via customization

> From: Simon Lang <[hidden email]>
> CC: "[hidden email]" <[hidden email]>, "[hidden email]"
>       <[hidden email]>, "[hidden email]" <[hidden email]>
> 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.

0001-lisp-progmodes-grep.el-grep-match-regexp-New-variabl.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Simon Lang
In reply to this post by Juri Linkov-2
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 <[hidden email]>
Sent: 11 June 2020 00:10
To: Dmitry Gutov
Cc: Simon Lang; Basil L. Contovounesios; [hidden email]
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")



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
> 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")))



Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
In reply to this post by Simon Lang
Simon Lang <[hidden email]> 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