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

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

[Raimon Grau]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Guard-occur-against-an-undefined-orig-line.patch --]
[-- Type: text/x-diff, Size: 1530 bytes --]

From 2f874d70b6d69debd7370da562e768fc3b9b8198 Mon Sep 17 00:00:00 2001
From: Raimon Grau <raimonster@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

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: occur-orig-line.patch --]
[-- Type: text/x-diff, Size: 1901 bytes --]

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

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

[Eli Zaretskii]

> From: Juri Linkov <juri@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.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@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.

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

[Juri Linkov]

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

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33476@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.

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

[Juri Linkov]

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

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33476@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.

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

[Richard Stallman]

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

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

[Glenn Morris]

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.

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Juri Linkov <juri@linkov.net>,  33476@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.

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

[Richard Stallman]

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

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

[Robert Pluim]

Richard Stallman <rms@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

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

[Richard Stallman]

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