bug#9438: grep regressions

bug#9438: grep regressions

[Juri Linkov]

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

bug#9438: grep regressions

[Stefan Monnier]

> 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

bug#9438: grep regressions

[Juri Linkov]

>> +	(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))

bug#9438: grep regressions

[Juri Linkov]

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