bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#17544: 24.3; [PATCH] Improved diff-mode navigation/manipulation


From: Dima Kogan
Subject: bug#17544: 24.3; [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 16 Nov 2016 20:15:34 -0800
User-agent: mu4e 0.9.17; emacs 26.0.50.1

New patch attached.

>From b02085abd998d2c1e1519973964295fb5b798e0c Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan@jpl.nasa.gov>
Date: Wed, 14 Sep 2016 15:25:06 -0700
Subject: [PATCH] Improved diff-mode navigation/manipulation

Navigation and use of diff buffers had several annoying corner cases that this
patch fixes. These corner cases were largely due to inconsistent treatment of
file headers. Say you have a diff such as this:

 --- aaa
 +++ bbb
 @@ -52,7 +52,7 @@
 hunk1
 @@ -74,7 +74,7 @@
 hunk2
 --- ccc
 +++ ddd
 @@ -608,6 +608,6 @@
 hunk3
 @@ -654,7 +654,7 @@
 hunk4

The file headers here are the '---' and '+++' lines. With the point on such a
line, hunk operations would sometimes refer to the next hunk and sometimes to
the previous hunk. Most of the time it would be the previous hunk, which is
not what the user would expect. This patch consistently treats such headers as
the next hunk. So with this patch, if the point is on the '--- ccc' line, the
point is seen as referring to hunk3.

Specific behaviors this fixes are:

1. It should be possible to place the point in the middle of a diff buffer,
and press M-k repeatedly to kill hunks in the order they appear in the buffer.
With the point on hunk1, M-k M-k would kill hunk1 then hunk2. With the point
on hunk3, it would kill hunk3 then hunk4; this is fine. However, with the
point on hunk2, it'd kill hunk2 then hunk1. This is fixed by this patch.

2. Similarly, it should be possible to apply hunks in order. Previously with
the point at the start, C-c C-a would apply the hunk1, then move the point to
the first @@ header, and thus C-c C-a would try to apply the same hunk again.
---
 lisp/vc/diff-mode.el | 117 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 13 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..4f05b0c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -551,7 +551,7 @@ diff--auto-refine-data
 
 ;; Define diff-{hunk,file}-{prev,next}
 (easy-mmode-define-navigation
- diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
+ diff--internal-hunk diff-hunk-header-re "hunk" diff-end-of-hunk 
diff-restrict-view
  (when diff-auto-refine-mode
    (unless (prog1 diff--auto-refine-data
              (setq diff--auto-refine-data
@@ -570,7 +570,88 @@ diff--auto-refine-data
                                 (diff-refine-hunk))))))))))))
 
 (easy-mmode-define-navigation
- diff-file diff-file-header-re "file" diff-end-of-file)
+ diff--internal-file diff-file-header-re "file" diff-end-of-file)
+
+(defun diff--wrap-navigation (skip-hunk-start
+                              what orig
+                              header-re goto-start-func count)
+  "I override the default diff-{hunk,file}-{next,prev}
+implementation by skipping any lines that are associated with
+this hunk/file but precede the hunk-start marker. For instance, a
+diff file could contain
+
+diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
+index 923de9a..6b1c24f 100644
+--- a/lisp/vc/diff-mode.el
++++ b/lisp/vc/diff-mode.el
+@@ -590,6 +590,22 @@
+.......
+
+If a point is on 'index', then the point is considered to be in
+this first hunk. Here I move the point to the @@... marker before
+executing the default diff-hunk-next/prev implementation to move
+to the NEXT marker"
+  (if (not skip-hunk-start)
+      (funcall orig count)
+
+    (let ((start (point)))
+      (funcall goto-start-func)
+
+      ;; I trap the error
+      (condition-case nil
+          (funcall orig count)
+        (error nil))
+
+      (when (not (looking-at header-re))
+        (goto-char start)
+        (user-error (format "No %s" what))))))
+
+(defun diff-hunk-prev (&optional count skip-hunk-start)
+  "Go to the previous COUNT'th hunk"
+  (interactive (list (prefix-numeric-value current-prefix-arg) t))
+  (diff--wrap-navigation
+   skip-hunk-start
+   "prev hunk"
+   'diff--internal-hunk-prev
+   diff-hunk-header-re
+   (lambda () (goto-char (car (diff-bounds-of-hunk))))
+   count))
+
+(defun diff-hunk-next (&optional count skip-hunk-start)
+  "Go to the next COUNT'th hunk"
+  (interactive (list (prefix-numeric-value current-prefix-arg) t))
+  (diff--wrap-navigation
+   skip-hunk-start
+   "next hunk"
+   'diff--internal-hunk-next
+   diff-hunk-header-re
+   (lambda () (goto-char (car (diff-bounds-of-hunk))))
+   count))
+
+(defun diff-file-prev (&optional count skip-hunk-start)
+  "Go to the previous COUNT'th file"
+  (interactive (list (prefix-numeric-value current-prefix-arg) t))
+  (diff--wrap-navigation
+   skip-hunk-start
+   "prev file"
+   'diff--internal-file-prev
+   diff-file-header-re
+   (lambda () (goto-char (car (diff-bounds-of-file))) 
(diff--internal-hunk-next))
+   count))
+
+(defun diff-file-next (&optional count skip-hunk-start)
+  "Go to the next COUNT'th file"
+  (interactive (list (prefix-numeric-value current-prefix-arg) t))
+  (diff--wrap-navigation
+   skip-hunk-start
+   "next file"
+   'diff--internal-file-next
+   diff-file-header-re
+   (lambda () (goto-char (car (diff-bounds-of-file))) 
(diff--internal-hunk-next))
+   count))
+
+
+
 
 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
@@ -581,12 +662,13 @@ diff-bounds-of-hunk
     (let ((pos (point))
          (beg (diff-beginning-of-hunk t))
          (end (diff-end-of-hunk)))
-      (cond ((>= end pos)
+      (cond ((> end pos)
             (list beg end))
            ;; If this hunk ends above POS, consider the next hunk.
            ((re-search-forward diff-hunk-header-re nil t)
             (list (match-beginning 0) (diff-end-of-hunk)))
-           (t (error "No hunk found"))))))
+           ;; There's no next hunk, so just take the one we have
+           (t (list beg end))))))
 
 (defun diff-bounds-of-file ()
   "Return the bounds of the file segment at point.
@@ -1665,8 +1747,9 @@ diff-find-source-location
 SWITCHED is non-nil if the patch is already applied.
 NOPROMPT, if non-nil, means not to prompt the user."
   (save-excursion
-    (let* ((other (diff-xor other-file diff-jump-to-old-file))
-          (char-offset (- (point) (diff-beginning-of-hunk t)))
+    (let* ((hunk-bounds (diff-bounds-of-hunk))
+           (other (diff-xor other-file diff-jump-to-old-file))
+           (char-offset (- (point) (goto-char (car hunk-bounds))))
            ;; Check that the hunk is well-formed.  Otherwise diff-mode and
            ;; the user may disagree on what constitutes the hunk
            ;; (e.g. because an empty line truncates the hunk mid-course),
@@ -1675,7 +1758,7 @@ diff-find-source-location
           ;; Suppress check when NOPROMPT is non-nil (Bug#3033).
            (_ (unless noprompt (diff-sanity-check-hunk)))
           (hunk (buffer-substring
-                  (point) (save-excursion (diff-end-of-hunk) (point))))
+                  (point) (cadr hunk-bounds)))
           (old (diff-hunk-text hunk reverse char-offset))
           (new (diff-hunk-text hunk (not reverse) char-offset))
           ;; Find the location specification.
@@ -1783,8 +1866,15 @@ diff-apply-hunk
       ;; Display BUF in a window
       (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
       (diff-hunk-status-msg line-offset (diff-xor switched reverse) nil)
+
+      ;; I advance to the next hunk with skip-hunk-start set to t
+      ;; because I want the behavior of moving to the next logical
+      ;; hunk, not the legacy behavior where were would sometimes stay
+      ;; on the curent hunk.  This is the behavior we get when
+      ;; navigating through hunks interactively, and we want it when
+      ;; applying hunks too.  See http://debbugs.gnu.org/17544
       (when diff-advance-after-apply-hunk
-       (diff-hunk-next))))))
+        (diff-hunk-next nil t))))))
 
 
 (defun diff-test-hunk (&optional reverse)
@@ -1865,14 +1955,15 @@ diff-current-defun
 (defun diff-ignore-whitespace-hunk ()
   "Re-diff the current hunk, ignoring whitespace differences."
   (interactive)
-  (let* ((char-offset (- (point) (diff-beginning-of-hunk t)))
+  (let* ((hunk-bounds (diff-bounds-of-hunk))
+         (char-offset (- (point) (goto-char (car hunk-bounds))))
         (opts (pcase (char-after) (?@ "-bu") (?* "-bc") (_ "-b")))
         (line-nb (and (or (looking-at "[^0-9]+\\([0-9]+\\)")
                           (error "Can't find line number"))
                       (string-to-number (match-string 1))))
         (inhibit-read-only t)
         (hunk (delete-and-extract-region
-               (point) (save-excursion (diff-end-of-hunk) (point))))
+               (point) (cadr hunk-bounds)))
         (lead (make-string (1- line-nb) ?\n)) ;Line nums start at 1.
         (file1 (make-temp-file "diff1"))
         (file2 (make-temp-file "diff2"))
@@ -1959,8 +2050,8 @@ diff-refine-hunk
   (interactive)
   (require 'smerge-mode)
   (save-excursion
-    (diff-beginning-of-hunk t)
-    (let* ((start (point))
+    (let* ((hunk-bounds (diff-bounds-of-hunk))
+           (start (goto-char (car hunk-bounds)))
            (style (diff-hunk-style))    ;Skips the hunk header as well.
            (beg (point))
            (props-c '((diff-mode . fine) (face diff-refine-changed)))
@@ -1968,7 +2059,7 @@ diff-refine-hunk
            (props-a '((diff-mode . fine) (face diff-refine-added)))
            ;; Be careful to go back to `start' so diff-end-of-hunk gets
            ;; to read the hunk header's line info.
-           (end (progn (goto-char start) (diff-end-of-hunk) (point))))
+           (end (goto-char (cadr hunk-bounds))))
 
       (remove-overlays beg end 'diff-mode 'fine)
 
-- 
2.8.0.rc3

npostavs@users.sourceforge.net writes:

> Dima Kogan <dima@secretsauce.net> writes:
>
>> npostavs@users.sourceforge.net writes:
>>
>> Revised patch attached. I'll try to respond to any comments
>> quickly so that we all can remember what this is about.
> [...]
>> -        (t (error "No hunk found"))))))
>> +            ;; There's no next hunk, so just take the one we have
>> +        (t (list beg end))))))
>
> Indentation on the comment looks a bit off.

This file used tabs in some places and spaces in others. Indentation
updated to match.


>> +
>> +      ;; I advance to the next hunk interactively because I want the
>> +      ;; interactive behavior of moving to the next logical hunk, not
>> +      ;; the legacy behavior where were would sometimes sty on the
>> +      ;; curent hunk.  See http://debbugs.gnu.org/17544
>>        (when diff-advance-after-apply-hunk
>> -    (diff-hunk-next))))))
>> +        (diff-hunk-next nil t))))))
>
> Updating the comment here will be useful for the next person trying to
> figure out what this is all about in a couple more years.

done


>>       (hunk (delete-and-extract-region
>> -            (point) (save-excursion (diff-end-of-hunk) (point))))
>> +                (point) (cadr hunk-bounds)))
>
> Indentation looks off here.

Same as above.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]