all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* 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 external index

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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.