unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
@ 2011-10-06  6:14 ` Ari Roponen
  2011-10-06 15:27   ` Juri Linkov
       [not found]   ` <handler.9679.C.13363577086087.notifdonectrl.0@debbugs.gnu.org>
  0 siblings, 2 replies; 13+ messages in thread
From: Ari Roponen @ 2011-10-06  6:14 UTC (permalink / raw)
  To: 9679


Sometimes next-error goes to the wrong line when stepping through rgrep
matches.

Using git bisect and the code below, I found the commit that causes the
problem (rev 105677). Reverting that (and before that a commit that
depended on it) makes the problem disappear.

rev 105685: (grep-regexp-alist): Move dangling comment to the previous rule.
rev 105677: progmodes/grep.el (grep-regexp-alist): Calculate column positions

I used the following test case to find the problematic commit. It
creates a file with two lines matching "mapcar" and then rgreps for it.
After that it calls next-error two times. The second call goes into the
line starting with " (list 'face ...".
                          ^- here

--- code begins ----

(let ((dir (expand-file-name
	    (format "tmp%d" (emacs-pid))
	    temporary-file-directory)))
  (ignore-errors (make-directory dir))

  (with-temp-file (expand-file-name "test.el" dir)
    (insert
"(defun my-diary-lib--date-to-string (date)
  (destructuring-bind (month day year)
      (mapcar 'number-to-string date)
    (concat day \".\" month \".\" year)))

\(defun my-diary-lib--string-to-date (datestr)
  (destructuring-bind (day month year)
      (mapcar #'string-to-number (split-string datestr \"\\\\.\"))
    (list month day year)))

\(defun my-diary-lib-add-dayname (datestr)
  (list 'face 'font-lock-comment-face
	'display (concat datestr \" \" (calendar-day-name
					(my-diary-lib--string-to-date datestr)))))
"))

  (grep-compute-defaults)
  (rgrep "mapcar" "test.el" dir)
  (run-with-idle-timer 0.5 nil #'next-error 1)
  (run-with-idle-timer 1.0 nil #'next-error 1))

--- code ends ---


In GNU Emacs 24.0.90.7 (x86_64-unknown-linux-gnu, GTK+ Version 3.2.0)
 of 2011-10-06 on arirop
Windowing system distributor `Fedora Project', version 11.0.11000000
configured using `configure  '--with-x-toolkit=gtk3''

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-06  6:14 ` bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line Ari Roponen
@ 2011-10-06 15:27   ` Juri Linkov
  2011-10-07  5:15     ` Ari Roponen
       [not found]   ` <handler.9679.C.13363577086087.notifdonectrl.0@debbugs.gnu.org>
  1 sibling, 1 reply; 13+ messages in thread
From: Juri Linkov @ 2011-10-06 15:27 UTC (permalink / raw)
  To: Ari Roponen; +Cc: 9679

> Sometimes next-error goes to the wrong line when stepping through rgrep
> matches.
>
> Using git bisect and the code below, I found the commit that causes the
> problem (rev 105677). Reverting that (and before that a commit that
> depended on it) makes the problem disappear.
>
> rev 105685: (grep-regexp-alist): Move dangling comment to the previous rule.
> rev 105677: progmodes/grep.el (grep-regexp-alist): Calculate column positions
>
> I used the following test case to find the problematic commit. It
> creates a file with two lines matching "mapcar" and then rgreps for it.
> After that it calls next-error two times. The second call goes into the
> line starting with " (list 'face ...".
>                           ^- here

Thanks for the comprehensive test case.  I tried it but the second call
goes into the line starting with "(mapcar #'string-to-number ...".





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-06 15:27   ` Juri Linkov
@ 2011-10-07  5:15     ` Ari Roponen
  2011-10-07  8:41       ` Ari Roponen
  2011-10-07 15:20       ` Juri Linkov
  0 siblings, 2 replies; 13+ messages in thread
From: Ari Roponen @ 2011-10-07  5:15 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 9679

Juri Linkov <juri@jurta.org> writes:

> Thanks for the comprehensive test case.  I tried it but the second call
> goes into the line starting with "(mapcar #'string-to-number ...".

Hi,

I compared the behavior between the version with commits reverted and
not reverted.

In the reverted commits case, typing M-g M-p and M-g M-n goes to the
correct line and column, but highlights the whole line.

In the non-reverted commits case, typing those commands goes to the
wrong line but right column. The highlight seems to start at the end of
correct match and end at the cursor position.

I found something that may be relevant to the problem. These are the
values of *grep*-buffer's variable compilation-locs in both cases:

* Reverted commits case:

Value: #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
	      (
	       ("./test.el")
	       (("./test.el" nil)
		nil
		(8
		 (nil 8 #1 #<marker at 251 in test.el> nil . t))
		(3
		 (nil 3 #1 #<marker at 89 in test.el> nil . t)))))

* Non-reverted commits case:

Value: #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
	      (("Grep started at Fri Oct  7 07")
	       (("Grep started at Fri Oct  7 07" nil)
		nil
		(31
		 (nil 31 #1 nil nil)))
	       ("./test.el")
	       (("./test.el" nil)
		nil
		(8
		 (13 8 #1 #<marker at 258 in test.el> nil)
		 (7 8 #1 #<marker at 387 in test.el> nil . t))
		(3
		 (13 3 #1 #<marker at 96 in test.el> nil)
		 (7 3 #1 #<marker at 90 in test.el> nil . t)))
	       ("Grep finished (matches found) at Fri Oct  7 07")
	       (("Grep finished (matches found) at Fri Oct  7 07" nil)
		nil
		(31
		 (nil 31 #1 nil nil)))))

In the non-reverted commits case, The marker pairs seem to delimit the
matched text. The first pair (from 258 to 387) starts at the end of
correct match and goes too far. The second pair (from 90 to 96)
highlight correctly the first match.

I noticed that killing the buffer the markers use seems to fix the
problem:

1. Run the test case from the original bug report.
2. Kill the "test.el"-buffer
3. Type M-g M-p M-g M-n

Now the cursor is at the beginning of the match and the match is
highlighted correctly. The value of compilation-locs in *grep*-buffer is
also correct:

Value: #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
	      (("Grep started at Fri Oct  7 07")
	       (("Grep started at Fri Oct  7 07" nil)
		nil
		(56
		 (nil 56 #1 nil nil)))
	       ("./test.el")
	       (("./test.el" nil)
		nil
		(8
		 (13 8 #1 #<marker at 258 in test.el> nil)
		 (7 8 #1 #<marker at 252 in test.el> nil . t))
		(3
		 (13 3 #1 #<marker at 96 in test.el> nil)
		 (7 3 #1 #<marker at 90 in test.el> nil . t)))
	       ("Grep finished (matches found) at Fri Oct  7 07")
	       (("Grep finished (matches found) at Fri Oct  7 07" nil)
		nil
		(56
		 (nil 56 #1 nil nil)))))

I guess the problem has something to do with those markers.

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-07  5:15     ` Ari Roponen
@ 2011-10-07  8:41       ` Ari Roponen
  2011-10-07 15:20       ` Juri Linkov
  1 sibling, 0 replies; 13+ messages in thread
From: Ari Roponen @ 2011-10-07  8:41 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 9679

Ari Roponen <ari.roponen@gmail.com> writes:

> I guess the problem has something to do with those markers.

I found a way to fix the problem locally without reverting any commits.

The function `compilation-next-error-function' contains this:

    ;; If loc contains no marker, no error in that file has been visited.
    ;; If the marker is invalid the buffer has been killed.
    ;; So, recalculate all markers for that file.
    (unless (and (compilation--loc->marker loc)
                 (marker-buffer (compilation--loc->marker loc))
                 ;; FIXME-omake: For "omake -P", which automatically recompiles
                 ;; when the file is modified, the line numbers of new output
                 ;; may not be related to line numbers from earlier output

When I add "nil" after "and", the problem goes away.

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-07  5:15     ` Ari Roponen
  2011-10-07  8:41       ` Ari Roponen
@ 2011-10-07 15:20       ` Juri Linkov
  2011-10-07 15:51         ` Ari Roponen
  1 sibling, 1 reply; 13+ messages in thread
From: Juri Linkov @ 2011-10-07 15:20 UTC (permalink / raw)
  To: Ari Roponen; +Cc: 9679

> 	       ("./test.el")
> 	       (("./test.el" nil)
> 		nil
> 		(8
> 		 (13 8 #1 #<marker at 258 in test.el> nil)
> 		 (7 8 #1 #<marker at 387 in test.el> nil . t))
> 		(3
> 		 (13 3 #1 #<marker at 96 in test.el> nil)
> 		 (7 3 #1 #<marker at 90 in test.el> nil . t)))

This is odd, I have in the non-reverted commits case
after visiting the second match:

  #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
   (("./test.el")
    (("./test.el" nil)
     nil
     (8
      (13 8 #1 #<marker at 258 in test.el> nil)
      (7 8 #1 #<marker at 252 in test.el> nil . t))
     (3
      (13 3 #1 #<marker at 96 in test.el> nil)
      (7 3 #1 #<marker at 90 in test.el> nil . t)))))

So unfortunately I can't reproduce this bug.

I suspect it may be related to screen columns because somehow
`compilation-move-to-column' in `compilation-next-error-function'
goes to the wrong line and column, i.e. to the position 387.
What values of `compilation-error-screen-columns' and
`grep-error-screen-columns' do you have in the *grep* buffer?
I have both of them equal to `nil'.

Could you please get the values of arguments that
`compilation-move-to-column' receives here by debugging
or adding (message "...") and seeing what values it prints
in the *Message* buffer?





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-07 15:20       ` Juri Linkov
@ 2011-10-07 15:51         ` Ari Roponen
  2011-10-07 16:36           ` Juri Linkov
  0 siblings, 1 reply; 13+ messages in thread
From: Ari Roponen @ 2011-10-07 15:51 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 9679

Juri Linkov <juri@jurta.org> writes:

> I suspect it may be related to screen columns because somehow
> `compilation-move-to-column' in `compilation-next-error-function'
> goes to the wrong line and column, i.e. to the position 387.
> What values of `compilation-error-screen-columns' and
> `grep-error-screen-columns' do you have in the *grep* buffer?
> I have both of them equal to `nil'.

I run the test case with "emacs -Q -l bug.el", so the variables
use their default values:

compilation-error-screen-columns is a variable defined in `compile.el'.
Its value is nil
Original value was t
Local in buffer *grep*; global value is t

grep-error-screen-columns is a variable defined in `grep.el'.
Its value is nil

>
> Could you please get the values of arguments that
> `compilation-move-to-column' receives here by debugging
> or adding (message "...") and seeing what values it prints
> in the *Message* buffer?

(defun compilation-move-to-column (col screen)
  "Go to column COL on the current line.
If SCREEN is non-nil, columns are screen columns, otherwise, they are
just char-counts."
  (message "col: %S, screen %S" col screen) ; Display parameters.
  (if screen
      (move-to-column (max col 0))
    (goto-char (min (+ (line-beginning-position) col) (line-end-position)))))

=>
Grep finished (matches found)
col: 13, screen nil
col: 7, screen nil
col: 13, screen t
col: 7, screen t

I get the same values also with the simpler "fix" that I mentioned in
the other post.

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-07 15:51         ` Ari Roponen
@ 2011-10-07 16:36           ` Juri Linkov
  2011-10-07 17:48             ` Ari Roponen
  0 siblings, 1 reply; 13+ messages in thread
From: Juri Linkov @ 2011-10-07 16:36 UTC (permalink / raw)
  To: Ari Roponen; +Cc: 9679

> col: 13, screen nil
> col: 7, screen nil
> col: 13, screen t
> col: 7, screen t

Something is completely broken in your test case, because I get:

col: 13, screen nil
col: 7, screen nil
col: 13, screen nil
col: 7, screen nil

i.e. `nil' is what's expected for the second match.

I don't understand how `t' is possible.  It looks like
`compilation-next-error-function' is not in `grep-mode' at that time.

Could you please add the following `message' to `compilation-next-error-function':

(defun compilation-next-error-function (n &optional reset)
  "Advance to the next error message and visit the file where the error was.
This is the value of `next-error-function' in Compilation buffers."
  (interactive "p")
  (when reset
    (setq compilation-current-error nil))
  (message "mode: %S, buffer: %S" major-mode (current-buffer))
  ...

I get the following output:

mode: grep-mode, buffer: #<buffer *grep*>
col: 13, screen nil
col: 7, screen nil
col: 13, screen nil
col: 7, screen nil
mode: grep-mode, buffer: #<buffer *grep*>





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-07 16:36           ` Juri Linkov
@ 2011-10-07 17:48             ` Ari Roponen
  2011-10-08 16:30               ` Juri Linkov
  0 siblings, 1 reply; 13+ messages in thread
From: Ari Roponen @ 2011-10-07 17:48 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 9679

Juri Linkov <juri@jurta.org> writes:

> I don't understand how `t' is possible.  It looks like
> `compilation-next-error-function' is not in `grep-mode' at that time.
>
> Could you please add the following `message' to `compilation-next-error-function':
>
> (defun compilation-next-error-function (n &optional reset)
>   "Advance to the next error message and visit the file where the error was.
> This is the value of `next-error-function' in Compilation buffers."
>   (interactive "p")
>   (when reset
>     (setq compilation-current-error nil))
>   (message "mode: %S, buffer: %S" major-mode (current-buffer))
>   ...
>
> I get the following output:
>
> mode: grep-mode, buffer: #<buffer *grep*>
> col: 13, screen nil
> col: 7, screen nil
> col: 13, screen nil
> col: 7, screen nil
> mode: grep-mode, buffer: #<buffer *grep*>

Grep finished (matches found)
mode: grep-mode, buffer: #<buffer *grep*>
col: 13, screen nil
col: 7, screen nil
col: 13, screen t
col: 7, screen t
mode: grep-mode, buffer: #<buffer *grep*>

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-07 17:48             ` Ari Roponen
@ 2011-10-08 16:30               ` Juri Linkov
  2011-10-09  5:38                 ` Ari Roponen
  0 siblings, 1 reply; 13+ messages in thread
From: Juri Linkov @ 2011-10-08 16:30 UTC (permalink / raw)
  To: Ari Roponen; +Cc: 9679

>> mode: grep-mode, buffer: #<buffer *grep*>
>> col: 13, screen nil
>> col: 7, screen nil
>> col: 13, screen nil
>> col: 7, screen nil
>> mode: grep-mode, buffer: #<buffer *grep*>
>
> Grep finished (matches found)
> mode: grep-mode, buffer: #<buffer *grep*>
> col: 13, screen nil
> col: 7, screen nil
> col: 13, screen t
> col: 7, screen t
> mode: grep-mode, buffer: #<buffer *grep*>

Thanks, I can now reproduce this bug.  Its occurrence depends on the
frame's size.  When frame's dimensions are 80x43 with 43 lines or more, so
the first grep hit is visible in the *grep* buffer, then font-lock kicks in,
calculates absolute column positions, after that `compilation-next-error-function'
calculates relative column positions with markers for BOTH grep hits,
where the final position of the second grep hit is correct.

But when frame's dimensions are 80x42 with 42 lines or less, so the first
grep hit is not visible in the *grep* buffer, then fontification doesn't
start, and when you call `next-error', `compilation-next-error-function'
calculates column positions with markers only for the FIRST grep hit,
because the second grep hit is not fontified yet in the *grep* buffer.

The second grep hit gets processes later with `jit-lock-fontify-now'
that calls `compilation-internal-error-properties' and uses
`compilation-move-to-column' to calculate columns.

The problem is that `compilation-internal-error-properties' calculates
column marker positions differently than `compilation-next-error-function'.
Its markers have wrong positions as a result.

`compilation-next-error-function' is always called in the *grep* buffer
where it gets the buffer-local value of `compilation-error-screen-columns'
and keeps its value locally.  After that it visits the source buffer and
correctly calculates column positions using `compilation-move-to-column'.

`compilation-internal-error-properties' tries to do the same,
but uses a different algorithm.  It gets the global value
of `compilation-error-screen-columns' instead of buffer-local
from the *grep* buffer.

Another difference that contributes to the wrong position
is using:
	    (beginning-of-line (if end-line
				   (- line end-line -1)
				 (- loc marker-line -1)))
Contrary to this, `compilation-next-error-function' doesn't
call `beginning-of-line' after `compilation-move-to-column'
moves to the correct column.

Unfortunately, I can't fix this, because I don't understand
why there are two implementations of column position calculations.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-08 16:30               ` Juri Linkov
@ 2011-10-09  5:38                 ` Ari Roponen
  2012-05-06  6:23                   ` Ari Roponen
  0 siblings, 1 reply; 13+ messages in thread
From: Ari Roponen @ 2011-10-09  5:38 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 9679

Juri Linkov <juri@jurta.org> writes:

> Thanks, I can now reproduce this bug.

Thank you for looking at this problem.

>
> Unfortunately, I can't fix this, because I don't understand
> why there are two implementations of column position calculations.

No worries. As they say, a known bug is better than an unknown
regression. :-)

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line
  2011-10-09  5:38                 ` Ari Roponen
@ 2012-05-06  6:23                   ` Ari Roponen
  0 siblings, 0 replies; 13+ messages in thread
From: Ari Roponen @ 2012-05-06  6:23 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 9679

Ari Roponen <ari.roponen@gmail.com> writes:

> Juri Linkov <juri@jurta.org> writes:
>
>> Thanks, I can now reproduce this bug.
>
> Thank you for looking at this problem.
>
>>
>> Unfortunately, I can't fix this, because I don't understand
>> why there are two implementations of column position calculations.
>
> No worries. As they say, a known bug is better than an unknown
> regression. :-)

Hi,

I can't reproduce this bug any more with todays checkout. It seems that
this commit fixed the bug:

revno: 108138
fixes bug(s): http://debbugs.gnu.org/11382
author: Troels Nielsen <bn.troels@gmail.com>
committer: Chong Yidong <cyd@gnu.org>
branch nick: trunk
timestamp: Sun 2012-05-06 12:52:58 +0800
message:
  Fix match highlighting in compilation buffers.
  
  * progmodes/compile.el (compilation-internal-error-properties):
  Calculate start position correctly when end-col is set but
  end-line is not.

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: acknowledged by developer (close 9679)
       [not found]   ` <handler.9679.C.13363577086087.notifdonectrl.0@debbugs.gnu.org>
@ 2012-05-13  6:39     ` Ari Roponen
  2012-05-13  9:18       ` Chong Yidong
  0 siblings, 1 reply; 13+ messages in thread
From: Ari Roponen @ 2012-05-13  6:39 UTC (permalink / raw)
  To: 9679


This bug ("24.0.90; After rgrep, next-error goes to the wrong line") was
fixed in trunk, but it still happens in emacs-24 branch.

Maybe the patch from the following revision could be backported/applied
into the emacs-24 branch.  I just tested it locally, and it indeed fixed
the problem there, too.


revno: 108138
fixes bug(s): http://debbugs.gnu.org/11382
author: Troels Nielsen <bn.troels@gmail.com>
committer: Chong Yidong <cyd@gnu.org>
branch nick: trunk
timestamp: Sun 2012-05-06 12:52:58 +0800
message:
  Fix match highlighting in compilation buffers.
  
  * progmodes/compile.el (compilation-internal-error-properties):
  Calculate start position correctly when end-col is set but
  end-line is not.

-- 
Ari Roponen





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#9679: acknowledged by developer (close 9679)
  2012-05-13  6:39     ` bug#9679: acknowledged by developer (close 9679) Ari Roponen
@ 2012-05-13  9:18       ` Chong Yidong
  0 siblings, 0 replies; 13+ messages in thread
From: Chong Yidong @ 2012-05-13  9:18 UTC (permalink / raw)
  To: Ari Roponen; +Cc: 9679

Ari Roponen <ari.roponen@gmail.com> writes:

> This bug ("24.0.90; After rgrep, next-error goes to the wrong line") was
> fixed in trunk, but it still happens in emacs-24 branch.
>
> Maybe the patch from the following revision could be backported/applied
> into the emacs-24 branch.  I just tested it locally, and it indeed fixed
> the problem there, too.

Ah, I didn't realize this was a regression against Emacs 23.  I've
backported the fix the emacs-24 branch; thanks for the heads-up.





^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-05-13  9:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <87r4uwybeh.fsf@gnu.org>
2011-10-06  6:14 ` bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line Ari Roponen
2011-10-06 15:27   ` Juri Linkov
2011-10-07  5:15     ` Ari Roponen
2011-10-07  8:41       ` Ari Roponen
2011-10-07 15:20       ` Juri Linkov
2011-10-07 15:51         ` Ari Roponen
2011-10-07 16:36           ` Juri Linkov
2011-10-07 17:48             ` Ari Roponen
2011-10-08 16:30               ` Juri Linkov
2011-10-09  5:38                 ` Ari Roponen
2012-05-06  6:23                   ` Ari Roponen
     [not found]   ` <handler.9679.C.13363577086087.notifdonectrl.0@debbugs.gnu.org>
2012-05-13  6:39     ` bug#9679: acknowledged by developer (close 9679) Ari Roponen
2012-05-13  9:18       ` Chong Yidong

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).