GNU bug report logs - #9438
grep regressions

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9438 in the body.
You can then email your comments to 9438 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: bug-gnu-emacs <at> gnu.org
Subject: grep regressions
Date: Mon, 05 Sep 2011 11:34:04 +0300

grep used to navigate directly to the correct column and highlight the
match instantly with `next-error-highlight'.  But now grep.el doesn't do
that because escape sequences are processed earlier than font-locking.

One way to fix this regression is to find matches highligted by
`grep-filter' and take their column positions.

Unfortunately, I can't find a simpler way to fix it than this patch:

=== modified file 'lisp/progmodes/grep.el'
--- lisp/progmodes/grep.el	2011-08-22 09:54:38 +0000
+++ lisp/progmodes/grep.el	2011-09-05 08:30:35 +0000
@@ -344,7 +344,27 @@ (defvar grep-last-buffer nil
 ;;;###autoload
 (defconst grep-regexp-alist
   '(("^\\(.+?\\)\\(:[ \t]*\\)\\([1-9][0-9]*\\)\\2"
-     1 3)
+     1 3
+     ;; Calculate column positions (col . end-col) of first grep match on a line
+     ((lambda ()
+	(when grep-highlight-matches
+	  (setq compilation-error-screen-columns nil)
+	  (save-excursion
+	    (let* ((beg (progn (goto-char (match-end 0)) (point)))
+		   (end (line-end-position))
+		   (mbeg (text-property-any beg end 'font-lock-face 'match)))
+	      (when mbeg
+		(- mbeg beg))))))
+      .
+      (lambda ()
+	(when grep-highlight-matches
+	  (save-excursion
+	    (let* ((beg (progn (goto-char (match-end 0)) (point)))
+		   (end (line-end-position))
+		   (mbeg (text-property-any beg end 'font-lock-face 'match))
+		   (mend (and mbeg (next-single-property-change mbeg 'font-lock-face nil end))))
+	      (when mend
+		(- mend beg))))))))
     ;; Rule to match column numbers is commented out since no known grep
     ;; produces them
     ;; ("^\\(.+?\\)\\(:[ \t]*\\)\\([0-9]+\\)\\2\\(?:\\([0-9]+\\)\\(?:-\\([0-9]+\\)\\)?\\2\\)?"
@@ -353,17 +373,6 @@ (defconst grep-regexp-alist
     ;; handle weird file names (with colons in them) as well as possible.
     ;; E.g. we use [1-9][0-9]* rather than [0-9]+ so as to accept ":034:" in
     ;; file names.
-    ("^\\(\\(.+?\\):\\([1-9][0-9]*\\):\\).*?\
-\\(\033\\[01;31m\\(?:\033\\[K\\)?\\)\\(.*?\\)\\(\033\\[[0-9]*m\\)"
-     2 3
-     ;; Calculate column positions (beg . end) of first grep match on a line
-     ((lambda ()
-	(setq compilation-error-screen-columns nil)
-        (- (match-beginning 4) (match-end 1)))
-      .
-      (lambda () (- (match-end 5) (match-end 1)
-               (- (match-end 4) (match-beginning 4)))))
-     nil 1)
     ("^Binary file \\(.+\\) matches$" 1 nil nil 0 1))
   "Regexp used to match grep hits.  See `compilation-error-regexp-alist'.")
 

Message #8 received at 9438 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 9438 <at> debbugs.gnu.org
Subject: Re: bug#9438: grep regressions
Date: Tue, 06 Sep 2011 21:22:30 -0400

> Unfortunately, I can't find a simpler way to fix it than this patch:

It doesn't look too terrible.

> +     ;; Calculate column positions (col . end-col) of first grep match on a line
> +     ((lambda ()
> +	(when grep-highlight-matches
> +	  (setq compilation-error-screen-columns nil)

Why set it here?  I know it used to be set similarly, but I think it
should be set once and for all in grep-mode instead.

> +	  (save-excursion
> +	    (let* ((beg (progn (goto-char (match-end 0)) (point)))

Why not

   	  (save-excursion
            (goto-char (match-end 0))
            (let* ((beg (point))

Or better yet:

         (let* ((beg (match-end 0))
                (end (save-excursion (goto-char beg) (line-end-position)))


-- Stefan

Message #11 received at 9438 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 9438 <at> debbugs.gnu.org
Subject: Re: bug#9438: grep regressions
Date: Wed, 07 Sep 2011 15:09:24 +0300

>> +	(when grep-highlight-matches
>> +	  (setq compilation-error-screen-columns nil)
>
> Why set it here?  I know it used to be set similarly, but I think it
> should be set once and for all in grep-mode instead.

I just discovered `grep-error-screen-columns', a defcustom
added in 2004.  Its default value is nil, so I suppose
its purpose was to override `compilation-error-screen-columns'.
It's currently unused because it misses the following lines
in `grep-mode':

  (set (make-local-variable 'compilation-error-screen-columns)
       grep-error-screen-columns)

I added that to `grep-mode'.

> Or better yet:
>
>          (let* ((beg (match-end 0))
>                 (end (save-excursion (goto-char beg) (line-end-position)))

Yes, this is better.  I installed these changes.

Beside `grep-error-screen-columns', I discovered two more unused defcustoms
in grep.el:

  (defcustom grep-window-height nil
    "*Number of lines in a grep window.  If nil, use `compilation-window-height'."
    :type '(choice (const :tag "Default" nil)
                   integer)
    :version "22.1"
    :group 'grep)

  (defcustom grep-scroll-output nil
    "*Non-nil to scroll the *grep* buffer window as output appears.
  Setting it causes the grep commands to put point at the end of their
  output window so that the end of the output is always visible rather
  than the begining."
    :type 'boolean
    :version "22.1"
    :group 'grep)

IIUC, since the docstring says "If nil, use `compilation-window-height'"
they should be used in `grep-mode' only when the value is non-nil like:

  (when grep-window-height
    (set (make-local-variable 'compilation-window-height)
         grep-window-height))
  (when grep-scroll-output
    (set (make-local-variable 'compilation-scroll-output)
         grep-scroll-output))

Message #14 received at 9438 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 9438 <at> debbugs.gnu.org
Subject: Re: bug#9438: grep regressions
Date: Thu, 08 Sep 2011 23:18:05 +0300

> Beside `grep-error-screen-columns', I discovered two more unused defcustoms
> in grep.el:
>
>   (defcustom grep-window-height nil
>     "*Number of lines in a grep window.  If nil, use `compilation-window-height'."
>     :type '(choice (const :tag "Default" nil)
>                    integer)
>     :version "22.1"
>     :group 'grep)
>
>   (defcustom grep-scroll-output nil
>     "*Non-nil to scroll the *grep* buffer window as output appears.
>   Setting it causes the grep commands to put point at the end of their
>   output window so that the end of the output is always visible rather
>   than the begining."
>     :type 'boolean
>     :version "22.1"
>     :group 'grep)
>
> IIUC, since the docstring says "If nil, use `compilation-window-height'"
> they should be used in `grep-mode' only when the value is non-nil like:
>
>   (when grep-window-height
>     (set (make-local-variable 'compilation-window-height)
>          grep-window-height))
>   (when grep-scroll-output
>     (set (make-local-variable 'compilation-scroll-output)
>          grep-scroll-output))

According to bzr logs, before revno:54335 this used to be:

  (if grep-window-height
      (set (make-local-variable 'compilation-window-height)
	   grep-window-height))
  (set (make-local-variable 'compilation-scroll-output)
       grep-scroll-output)

So either we should restore this code in `grep-process-setup' to use
the customized values of defcustoms, or remove unused defcustoms.
I have no settled opinion what would be better to do.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.