GNU bug report logs - #25410
26.0.50; Refine an unified diff hunk only if adds lines

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25410 in the body.
You can then email your comments to 25410 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Tue, 10 Jan 2017 19:08:20 +0900

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

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

From: npostavs <at> users.sourceforge.net
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25410 <at> debbugs.gnu.org
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Tue, 10 Jan 2017 09:22:36 -0500

Tino Calancha <tino.calancha <at> 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?

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: 25410 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Wed, 11 Jan 2017 00:07:28 +0900 (JST)

On Tue, 10 Jan 2017, npostavs <at> users.sourceforge.net wrote:

> Tino Calancha <tino.calancha <at> 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 <at> 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.

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: 25410 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Wed, 11 Jan 2017 11:49:45 +0900

npostavs <at> users.sourceforge.net writes:

> Tino Calancha <tino.calancha <at> 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 <at> 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

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: 25410 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Wed, 11 Jan 2017 17:13:02 +0900

Tino Calancha <tino.calancha <at> gmail.com> writes:

> npostavs <at> 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 <at> 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

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: 25410 <at> debbugs.gnu.org
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Thu, 12 Jan 2017 14:32:02 +0900 (JST)

On Wed, 11 Jan 2017, npostavs <at> users.sourceforge.net wrote:

> Tino Calancha <tino.calancha <at> 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 <at> 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
>

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

From: npostavs <at> users.sourceforge.net
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25410 <at> debbugs.gnu.org
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Thu, 12 Jan 2017 23:50:40 -0500

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

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

[v2-0001-Avoid-inefficient-regex-in-diff-refine-hunk-Bug-2.patch (text/x-diff, inline)]

From dd49ab3921744930b422dc408f44206055c94cae Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> 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

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: 25410 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Fri, 13 Jan 2017 15:14:04 +0900 (JST)

On Thu, 12 Jan 2017, npostavs <at> 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))

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 25410 <at> debbugs.gnu.org
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Fri, 13 Jan 2017 21:11:20 +0900 (JST)

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

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25410 <at> debbugs.gnu.org
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Fri, 13 Jan 2017 11:31:10 -0500

On Fri, Jan 13, 2017 at 1:14 AM, Tino Calancha <tino.calancha <at> 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.

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 25410 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Sat, 14 Jan 2017 14:38:36 +0900 (JST)

On Fri, 13 Jan 2017, Noam Postavsky wrote:

> On Fri, Jan 13, 2017 at 1:14 AM, Tino Calancha <tino.calancha <at> 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.

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

From: npostavs <at> users.sourceforge.net
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25410 <at> debbugs.gnu.org
Subject: Re: bug#25410: 26.0.50; Refine an unified diff hunk only if adds lines
Date: Wed, 18 Jan 2017 20:55:07 -0500

tags 25410 fixed
close 25410 26.1
quit

npostavs <at> 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)

Previous Next

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