* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
@ 2017-01-10 10:08 Tino Calancha
2017-01-10 14:22 ` npostavs
0 siblings, 1 reply; 12+ messages in thread
From: Tino Calancha @ 2017-01-10 10:08 UTC (permalink / raw)
To: 25410
After deletion of a large file from CVS, a diff shows
a very large hunk with just deleted lines. Then, for unified diffs, a call
to `diff-refine-hunk' on that hunk takes a huge time.
Instead, it's better to first check if the hunk adds new lines: only when
this is true, then proceed with the hunk refinement.
emacs -Q
M-! git diff ef8c9f8^ ef8c9f8 RET
C-x o
M-x diff-mode RET
C-c C-b
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From b3252092f8fdfc03c02d23022d901a625d183d89 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Tue, 10 Jan 2017 18:46:00 +0900
Subject: [PATCH] Refine an unified diff hunk only if adds lines
* lisp/vc/diff-mode.el (diff-refine-hunk): Refine the hunk
only when adds some new lines (Bug#25410).
---
lisp/vc/diff-mode.el | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..e045a5d974 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2075,22 +2075,23 @@ diff-refine-hunk
(props-c '((diff-mode . fine) (face diff-refine-changed)))
(props-r '((diff-mode . fine) (face diff-refine-removed)))
(props-a '((diff-mode . fine) (face diff-refine-added))))
-
(remove-overlays beg end 'diff-mode 'fine)
-
(goto-char beg)
(pcase style
(`unified
- (while (re-search-forward
- (eval-when-compile
- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
- "\\(\\)"
- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
- end t)
- (smerge-refine-subst (match-beginning 0) (match-end 1)
- (match-end 1) (match-end 0)
- nil 'diff-refine-preproc props-r props-a)))
+ ;; Refine hunk only when it adds lines (Bug#25410).
+ (when (re-search-forward "^\\(?:\\+.*\n\\)+" end t)
+ (goto-char beg)
+ (while (re-search-forward
+ (eval-when-compile
+ (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
+ (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
+ "\\(\\)"
+ "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
+ end t)
+ (smerge-refine-subst (match-beginning 0) (match-end 1)
+ (match-end 1) (match-end 0)
+ nil 'diff-refine-preproc props-r props-a))))
(`context
(let* ((middle (save-excursion (re-search-forward "^---")))
(other middle))
--
2.11.0
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.5)
of 2017-01-09
Repository revision: ef8c9f8fc922b615aca91b47820d1f1900fddc96
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-10 10:08 bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines Tino Calancha
@ 2017-01-10 14:22 ` npostavs
2017-01-10 15:07 ` Tino Calancha
2017-01-11 2:49 ` Tino Calancha
0 siblings, 2 replies; 12+ messages in thread
From: npostavs @ 2017-01-10 14:22 UTC (permalink / raw)
To: Tino Calancha; +Cc: 25410
Tino Calancha <tino.calancha@gmail.com> writes:
> After deletion of a large file from CVS, a diff shows
> a very large hunk with just deleted lines. Then, for unified diffs, a call
> to `diff-refine-hunk' on that hunk takes a huge time.
> Instead, it's better to first check if the hunk adds new lines: only when
> this is true, then proceed with the hunk refinement.
What about a diff that adds a very large file? Perhaps we should only
refine if there added lines *and* deleted lines?
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-10 14:22 ` npostavs
@ 2017-01-10 15:07 ` Tino Calancha
2017-01-11 2:49 ` Tino Calancha
1 sibling, 0 replies; 12+ messages in thread
From: Tino Calancha @ 2017-01-10 15:07 UTC (permalink / raw)
To: npostavs; +Cc: 25410, Tino Calancha
On Tue, 10 Jan 2017, npostavs@users.sourceforge.net wrote:
> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> After deletion of a large file from CVS, a diff shows
>> a very large hunk with just deleted lines. Then, for unified diffs, a call
>> to `diff-refine-hunk' on that hunk takes a huge time.
>> Instead, it's better to first check if the hunk adds new lines: only when
>> this is true, then proceed with the hunk refinement.
>
> What about a diff that adds a very large file? Perhaps we should only
> refine if there added lines *and* deleted lines?
On Tue, 10 Jan 2017, npostavs@users.sourceforge.net wrote:
>What about a diff that adds a very large file? Perhaps we should only
>refine if there added lines *and* deleted lines?
That's logical; at the end neither a hunk just deleting nor one
just adding lines need to be refined. We might do that if you like. It
would be more symmetrical.
From a performance point of view, current code in the case where
the hunk just adds lines is not as patological as the opposite one.
For instance:
emacs -Q
M-! git diff ef8c9f8^ ef8c9f8 RET
C-x o C-x C-q
M-x diff-mode RET
R ; Reverse the direction of the diffs.
C-c C-b ; Refine hunk.
;; Perform reasonably fast.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-10 14:22 ` npostavs
2017-01-10 15:07 ` Tino Calancha
@ 2017-01-11 2:49 ` Tino Calancha
2017-01-11 8:13 ` Tino Calancha
1 sibling, 1 reply; 12+ messages in thread
From: Tino Calancha @ 2017-01-11 2:49 UTC (permalink / raw)
To: npostavs; +Cc: 25410, Tino Calancha
npostavs@users.sourceforge.net writes:
> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> After deletion of a large file from CVS, a diff shows
>> a very large hunk with just deleted lines. Then, for unified diffs, a call
>> to `diff-refine-hunk' on that hunk takes a huge time.
>> Instead, it's better to first check if the hunk adds new lines: only when
>> this is true, then proceed with the hunk refinement.
>
> What about a diff that adds a very large file? Perhaps we should only
> refine if there added lines *and* deleted lines?
I have updated the patch. Now it checks before the `pcase' that
the hunk adds and removes lines. Only when this is true, we enter
in the `pcase'.
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 0ff79ca6f106f121a05b8c5de55990b88fecb4d2 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Wed, 11 Jan 2017 11:42:56 +0900
Subject: [PATCH] Only refine diff hunks that both remove and add lines
* lisp/vc/diff-mode.el (diff-refine-hunk): Refine the hunk
only if it adds and removes some lines (Bug#25410).
---
lisp/vc/diff-mode.el | 77 ++++++++++++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 38 deletions(-)
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..f0c53e6f8a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2075,44 +2075,45 @@ diff-refine-hunk
(props-c '((diff-mode . fine) (face diff-refine-changed)))
(props-r '((diff-mode . fine) (face diff-refine-removed)))
(props-a '((diff-mode . fine) (face diff-refine-added))))
-
- (remove-overlays beg end 'diff-mode 'fine)
-
- (goto-char beg)
- (pcase style
- (`unified
- (while (re-search-forward
- (eval-when-compile
- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
- "\\(\\)"
- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
- end t)
- (smerge-refine-subst (match-beginning 0) (match-end 1)
- (match-end 1) (match-end 0)
- nil 'diff-refine-preproc props-r props-a)))
- (`context
- (let* ((middle (save-excursion (re-search-forward "^---")))
- (other middle))
- (while (re-search-forward "^\\(?:!.*\n\\)+" middle t)
- (smerge-refine-subst (match-beginning 0) (match-end 0)
- (save-excursion
- (goto-char other)
- (re-search-forward "^\\(?:!.*\n\\)+" end)
- (setq other (match-end 0))
- (match-beginning 0))
- other
- (if diff-use-changed-face props-c)
- 'diff-refine-preproc
- (unless diff-use-changed-face props-r)
- (unless diff-use-changed-face props-a)))))
- (_ ;; Normal diffs.
- (let ((beg1 (1+ (point))))
- (when (re-search-forward "^---.*\n" end t)
- ;; It's a combined add&remove, so there's something to do.
- (smerge-refine-subst beg1 (match-beginning 0)
- (match-end 0) end
- nil 'diff-refine-preproc props-r props-a))))))))
+ ;; Only refine the hunk if both adds and removes lines (Bug#25410).
+ (when (and (save-excursion (re-search-forward "^-.*\n" end t))
+ (re-search-forward "^\\+.*\n" end t))
+ (remove-overlays beg end 'diff-mode 'fine)
+ (goto-char beg)
+ (pcase style
+ (`unified
+ (while (re-search-forward
+ (eval-when-compile
+ (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
+ (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
+ "\\(\\)"
+ "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
+ end t)
+ (smerge-refine-subst (match-beginning 0) (match-end 1)
+ (match-end 1) (match-end 0)
+ nil 'diff-refine-preproc props-r props-a)))
+ (`context
+ (let* ((middle (save-excursion (re-search-forward "^---")))
+ (other middle))
+ (while (re-search-forward "^\\(?:!.*\n\\)+" middle t)
+ (smerge-refine-subst (match-beginning 0) (match-end 0)
+ (save-excursion
+ (goto-char other)
+ (re-search-forward "^\\(?:!.*\n\\)+" end)
+ (setq other (match-end 0))
+ (match-beginning 0))
+ other
+ (if diff-use-changed-face props-c)
+ 'diff-refine-preproc
+ (unless diff-use-changed-face props-r)
+ (unless diff-use-changed-face props-a)))))
+ (_ ;; Normal diffs.
+ (let ((beg1 (1+ (point))))
+ (when (re-search-forward "^---.*\n" end t)
+ ;; It's a combined add&remove, so there's something to do.
+ (smerge-refine-subst beg1 (match-beginning 0)
+ (match-end 0) end
+ nil 'diff-refine-preproc props-r props-a)))))))))
(defun diff-undo (&optional arg)
"Perform `undo', ignoring the buffer's read-only status."
--
2.11.0
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.5)
of 2017-01-10
Repository revision: fa0a2b4e7c81f57aecc1d94df00588a4dd5c281d
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-11 2:49 ` Tino Calancha
@ 2017-01-11 8:13 ` Tino Calancha
[not found] ` <87wpe0zz0s.fsf@users.sourceforge.net>
0 siblings, 1 reply; 12+ messages in thread
From: Tino Calancha @ 2017-01-11 8:13 UTC (permalink / raw)
To: npostavs; +Cc: 25410, Tino Calancha
Tino Calancha <tino.calancha@gmail.com> writes:
> npostavs@users.sourceforge.net writes:
>
>> What about a diff that adds a very large file? Perhaps we should only
>> refine if there added lines *and* deleted lines?
> I have updated the patch. Now it checks before the `pcase' that
> the hunk adds and removes lines. Only when this is true, we enter
> in the `pcase'.
> - nil 'diff-refine-preproc props-r props-a))))))))
> + ;; Only refine the hunk if both adds and removes lines (Bug#25410).
> + (when (and (save-excursion (re-search-forward "^-.*\n" end t))
> + (re-search-forward "^\\+.*\n" end t))
> + (remove-overlays beg end 'diff-mode 'fine)
> + (goto-char beg)
> + (pcase style
> + (`unified
This check only has sense for unified diffs, where the new lines start with
'+' and the deleted ones start with '-'.
We might use the first patch in this thread or the following one:
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 89742f18291c1bb7fc99dfd5ac71a7d625699534 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Wed, 11 Jan 2017 17:05:05 +0900
Subject: [PATCH] Refine an unified diff hunk only if both removes and adds
lines
* lisp/vc/diff-mode.el (diff-refine-hunk): Refine the unified
diff hunk only if it adds and removes some lines (Bug#25410).
---
lisp/vc/diff-mode.el | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..4cc20338c1 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2075,22 +2075,23 @@ diff-refine-hunk
(props-c '((diff-mode . fine) (face diff-refine-changed)))
(props-r '((diff-mode . fine) (face diff-refine-removed)))
(props-a '((diff-mode . fine) (face diff-refine-added))))
-
(remove-overlays beg end 'diff-mode 'fine)
-
(goto-char beg)
(pcase style
(`unified
- (while (re-search-forward
- (eval-when-compile
- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
- "\\(\\)"
- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
- end t)
- (smerge-refine-subst (match-beginning 0) (match-end 1)
- (match-end 1) (match-end 0)
- nil 'diff-refine-preproc props-r props-a)))
+ ;; Only refine an unified hunk if both adds and removes lines (Bug#25410).
+ (when (and (save-excursion (re-search-forward "^-.*\n" end t))
+ (save-excursion (re-search-forward "^\\+.*\n" end t)))
+ (while (re-search-forward
+ (eval-when-compile
+ (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
+ (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
+ "\\(\\)"
+ "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
+ end t)
+ (smerge-refine-subst (match-beginning 0) (match-end 1)
+ (match-end 1) (match-end 0)
+ nil 'diff-refine-preproc props-r props-a))))
(`context
(let* ((middle (save-excursion (re-search-forward "^---")))
(other middle))
--
2.11.0
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.5)
of 2017-01-11
Repository revision: fa0a2b4e7c81f57aecc1d94df00588a4dd5c281d
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
[not found] ` <87wpe0zz0s.fsf@users.sourceforge.net>
@ 2017-01-12 5:32 ` Tino Calancha
2017-01-13 4:50 ` npostavs
0 siblings, 1 reply; 12+ messages in thread
From: Tino Calancha @ 2017-01-12 5:32 UTC (permalink / raw)
To: npostavs; +Cc: 25410
On Wed, 11 Jan 2017, npostavs@users.sourceforge.net wrote:
> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> From a performance point of view, current code in the case where
>> the hunk just adds lines is not as patological as the opposite one.
>> M-! git diff ef8c9f8^ ef8c9f8 RET
>
> By the way, with Emacs 25.1, I notice trying to refine this gives me a
> regex stack overflow error. Probably my recent changes to the regex
> stack limits allow Emacs to spend more time on this instead of
> overflowing.
Yeah, your commit has revealed such nasty regexp's. Before we get
a stack overflow and the refine of the hunk ends.
>> We might use the first patch in this thread or the following one:
>
> It's probably okay to use the first patch (i.e., don't bother checking
> for delete-only hunks), with an added comment about the asymmetry. But
> I think it would be better to change diff-refine-hunk to avoid the
> inefficient regex, like this:
I agree with you it's better to not use such heavy regexp matching
too many lines.
Your patch LGTM. Thank very much.
>From f5ea9e585b535390a69e442d83ecbeec8e8e18d2 Mon Sep 17 00:00:00 2001
>From: Noam Postavsky <npostavs@gmail.com>
>Date: Wed, 11 Jan 2017 23:21:38 -0500
>Subject: [PATCH v1] Avoid inefficient regex in diff-refine-hunk
(Bug#25410)
>
>* lisp/vc/diff-mode.el (diff--forward-while-leading-char): New function.
>(diff-refine-hunk): Use it instead of trying to match multiple lines
>with a single lines.
>---
> lisp/vc/diff-mode.el | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
>diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
>index 9dfcd94..915e0b1 100644
>--- a/lisp/vc/diff-mode.el
>+++ b/lisp/vc/diff-mode.el
>@@ -2062,6 +2062,15 @@ diff-refine-preproc
> (declare-function smerge-refine-subst "smerge-mode"
> (beg1 end1 beg2 end2 props-c &optional preproc props-r
props-a))
>
>+(defun diff--forward-while-leading-char (char bound)
>+ "Move point until reaching a line not starting with CHAR.
>+Return new point, if it was moved."
>+ (let ((pt nil))
>+ (while (and (< (point) bound) (eql (following-char) char))
>+ (forward-line 1)
>+ (setq pt (point)))
>+ pt))
>+
> (defun diff-refine-hunk ()
> "Highlight changes of hunk at point at a finer granularity."
> (interactive)
>@@ -2081,16 +2090,14 @@ diff-refine-hunk
> (goto-char beg)
> (pcase style
> (`unified
>- (while (re-search-forward
>- (eval-when-compile
>- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
>- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
>- "\\(\\)"
>- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
>- end t)
>- (smerge-refine-subst (match-beginning 0) (match-end 1)
>- (match-end 1) (match-end 0)
>- nil 'diff-refine-preproc props-r
props-a)))
>+ (while (re-search-forward "^-" end t)
>+ (let ((beg-del (progn (beginning-of-line) (point)))
>+ beg-add end-add)
>+ (when (and (setq beg-add (diff--forward-while-leading-char
?- end))
>+ (or (diff--forward-while-leading-char ?\\ end)
t)
>+ (setq end-add (diff--forward-while-leading-char
?+ end)))
>+ (smerge-refine-subst beg-del beg-add beg-add end-add
>+ nil 'diff-refine-preproc props-r
props-a)))))
> (`context
> (let* ((middle (save-excursion (re-search-forward "^---")))
> (other middle))
>--
>2.9.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-12 5:32 ` Tino Calancha
@ 2017-01-13 4:50 ` npostavs
2017-01-13 6:14 ` Tino Calancha
2017-01-19 1:55 ` npostavs
0 siblings, 2 replies; 12+ messages in thread
From: npostavs @ 2017-01-13 4:50 UTC (permalink / raw)
To: Tino Calancha; +Cc: 25410
[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]
tags 25410 patch
quit
>> (`unified
>>- (while (re-search-forward
>>- (eval-when-compile
>>- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
>>- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
>>- "\\(\\)"
>>- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
>>- end t)
[...]
>> + (while (re-search-forward "^-" end t)
>> + (let ((beg-del (progn (beginning-of-line) (point)))
>> + beg-add end-add)
>> + (when (and (setq beg-add (diff--forward-while-leading-char ?- end))
>> + (or (diff--forward-while-leading-char ?\\ end) t)
>> + (setq end-add (diff--forward-while-leading-char ?+ end)))
Actually I made a mistake here, this doesn't allow for "\ No newline at
end of file" in the + part of the hunk. Here's v2 (seems I hit Reply
instead of Followup when sending v1, so it didn't reach the list).
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2775 bytes --]
From dd49ab3921744930b422dc408f44206055c94cae Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 12 Jan 2017 23:32:44 -0500
Subject: [PATCH v2] Avoid inefficient regex in diff-refine-hunk (Bug#25410)
* lisp/vc/diff-mode.el (diff--forward-while-leading-char): New function.
(diff-refine-hunk): Use it instead of trying to match multiple lines
with a single lines.
---
lisp/vc/diff-mode.el | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd94..b50b4a2 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2062,6 +2062,15 @@ diff-refine-preproc
(declare-function smerge-refine-subst "smerge-mode"
(beg1 end1 beg2 end2 props-c &optional preproc props-r props-a))
+(defun diff--forward-while-leading-char (char bound)
+ "Move point until reaching a line not starting with CHAR.
+Return new point, if it was moved."
+ (let ((pt nil))
+ (while (and (< (point) bound) (eql (following-char) char))
+ (forward-line 1)
+ (setq pt (point)))
+ pt))
+
(defun diff-refine-hunk ()
"Highlight changes of hunk at point at a finer granularity."
(interactive)
@@ -2081,16 +2090,18 @@ diff-refine-hunk
(goto-char beg)
(pcase style
(`unified
- (while (re-search-forward
- (eval-when-compile
- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
- "\\(\\)"
- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
- end t)
- (smerge-refine-subst (match-beginning 0) (match-end 1)
- (match-end 1) (match-end 0)
- nil 'diff-refine-preproc props-r props-a)))
+ (while (re-search-forward "^-" end t)
+ (let ((beg-del (progn (beginning-of-line) (point)))
+ beg-add end-add)
+ (when (and (diff--forward-while-leading-char ?- end)
+ ;; Allow for "\ No newline at end of file".
+ (progn (diff--forward-while-leading-char ?\\ end)
+ (setq beg-add (point)))
+ (diff--forward-while-leading-char ?+ end)
+ (progn (diff--forward-while-leading-char ?\\ end)
+ (setq end-add (point))))
+ (smerge-refine-subst beg-del beg-add beg-add end-add
+ nil 'diff-refine-preproc props-r props-a)))))
(`context
(let* ((middle (save-excursion (re-search-forward "^---")))
(other middle))
--
2.9.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-13 4:50 ` npostavs
@ 2017-01-13 6:14 ` Tino Calancha
2017-01-13 12:11 ` Tino Calancha
2017-01-13 16:31 ` Noam Postavsky
2017-01-19 1:55 ` npostavs
1 sibling, 2 replies; 12+ messages in thread
From: Tino Calancha @ 2017-01-13 6:14 UTC (permalink / raw)
To: npostavs; +Cc: 25410, Tino Calancha
On Thu, 12 Jan 2017, npostavs@users.sourceforge.net wrote:
>+ (while (re-search-forward "^-" end t)
>+ (let ((beg-del (progn (beginning-of-line) (point)))
>+ beg-add end-add)
>+ (when (and (diff--forward-while-leading-char ?- end)
>+ ;; Allow for "\ No newline at end of file".
>+ (progn (diff--forward-while-leading-char ?\\ end)
>+ (setq beg-add (point)))
>+ (diff--forward-while-leading-char ?+ end)
>+ (progn (diff--forward-while-leading-char ?\\ end)
>+ (setq end-add (point))))
>+ (smerge-refine-subst beg-del beg-add beg-add end-add
>+ nil 'diff-refine-preproc props-r props-a)))))
How about hide the complexity resulting for checking ?\\ inside the
auxiliary function?
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..e3487a722b 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2062,6 +2062,19 @@ diff-refine-preproc
(declare-function smerge-refine-subst "smerge-mode"
(beg1 end1 beg2 end2 props-c &optional preproc props-r props-a))
+(defun diff--forward-while-leading-char (char bound &optional ok-no-lf-eol)
+ "Move point until reaching a line not starting with CHAR.
+Return new point, if it was moved.
+If optional arg OK-NO-LF-EOL is non-nil, then allow no newline at end of line."
+ (let ((orig (point)))
+ (cl-labels ((fn (ch limit)
+ (while (and (< (point) limit) (eql (following-char) ch))
+ (forward-line 1))))
+ (progn
+ (fn char bound)
+ (when ok-no-lf-eol (fn ?\\ bound))
+ (unless (= orig (point)) (point))))))
+
(defun diff-refine-hunk ()
"Highlight changes of hunk at point at a finer granularity."
(interactive)
@@ -2081,16 +2094,13 @@ diff-refine-hunk
(goto-char beg)
(pcase style
(`unified
- (while (re-search-forward
- (eval-when-compile
- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
- "\\(\\)"
- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
- end t)
- (smerge-refine-subst (match-beginning 0) (match-end 1)
- (match-end 1) (match-end 0)
- nil 'diff-refine-preproc props-r props-a)))
+ (while (re-search-forward "^-" end t)
+ (let ((beg-del (progn (beginning-of-line) (point)))
+ (beg-add (diff--forward-while-leading-char ?- end 'no-lf-eol))
+ (end-add (diff--forward-while-leading-char ?+ end 'no-lf-eol)))
+ (when (and beg-add end-add)
+ (smerge-refine-subst beg-del beg-add beg-add end-add
+ nil 'diff-refine-preproc props-r props-a)))))
(`context
(let* ((middle (save-excursion (re-search-forward "^---")))
(other middle))
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-13 6:14 ` Tino Calancha
@ 2017-01-13 12:11 ` Tino Calancha
2017-01-13 16:31 ` Noam Postavsky
1 sibling, 0 replies; 12+ messages in thread
From: Tino Calancha @ 2017-01-13 12:11 UTC (permalink / raw)
To: 25410
On Fri, 13 Jan 2017, Tino Calancha wrote:
> + (while (re-search-forward "^-" end t)
> + (let ((beg-del (progn (beginning-of-line) (point)))
> + (beg-add (diff--forward-while-leading-char ?- end 'no-lf-eol))
> + (end-add (diff--forward-while-leading-char ?+ end 'no-lf-eol)))
> + (when (and beg-add end-add)
> + (smerge-refine-subst beg-del beg-add beg-add end-add
> + nil 'diff-refine-preproc props-r props-a)))))
This is symmetrical respect beg-add/end-add but we could save a
`diff--forward-while-leading-char' call if beg-add is nil.
I think the following lazy version is better:
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..d550a59d6d 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2062,6 +2062,19 @@ diff-refine-preproc
(declare-function smerge-refine-subst "smerge-mode"
(beg1 end1 beg2 end2 props-c &optional preproc props-r props-a))
+(defun diff--forward-while-leading-char (char bound &optional ok-no-lf-eol)
+ "Move point until reaching a line not starting with CHAR.
+Return new point, if it was moved.
+If optional arg OK-NO-LF-EOL is non-nil, then allow no newline at end of line."
+ (let ((orig (point)))
+ (cl-labels ((fn (ch limit)
+ (while (and (< (point) limit) (eql (following-char) ch))
+ (forward-line 1))))
+ (progn
+ (fn char bound)
+ (when ok-no-lf-eol (fn ?\\ bound))
+ (unless (= orig (point)) (point))))))
+
(defun diff-refine-hunk ()
"Highlight changes of hunk at point at a finer granularity."
(interactive)
@@ -2081,16 +2094,14 @@ diff-refine-hunk
(goto-char beg)
(pcase style
(`unified
- (while (re-search-forward
- (eval-when-compile
- (let ((no-LF-at-eol-re "\\(?:\\\\.*\n\\)?"))
- (concat "^\\(?:-.*\n\\)+" no-LF-at-eol-re
- "\\(\\)"
- "\\(?:\\+.*\n\\)+" no-LF-at-eol-re)))
- end t)
- (smerge-refine-subst (match-beginning 0) (match-end 1)
- (match-end 1) (match-end 0)
- nil 'diff-refine-preproc props-r props-a)))
+ (while (re-search-forward "^-" end t)
+ (let* ((beg-del (progn (beginning-of-line) (point)))
+ (beg-add (diff--forward-while-leading-char ?- end t))
+ (end-add (and beg-add
+ (diff--forward-while-leading-char ?+ end t))))
+ (when end-add
+ (smerge-refine-subst beg-del beg-add beg-add end-add
+ nil 'diff-refine-preproc props-r props-a)))))
(`context
(let* ((middle (save-excursion (re-search-forward "^---")))
(other middle))
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-13 6:14 ` Tino Calancha
2017-01-13 12:11 ` Tino Calancha
@ 2017-01-13 16:31 ` Noam Postavsky
2017-01-14 5:38 ` Tino Calancha
1 sibling, 1 reply; 12+ messages in thread
From: Noam Postavsky @ 2017-01-13 16:31 UTC (permalink / raw)
To: Tino Calancha; +Cc: 25410
On Fri, Jan 13, 2017 at 1:14 AM, Tino Calancha <tino.calancha@gmail.com> wrote:
>
>> + (while (re-search-forward "^-" end t)
>> + (let ((beg-del (progn (beginning-of-line) (point)))
>> + beg-add end-add)
>> + (when (and (diff--forward-while-leading-char ?- end)
>> + ;; Allow for "\ No newline at end of file".
>> + (progn (diff--forward-while-leading-char ?\\ end)
>> + (setq beg-add (point)))
>> + (diff--forward-while-leading-char ?+ end)
>> + (progn (diff--forward-while-leading-char ?\\ end)
>> + (setq end-add (point))))
>
> How about hide the complexity resulting for checking ?\\ inside the
> auxiliary function?
I'm okay with doing this, but I slightly prefer leaving the complexity
at the top level. I think pushing the ?\\ check inside
diff--forward-while-leading-char makes that function's purpose a bit
incoherent and the complexity reduction in the caller doesn't look
significant enough to balance that.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-13 16:31 ` Noam Postavsky
@ 2017-01-14 5:38 ` Tino Calancha
0 siblings, 0 replies; 12+ messages in thread
From: Tino Calancha @ 2017-01-14 5:38 UTC (permalink / raw)
To: Noam Postavsky; +Cc: 25410, Tino Calancha
On Fri, 13 Jan 2017, Noam Postavsky wrote:
> On Fri, Jan 13, 2017 at 1:14 AM, Tino Calancha <tino.calancha@gmail.com> wrote:
>>
>>> + (while (re-search-forward "^-" end t)
>>> + (let ((beg-del (progn (beginning-of-line) (point)))
>>> + beg-add end-add)
>>> + (when (and (diff--forward-while-leading-char ?- end)
>>> + ;; Allow for "\ No newline at end of file".
>>> + (progn (diff--forward-while-leading-char ?\\ end)
>>> + (setq beg-add (point)))
>>> + (diff--forward-while-leading-char ?+ end)
>>> + (progn (diff--forward-while-leading-char ?\\ end)
>>> + (setq end-add (point))))
>>
>> How about hide the complexity resulting for checking ?\\ inside the
>> auxiliary function?
>
> I'm okay with doing this, but I slightly prefer leaving the complexity
> at the top level. I think pushing the ?\\ check inside
> diff--forward-while-leading-char makes that function's purpose a bit
> incoherent and the complexity reduction in the caller doesn't look
> significant enough to balance that.
Agreed. Indeed, keeping simple `diff--forward-while-leading-char'
favours reutilization: it seems to me like this function could be used
elsewhere.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
2017-01-13 4:50 ` npostavs
2017-01-13 6:14 ` Tino Calancha
@ 2017-01-19 1:55 ` npostavs
1 sibling, 0 replies; 12+ messages in thread
From: npostavs @ 2017-01-19 1:55 UTC (permalink / raw)
To: Tino Calancha; +Cc: 25410
tags 25410 fixed
close 25410 26.1
quit
npostavs@users.sourceforge.net writes:
> Subject: [PATCH v2] Avoid inefficient regex in diff-refine-hunk (Bug#25410)
Pushed to master [1: 8c0fcaf]
1: 2017-01-18 20:37:31 -0500 8c0fcaf66733f0538a3f024f383cb34a3c93d73c
Avoid inefficient regex in diff-refine-hunk (Bug#25410)
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-01-19 1:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-10 10:08 bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines Tino Calancha
2017-01-10 14:22 ` npostavs
2017-01-10 15:07 ` Tino Calancha
2017-01-11 2:49 ` Tino Calancha
2017-01-11 8:13 ` Tino Calancha
[not found] ` <87wpe0zz0s.fsf@users.sourceforge.net>
2017-01-12 5:32 ` Tino Calancha
2017-01-13 4:50 ` npostavs
2017-01-13 6:14 ` Tino Calancha
2017-01-13 12:11 ` Tino Calancha
2017-01-13 16:31 ` Noam Postavsky
2017-01-14 5:38 ` Tino Calancha
2017-01-19 1:55 ` npostavs
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).