* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
@ 2018-11-23 21:10 Raimon Grau
2018-11-24 21:54 ` Juri Linkov
0 siblings, 1 reply; 14+ messages in thread
From: Raimon Grau @ 2018-11-23 21:10 UTC (permalink / raw)
To: 33476
[-- 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
^ permalink raw reply related [flat|nested] 14+ messages in thread* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-11-23 21:10 bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line Raimon Grau
@ 2018-11-24 21:54 ` Juri Linkov
2018-11-26 17:54 ` Eli Zaretskii
2018-12-08 9:25 ` Eli Zaretskii
0 siblings, 2 replies; 14+ messages in thread
From: Juri Linkov @ 2018-11-24 21:54 UTC (permalink / raw)
To: 33476
[-- 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
^ permalink raw reply related [flat|nested] 14+ messages in thread* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-11-24 21:54 ` Juri Linkov
@ 2018-11-26 17:54 ` Eli Zaretskii
2018-12-08 9:25 ` Eli Zaretskii
1 sibling, 0 replies; 14+ messages in thread
From: Eli Zaretskii @ 2018-11-26 17:54 UTC (permalink / raw)
To: Juri Linkov; +Cc: 33476
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-11-24 21:54 ` Juri Linkov
2018-11-26 17:54 ` Eli Zaretskii
@ 2018-12-08 9:25 ` Eli Zaretskii
2018-12-08 23:16 ` Juri Linkov
1 sibling, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2018-12-08 9:25 UTC (permalink / raw)
To: Juri Linkov; +Cc: 33476
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-08 9:25 ` Eli Zaretskii
@ 2018-12-08 23:16 ` Juri Linkov
2018-12-09 7:55 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Juri Linkov @ 2018-12-08 23:16 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 33476
>> > 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-08 23:16 ` Juri Linkov
@ 2018-12-09 7:55 ` Eli Zaretskii
2018-12-09 23:54 ` Juri Linkov
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2018-12-09 7:55 UTC (permalink / raw)
To: Juri Linkov; +Cc: 33476
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-09 7:55 ` Eli Zaretskii
@ 2018-12-09 23:54 ` Juri Linkov
2018-12-10 6:27 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Juri Linkov @ 2018-12-09 23:54 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 33476
>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-09 23:54 ` Juri Linkov
@ 2018-12-10 6:27 ` Eli Zaretskii
2018-12-11 4:40 ` Richard Stallman
2018-12-11 23:07 ` Glenn Morris
0 siblings, 2 replies; 14+ messages in thread
From: Eli Zaretskii @ 2018-12-10 6:27 UTC (permalink / raw)
To: Juri Linkov; +Cc: 33476
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-10 6:27 ` Eli Zaretskii
@ 2018-12-11 4:40 ` Richard Stallman
2018-12-11 23:07 ` Glenn Morris
1 sibling, 0 replies; 14+ messages in thread
From: Richard Stallman @ 2018-12-11 4:40 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 33476, juri
[[[ 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-10 6:27 ` Eli Zaretskii
2018-12-11 4:40 ` Richard Stallman
@ 2018-12-11 23:07 ` Glenn Morris
2018-12-12 17:07 ` Eli Zaretskii
2018-12-13 0:12 ` Richard Stallman
1 sibling, 2 replies; 14+ messages in thread
From: Glenn Morris @ 2018-12-11 23:07 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 33476, Juri Linkov
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-11 23:07 ` Glenn Morris
@ 2018-12-12 17:07 ` Eli Zaretskii
2018-12-13 0:12 ` Richard Stallman
1 sibling, 0 replies; 14+ messages in thread
From: Eli Zaretskii @ 2018-12-12 17:07 UTC (permalink / raw)
To: Glenn Morris; +Cc: 33476, juri
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-11 23:07 ` Glenn Morris
2018-12-12 17:07 ` Eli Zaretskii
@ 2018-12-13 0:12 ` Richard Stallman
2018-12-13 8:57 ` Robert Pluim
1 sibling, 1 reply; 14+ messages in thread
From: Richard Stallman @ 2018-12-13 0:12 UTC (permalink / raw)
To: Glenn Morris; +Cc: 33476, juri
[[[ 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-13 0:12 ` Richard Stallman
@ 2018-12-13 8:57 ` Robert Pluim
2018-12-13 21:14 ` Richard Stallman
0 siblings, 1 reply; 14+ messages in thread
From: Robert Pluim @ 2018-12-13 8:57 UTC (permalink / raw)
To: Richard Stallman; +Cc: 33476, juri
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line
2018-12-13 8:57 ` Robert Pluim
@ 2018-12-13 21:14 ` Richard Stallman
0 siblings, 0 replies; 14+ messages in thread
From: Richard Stallman @ 2018-12-13 21:14 UTC (permalink / raw)
To: Robert Pluim; +Cc: 33476, juri
[[[ 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-12-13 21:14 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-23 21:10 bug#33476: [PATCH] Fix occur revert with list-matching-lines-jump-to-current-line Raimon Grau
2018-11-24 21:54 ` Juri Linkov
2018-11-26 17:54 ` Eli Zaretskii
2018-12-08 9:25 ` Eli Zaretskii
2018-12-08 23:16 ` Juri Linkov
2018-12-09 7:55 ` Eli Zaretskii
2018-12-09 23:54 ` Juri Linkov
2018-12-10 6:27 ` Eli Zaretskii
2018-12-11 4:40 ` Richard Stallman
2018-12-11 23:07 ` Glenn Morris
2018-12-12 17:07 ` Eli Zaretskii
2018-12-13 0:12 ` Richard Stallman
2018-12-13 8:57 ` Robert Pluim
2018-12-13 21:14 ` Richard Stallman
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).