GNU bug report logs - #33476
[PATCH] Fix occur revert with list-matching-lines-jump-to-current-line

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 33476 in the body.
You can then email your comments to 33476 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: Raimon Grau <raimon <at> konghq.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
Date: Fri, 23 Nov 2018 21:10:19 +0000

[Message part 1 (text/plain, inline)]

Hello,

I reproduced this bug with Emacs 26.1 and 26.1.90 using -Q option.

When pressing 'g' (revert-buffer) in an *Occur* buffer, in the case of
having `list-matching-lines-jump-to-current-line' set to non-nil, the
function errors as it can't find orig-line.

I'm attaching a patch that adds a guard to the list of guards before
inserting the current line.

Cheers,

Raimon Grau

[0001-Guard-occur-against-an-undefined-orig-line.patch (text/x-diff, inline)]

From 2f874d70b6d69debd7370da562e768fc3b9b8198 Mon Sep 17 00:00:00 2001
From: Raimon Grau <raimonster <at> gmail.com>
Date: Fri, 23 Nov 2018 20:37:12 +0000
Subject: [PATCH] Guard occur against an undefined orig-line

* lisp/replace.el (occur-engine): Avoid inserting the current line if
orig-line is nil. This happens, for example, when reverting an occur
buffer with `list-matching-lines-jump-to-current-line' set to t.
---
 lisp/replace.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/replace.el b/lisp/replace.el
index 940bf56..4f0cbf4 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1748,6 +1748,7 @@ occur-engine
                           (when (and list-matching-lines-jump-to-current-line
                                      (not multi-occur-p)
                                      (not orig-line-shown-p)
+                                     orig-line
                                      (>= curr-line orig-line))
                             (insert
                              (concat
@@ -1774,7 +1775,8 @@ occur-engine
                   ;; Insert original line if haven't done yet.
                   (when (and list-matching-lines-jump-to-current-line
                              (not multi-occur-p)
-                             (not orig-line-shown-p))
+                             (not orig-line-shown-p)
+                             orig-line)
                     (with-current-buffer out-buf
                       (insert
                        (concat
-- 
2.7.4

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

From: Juri Linkov <juri <at> linkov.net>
To: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Sat, 24 Nov 2018 23:54:24 +0200

[Message part 1 (text/plain, inline)]

> I reproduced this bug with Emacs 26.1 and 26.1.90 using -Q option.
>
> When pressing 'g' (revert-buffer) in an *Occur* buffer, in the case of
> having `list-matching-lines-jump-to-current-line' set to non-nil, the
> function errors as it can't find orig-line.
>
> I'm attaching a patch that adds a guard to the list of guards before
> inserting the current line.

Eli, is it ok to install this submitted patch to the release branch emacs-26?
At least, it prevents the error signal.

I see that for Emacs 27 in master this feature is completely rewritten.
But still I found a bug in master that can be fixed with another patch.
The following patch is for Emacs 27:

[occur-orig-line.patch (text/x-diff, inline)]

diff --git a/lisp/replace.el b/lisp/replace.el
index ecb47936e7..1e64514c1c 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1657,7 +1657,10 @@ occur-engine
                   (lines 0)               ; count of matching lines
 	          (matches 0)             ; count of matches
 		  (headerpt (with-current-buffer out-buf (point)))
-                  )
+		  (orig-line (if (not (overlayp boo))
+				 (line-number-at-pos)
+			       (line-number-at-pos
+				(overlay-get boo 'occur--orig-point)))))
 	      (save-excursion
                 ;; begin searching in the buffer
 		(goto-char (if (overlayp boo) (overlay-start boo) (point-min)))
@@ -1665,9 +1668,6 @@ occur-engine
 	        (let* ((limit (if (overlayp boo) (overlay-end boo) (point-max)))
                        (start-line (line-number-at-pos))
 		       (curr-line start-line) ; line count
-		       (orig-line (if (not (overlayp boo)) 1
-                                    (line-number-at-pos
-                                     (overlay-get boo 'occur--orig-point))))
 		       (orig-line-shown-p)
 		       (prev-line nil)        ; line number of prev match endpt
 		       (prev-after-lines nil) ; context lines of prev match
@@ -1796,7 +1796,7 @@ occur-engine
 				(setq orig-line-shown-p t)
 				(save-excursion
 				  (goto-char (point-min))
-				  (forward-line (- orig-line start-line 1))
+				  (forward-line (1- orig-line))
 				  (occur-engine-line (line-beginning-position)
 						     (line-end-position) keep-props)))))
 		        ;; Actually insert the match display data
@@ -1834,7 +1834,7 @@ occur-engine
 		    (let ((orig-line-str
 			   (save-excursion
 			     (goto-char (point-min))
-			     (forward-line (- orig-line start-line 1))
+			     (forward-line (1- orig-line))
 			     (occur-engine-line (line-beginning-position)
 						(line-end-position) keep-props))))
 		      (add-face-text-property

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Mon, 26 Nov 2018 19:54:36 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Date: Sat, 24 Nov 2018 23:54:24 +0200
> 
> > I reproduced this bug with Emacs 26.1 and 26.1.90 using -Q option.
> >
> > When pressing 'g' (revert-buffer) in an *Occur* buffer, in the case of
> > having `list-matching-lines-jump-to-current-line' set to non-nil, the
> > function errors as it can't find orig-line.
> >
> > I'm attaching a patch that adds a guard to the list of guards before
> > inserting the current line.
> 
> Eli, is it ok to install this submitted patch to the release branch emacs-26?
> At least, it prevents the error signal.
> 
> I see that for Emacs 27 in master this feature is completely rewritten.
> But still I found a bug in master that can be fixed with another patch.
> The following patch is for Emacs 27:

Sorry for not replying until now.  I need to refresh my memory about
the changes we did lately in this area, and I didn't yet have time to
do that.  Stay tuned.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Sat, 08 Dec 2018 11:25:14 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Date: Sat, 24 Nov 2018 23:54:24 +0200
> 
> > I reproduced this bug with Emacs 26.1 and 26.1.90 using -Q option.
> >
> > When pressing 'g' (revert-buffer) in an *Occur* buffer, in the case of
> > having `list-matching-lines-jump-to-current-line' set to non-nil, the
> > function errors as it can't find orig-line.
> >
> > I'm attaching a patch that adds a guard to the list of guards before
> > inserting the current line.
> 
> Eli, is it ok to install this submitted patch to the release branch emacs-26?
> At least, it prevents the error signal.
> 
> I see that for Emacs 27 in master this feature is completely rewritten.

It was rewritten to fix the same bug, AFAIU, see bug#32543.  Why do we
need to solve it again in emacs-26?

> But still I found a bug in master that can be fixed with another patch.
> The following patch is for Emacs 27:

What is the bug you found in the master branch?

And which patch do you propose for the emacs-26 branch?

Thanks.

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

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Sun, 09 Dec 2018 01:16:44 +0200

>> > I reproduced this bug with Emacs 26.1 and 26.1.90 using -Q option.
>> >
>> > When pressing 'g' (revert-buffer) in an *Occur* buffer, in the case of
>> > having `list-matching-lines-jump-to-current-line' set to non-nil, the
>> > function errors as it can't find orig-line.
>> >
>> > I'm attaching a patch that adds a guard to the list of guards before
>> > inserting the current line.
>> 
>> Eli, is it ok to install this submitted patch to the release branch emacs-26?
>> At least, it prevents the error signal.
>> 
>> I see that for Emacs 27 in master this feature is completely rewritten.
>
> It was rewritten to fix the same bug, AFAIU, see bug#32543.  Why do we
> need to solve it again in emacs-26?

I guess porting the fix from master to emacs-26 is not safe.

> And which patch do you propose for the emacs-26 branch?

I propose to install the patch from OP.  This patch avoids the error.
It still might behave incorrectly in some cases, but at least it
doesn't raise the error.

>> But still I found a bug in master that can be fixed with another patch.
>> The following patch is for Emacs 27:
>
> What is the bug you found in the master branch?

In the master branch when `list-matching-lines-jump-to-current-line'
is non-nil, it doesn't show the current line highlighted.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Sun, 09 Dec 2018 09:55:05 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Cc: 33476 <at> debbugs.gnu.org
> Date: Sun, 09 Dec 2018 01:16:44 +0200
> 
> >> I see that for Emacs 27 in master this feature is completely rewritten.
> >
> > It was rewritten to fix the same bug, AFAIU, see bug#32543.  Why do we
> > need to solve it again in emacs-26?
> 
> I guess porting the fix from master to emacs-26 is not safe.
> 
> > And which patch do you propose for the emacs-26 branch?
> 
> I propose to install the patch from OP.  This patch avoids the error.
> It still might behave incorrectly in some cases, but at least it
> doesn't raise the error.

OK, please push that to emacs-26, and thanks.

> >> But still I found a bug in master that can be fixed with another patch.
> >> The following patch is for Emacs 27:
> >
> > What is the bug you found in the master branch?
> 
> In the master branch when `list-matching-lines-jump-to-current-line'
> is non-nil, it doesn't show the current line highlighted.

Fine with me.

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

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Mon, 10 Dec 2018 01:54:14 +0200

>> I propose to install the patch from OP.  This patch avoids the error.
>> It still might behave incorrectly in some cases, but at least it
>> doesn't raise the error.
>
> OK, please push that to emacs-26, and thanks.

Done.

The file admin/notes/repo instructs to use the phrase
"Not to be merged to master" to indicate that it should
not be merged to the master.

But actually `gitmerge-skip-regexp' doesn't match this phrase:

(string-match gitmerge-skip-regexp "; Not to be merged to master")
nil

Should admin/gitmerge.el be fixed to support this phrase
before the next merge to master?

>> >> But still I found a bug in master that can be fixed with another patch.
>> >> The following patch is for Emacs 27:
>> >
>> > What is the bug you found in the master branch?
>> 
>> In the master branch when `list-matching-lines-jump-to-current-line'
>> is non-nil, it doesn't show the current line highlighted.
>
> Fine with me.

Done.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 33476 <at> debbugs.gnu.org
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Mon, 10 Dec 2018 08:27:01 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Cc: 33476 <at> debbugs.gnu.org
> Date: Mon, 10 Dec 2018 01:54:14 +0200
> 
> The file admin/notes/repo instructs to use the phrase
> "Not to be merged to master" to indicate that it should
> not be merged to the master.
> 
> But actually `gitmerge-skip-regexp' doesn't match this phrase:
> 
> (string-match gitmerge-skip-regexp "; Not to be merged to master")
> nil
> 
> Should admin/gitmerge.el be fixed to support this phrase
> before the next merge to master?

Please don't use the instructions in admin/notes/repo, use the
instructions in CONTRIBUTE instead.  Personally, I'm not sure why the
former exists, I'd be happy if we deleted it (and move any information
that isn't already there to CONTRIBUTE).

Thanks.

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

From: Richard Stallman <rms <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 33476 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Mon, 10 Dec 2018 23:40:09 -0500

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Please don't use the instructions in admin/notes/repo, use the
  > instructions in CONTRIBUTE instead.  Personally, I'm not sure why the
  > former exists, I'd be happy if we deleted it (and move any information
  > that isn't already there to CONTRIBUTE).

Does anyone want to argue for preserving the instructions in the
admin/notes/repo?
-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

From: Glenn Morris <rgm <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 33476 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Tue, 11 Dec 2018 18:07:52 -0500

Eli Zaretskii wrote:

> Please don't use the instructions in admin/notes/repo

They just happened to get outdated in this one specific area
({bzr,git}merge.el used to skip anything matching "merge",
but that was found to be too broad).

> use the instructions in CONTRIBUTE instead. Personally, I'm not sure
> why the former exists

I view(ed) it as:

CONTRIBUTE is for people without write access
admin/notes/repo is for people with write access

Obviously these can get a bit blurred with distributed VCS, but not totally.
Duplication of information should of course be avoided.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 33476 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Wed, 12 Dec 2018 19:07:46 +0200

> From: Glenn Morris <rgm <at> gnu.org>
> Cc: Juri Linkov <juri <at> linkov.net>,  33476 <at> debbugs.gnu.org
> Date: Tue, 11 Dec 2018 18:07:52 -0500
> 
> Eli Zaretskii wrote:
> 
> > Please don't use the instructions in admin/notes/repo
> 
> They just happened to get outdated in this one specific area
> ({bzr,git}merge.el used to skip anything matching "merge",
> but that was found to be too broad).

If we have just one place, there will be less risk of one of them
becoming outdated.

> > use the instructions in CONTRIBUTE instead. Personally, I'm not sure
> > why the former exists
> 
> I view(ed) it as:
> 
> CONTRIBUTE is for people without write access
> admin/notes/repo is for people with write access

I don't really see enough differences to justify an extra file.

I think admin/notes/repo is just a historic accident: it was created
when we migrated to Git, when we didn't yet have CONTRIBUTE.

> Duplication of information should of course be avoided.

I don't see how we can avoid duplication if we want each of these two
files be self-contained.

I'm not going to fight if someone wants to leave admin/notes/repo
alone, but the confusion such as this one will IMO continue until we
resolve the duplication.

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

From: Richard Stallman <rms <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: eliz <at> gnu.org, 33476 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Wed, 12 Dec 2018 19:12:32 -0500

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > CONTRIBUTE is for people without write access
  > admin/notes/repo is for people with write access

If that's what defines the difference, how about making each one
start with a statement about which situation it is meant for?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Richard Stallman <rms <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 33476 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Thu, 13 Dec 2018 09:57:02 +0100

Richard Stallman <rms <at> gnu.org> writes:

> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > CONTRIBUTE is for people without write access
>   > admin/notes/repo is for people with write access
>
> If that's what defines the difference, how about making each one
> start with a statement about which situation it is meant for?

CONTRIBUTE explicitly talks about committing changes on behalf of
others, so I donʼt think this distinction is accurate.

Personally Iʼd prefer everything to be in CONTRIBUTE, ideally with 'if
you have commit access do this else that' annotations.

Robert

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

From: Richard Stallman <rms <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: rgm <at> gnu.org, 33476 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#33476: [PATCH] Fix occur revert with
 list-matching-lines-jump-to-current-line
Date: Thu, 13 Dec 2018 16:14:21 -0500

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Personally Iʼd prefer everything to be in CONTRIBUTE, ideally with 'if
  > you have commit access do this else that' annotations.

I don't have an opinion about how to handle this -- I just want people
to do a clean job of whichever method they choose.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Previous Next

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