bug#25400: bug#25105: bug#25400: M-p in diff-mode jumps too far

[Tino Calancha]

On Thu, 12 Jan 2017, Dmitry Gutov wrote:

> I'd prefer two separate commits:
> one reverting 2c8a7e5 (if that is what we are doing),
> and one with your actual changes.
>
> That would make the history easier to read, and would probably improve 'git 
> blame' results as well.
Definitely.  Thanks for the suggestion!

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> From 4c0b6cb87438a5c812a4718452c2537d827a74a4 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Thu, 12 Jan 2017 12:46:03 +0900
Subject: [PATCH 1/2] ; Revert "Improve diff-mode navigation/manipulation"

This reverts commit 2c8a7e50d24daf19ea7d86f1cfeaa98a41c56085.
This change causes regressions:
https://lists.gnu.org/archive/html/emacs-devel/2016-11/msg00738.html

Fixes: debbugs:25105, 25400.
---
 lisp/vc/diff-mode.el | 174 ++++++++++-----------------------------------------
 1 file changed, 34 insertions(+), 140 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..44556ddd4a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -551,124 +551,26 @@ diff--auto-refine-data

 ;; Define diff-{hunk,file}-{prev,next}
 (easy-mmode-define-navigation
- diff--internal-hunk diff-hunk-header-re "hunk" diff-end-of-hunk 
diff-restrict-view)
+ diff-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
+                   (cons (current-buffer) (point-marker))))
+     (run-at-time 0.0 nil
+                  (lambda ()
+                    (when diff--auto-refine-data
+                      (let ((buffer (car diff--auto-refine-data))
+                            (point (cdr diff--auto-refine-data)))
+                        (setq diff--auto-refine-data nil)
+                        (with-local-quit
+                          (when (buffer-live-p buffer)
+                            (with-current-buffer buffer
+                              (save-excursion
+                                (goto-char point)
+                                (diff-refine-hunk))))))))))))

 (easy-mmode-define-navigation
- 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)
-  "Wrap diff-{hunk,file}-{next,prev} for more intuitive behavior.
-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.  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)
-
-      ;; 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)))
-
-      ;; We successfully moved to the next/prev hunk/file. Apply the
-      ;; auto-refinement if needed
-      (when diff-auto-refine-mode
-        (unless (prog1 diff--auto-refine-data
-                  (setq diff--auto-refine-data
-                        (cons (current-buffer) (point-marker))))
-          (run-at-time 0.0 nil
-                       (lambda ()
-                         (when diff--auto-refine-data
-                           (let ((buffer (car diff--auto-refine-data))
-                                 (point (cdr diff--auto-refine-data)))
-                             (setq diff--auto-refine-data nil)
-                             (with-local-quit
-                               (when (buffer-live-p buffer)
-                                 (with-current-buffer buffer
-                                   (save-excursion
-                                     (goto-char point)
-                                     (diff-refine-hunk))))))))))))))
-
-;; These functions all take a skip-hunk-start argument which controls
-;; whether we skip pre-hunk-start text or not.  In interactive uses we
-;; always want to do this, but the simple behavior is still necessary
-;; to, for example, avoid an infinite loop:
-;;
-;;   diff-hunk-next         calls
-;;   diff--wrap-navigation  calls
-;;   diff-bounds-of-hunk    calls
-;;   diff-beginning-of-hunk calls
-;;   diff-hunk-next
-;;
-;; Here the outer diff-hunk-next has skip-hunk-start set to t, but the
-;; inner one does not, which breaks the loop.
-(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))
-
-
-
+ diff-file diff-file-header-re "file" diff-end-of-file)

 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
@@ -679,13 +581,12 @@ 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)))
-           ;; There's no next hunk, so just take the one we have.
-           (t (list beg end))))))
+           (t (error "No hunk found"))))))

 (defun diff-bounds-of-file ()
   "Return the bounds of the file segment at point.
@@ -771,7 +672,7 @@ diff-beginning-of-file-and-junk
         (setq prevfile nextfile))
     (if (and previndex (numberp prevfile) (< previndex prevfile))
         (setq prevfile previndex))
-    (if (numberp prevfile)
+    (if (and (numberp prevfile) (<= prevfile start))
           (progn
             (goto-char prevfile)
             ;; Now skip backward over the leading junk we may have before the
@@ -1764,9 +1665,8 @@ 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* ((hunk-bounds (diff-bounds-of-hunk))
-           (other (diff-xor other-file diff-jump-to-old-file))
-           (char-offset (- (point) (goto-char (car hunk-bounds))))
+    (let* ((other (diff-xor other-file diff-jump-to-old-file))
+          (char-offset (- (point) (diff-beginning-of-hunk t)))
            ;; 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),
@@ -1775,7 +1675,7 @@ diff-find-source-location
           ;; Suppress check when NOPROMPT is non-nil (Bug#3033).
            (_ (unless noprompt (diff-sanity-check-hunk)))
           (hunk (buffer-substring
-                  (point) (cadr hunk-bounds)))
+                  (point) (save-excursion (diff-end-of-hunk) (point))))
           (old (diff-hunk-text hunk reverse char-offset))
           (new (diff-hunk-text hunk (not reverse) char-offset))
           ;; Find the location specification.
@@ -1883,15 +1783,8 @@ 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)
-
-      ;; Advance to the next hunk with skip-hunk-start set to t
-      ;; because we want the behavior of moving to the next logical
-      ;; hunk, not the original behavior where were would sometimes
-      ;; stay on the current 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 nil t))))))
+       (diff-hunk-next))))))


 (defun diff-test-hunk (&optional reverse)
@@ -1972,15 +1865,14 @@ diff-current-defun
 (defun diff-ignore-whitespace-hunk ()
   "Re-diff the current hunk, ignoring whitespace differences."
   (interactive)
-  (let* ((hunk-bounds (diff-bounds-of-hunk))
-         (char-offset (- (point) (goto-char (car hunk-bounds))))
+  (let* ((char-offset (- (point) (diff-beginning-of-hunk t)))
         (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) (cadr hunk-bounds)))
+               (point) (save-excursion (diff-end-of-hunk) (point))))
         (lead (make-string (1- line-nb) ?\n)) ;Line nums start at 1.
         (file1 (make-temp-file "diff1"))
         (file2 (make-temp-file "diff2"))
@@ -2067,14 +1959,16 @@ diff-refine-hunk
   (interactive)
   (require 'smerge-mode)
   (save-excursion
-    (let* ((hunk-bounds (diff-bounds-of-hunk))
-           (style (progn (goto-char (car hunk-bounds))
-                         (diff-hunk-style))) ;Skips the hunk header as well.
+    (diff-beginning-of-hunk t)
+    (let* ((start (point))
+           (style (diff-hunk-style))    ;Skips the hunk header as well.
            (beg (point))
-           (end (cadr hunk-bounds))
            (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))))
+           (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))))

       (remove-overlays beg end 'diff-mode 'fine)

--
2.11.0

> From a5809529fac90741c6ede3fc66dfdac1e011434c Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Thu, 12 Jan 2017 12:46:25 +0900
Subject: [PATCH 2/2] Fix Bug#17544

* lisp/vc/diff-mode.el (diff-file-junk-re):
Move definition before it's used.
(diff--at-diff-header-p): New predicate.
(diff-beginning-of-hunk): Use it.
(diff-apply-hunk): Jump to beginning of hunk before apply the hunk.
(diff-hunk-kill, diff-file-kill): Jump to beginning of hunk after kill.
(diff-post-command-hook): Call diff-beginning-of-hunk with non-nil argument.
---
 lisp/vc/diff-mode.el | 67 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 44556ddd4a..3fc4713f0f 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -498,22 +498,55 @@ diff-end-of-hunk
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))

+;; "index ", "old mode", "new mode", "new file mode" and
+;; "deleted file mode" are output by git-diff.
+(defconst diff-file-junk-re
+  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== 
modified file")
+
+;; If point is in a diff header, then return beginning
+;; of hunk position otherwise return nil.
+(defun diff--at-diff-header-p ()
+  "Return non-nil if point is inside a diff header."
+  (let ((regexp-hunk diff-hunk-header-re)
+        (regexp-file diff-file-header-re)
+        (regexp-junk diff-file-junk-re)
+        (orig (point)))
+    (catch 'headerp
+      (save-excursion
+        (forward-line 0)
+        (when (looking-at regexp-hunk) ; Hunk header.
+          (throw 'headerp (point)))
+        (forward-line -1)
+        (when (re-search-forward regexp-file (point-at-eol 4) t) ; File header.
+          (forward-line 0)
+          (throw 'headerp (point)))
+        (goto-char orig)
+        (forward-line 0)
+        (when (looking-at regexp-junk) ; Git diff junk.
+          (while (and (looking-at regexp-junk)
+                      (not (bobp)))
+            (forward-line -1))
+          (re-search-forward regexp-file nil t)
+          (forward-line 0)
+          (throw 'headerp (point)))) nil)))
+
 (defun diff-beginning-of-hunk (&optional try-harder)
   "Move back to the previous hunk beginning, and return its position.
 If point is in a file header rather than a hunk, advance to the
 next hunk if TRY-HARDER is non-nil; otherwise signal an error."
   (beginning-of-line)
-  (if (looking-at diff-hunk-header-re)
+  (if (looking-at diff-hunk-header-re) ; At hunk header.
       (point)
-    (forward-line 1)
-    (condition-case ()
-       (re-search-backward diff-hunk-header-re)
-      (error
-       (unless try-harder
-        (error "Can't find the beginning of the hunk"))
-       (diff-beginning-of-file-and-junk)
-       (diff-hunk-next)
-       (point)))))
+    (let ((pos (diff--at-diff-header-p))
+          (regexp diff-hunk-header-re))
+      (cond (pos ; At junk diff header.
+             (if try-harder
+                 (goto-char pos)
+               (error "Can't find the beginning of the hunk")))
+            ((re-search-backward regexp nil t)) ; In the middle of a hunk.
+            ((re-search-forward regexp nil t) ; At first hunk header.
+             (forward-line 0))
+            (t (error "Can't find the beginning of the hunk"))))))

 (defun diff-unified-hunk-p ()
   (save-excursion
@@ -632,12 +665,8 @@ diff-hunk-kill
                   hunk-bounds))
         (inhibit-read-only t))
     (apply 'kill-region bounds)
-    (goto-char (car bounds))))
-
-;; "index ", "old mode", "new mode", "new file mode" and
-;; "deleted file mode" are output by git-diff.
-(defconst diff-file-junk-re
-  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== 
modified file")
+    (goto-char (car bounds))
+    (diff-beginning-of-hunk t)))

 (defun diff-beginning-of-file-and-junk ()
   "Go to the beginning of file-related diff-info.
@@ -690,7 +719,8 @@ diff-file-kill
   "Kill current file's hunks."
   (interactive)
   (let ((inhibit-read-only t))
-    (apply 'kill-region (diff-bounds-of-file))))
+    (apply 'kill-region (diff-bounds-of-file)))
+  (diff-beginning-of-hunk t))

 (defun diff-kill-junk ()
   "Kill spurious empty diffs."
@@ -1274,7 +1304,7 @@ diff-post-command-hook
        ;; it's safer not to do it on big changes, e.g. when yanking a big
        ;; diff, or when the user edits the header, since we might then
        ;; screw up perfectly correct values.  --Stef
-       (diff-beginning-of-hunk)
+       (diff-beginning-of-hunk t)
         (let* ((style (if (looking-at "\\*\\*\\*") 'context))
                (start (line-beginning-position (if (eq style 'context) 3 2)))
                (mid (if (eq style 'context)
@@ -1738,6 +1768,7 @@ diff-apply-hunk

 With a prefix argument, REVERSE the hunk."
   (interactive "P")
+  (diff-beginning-of-hunk t)
   (pcase-let ((`(,buf ,line-offset ,pos ,old ,new ,switched)
                ;; Sometimes we'd like to have the following behavior: if
                ;; REVERSE go to the new file, otherwise go to the old.
--
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.5)
 of 2017-01-12
Repository revision: d40073f017ffb3dee2266f356c127ef587c40b71