* 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
[parent not found: <87wpe0zz0s.fsf@users.sourceforge.net>]
* 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).