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

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

[Stefan Monnier]

Package: Emacs
Version: 26.0.50

In a buffer with more than one hunk, if I'm in the middle of hunk number
N, diff-hunk-prev (usually bound to M-p) jumps to the header of hunk
number N-1 rather than to the header of hunk N.

This is contrary to the usual behavior of Emacs's navigation commands.


        Stefan



In GNU Emacs 26.0.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.31)
 of 2017-01-06 built on ceviche
Repository revision: 074618e4af08e8d818c471f90c6de9b895980e05
Windowing system distributor 'The X.Org Foundation', version 11.0.11802000
System Description:	Debian GNU/Linux testing (stretch)

Recent messages:
~/src/elisp/tuareg-mode/tuareg.el and /home/monnier/src/emacs/elpa/packages/tuareg/tuareg.el are the same file
Hunk applied
Saving file /home/monnier/src/emacs/elpa/packages/tuareg/tuareg.el...
Wrote /home/monnier/src/emacs/elpa/packages/tuareg/tuareg.el
Invalid face reference: change-log-date-face [4 times]
Warning: turn-on-eldoc-mode is obsolete!
Warning: hide-sublevels is obsolete!
Invalid face reference: change-log-date-face [2 times]
Finding changes in /home/monnier/src/emacs/trunk/lisp/textmodes/rst.el...done
Invalid face reference: change-log-date-face [24 times]

Configured using:
 'configure -C --enable-checking --enable-check-lisp-object-type
 'CFLAGS=-Wall -g3 -Og -Wno-pointer-sign'
 PKG_CONFIG_PATH=/home/monnier/lib/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND GPM DBUS GCONF GSETTINGS NOTIFY GNUTLS
LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS GTK2 X11

Important settings:
  value of $LANG: fr_CH.UTF-8
  locale-coding-system: utf-8-unix

Major mode: InactiveMinibuffer

Minor modes in effect:
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  electric-pair-mode: t
  global-reveal-mode: t
  reveal-mode: t
  auto-insert-mode: t
  savehist-mode: t
  minibuffer-electric-default-mode: t
  global-compact-docstrings-mode: t
  url-handler-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  global-prettify-symbols-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/monnier/src/emacs/elpa/packages/svg/svg hides /home/monnier/src/emacs/work/lisp/svg
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-xref hides /home/monnier/src/emacs/work/lisp/progmodes/ada-xref
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-mode hides /home/monnier/src/emacs/work/lisp/progmodes/ada-mode
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-stmt hides /home/monnier/src/emacs/work/lisp/progmodes/ada-stmt
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-prj hides /home/monnier/src/emacs/work/lisp/progmodes/ada-prj
/home/monnier/src/emacs/elpa/packages/landmark/landmark hides /home/monnier/src/emacs/work/lisp/obsolete/landmark
/home/monnier/src/emacs/elpa/packages/crisp/crisp hides /home/monnier/src/emacs/work/lisp/obsolete/crisp

Features:
(sort mail-extr emacsbug whitespace epa-file log-edit message sendmail
puny rfc822 mml mml-sec epa epg gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils mailheader ffap vc-bzr
vc-src vc-sccs vc-svn vc-cvs vc-rcs vc-dir all hippie-exp ocamldebug
derived caml tuareg_indent tuareg caml-help caml-types caml-emacs
completion hibtypes hsys-www browse-url klink hib-kbd hib-debbugs
hib-social hsys-org org org-macro org-footnote org-pcomplete org-list
org-faces org-entities org-version ob-emacs-lisp ob ob-tangle ob-ref
ob-lob ob-table ob-exp org-src ob-keys ob-comint ob-core ob-eval
org-compat org-macs org-loaddefs hactypes hmail hargs hypb locate hbut
hact hpath hui-select hvar set hhist hbdata htz cal-julian cal-menu
calendar cal-loaddefs hbmap hmoccur hversion hload-path rect dabbrev
grep compile smerge-mode add-log log-view pcvs-util vc vc-dispatcher map
eieio-opt speedbar sb-image ezimage dframe help-fns radix-tree xscheme
warnings unsafep trace testcover shadow scheme re-builder profiler
inf-lisp ielm gmm-utils ert pp find-func ewoc debug elp edebug cl-indent
misearch multi-isearch css-mode smie autorevert filenotify doc-view
jka-compr image-mode dired dired-loaddefs executable copyright files-x
tramp-cache tramp-sh tramp tramp-compat tramp-loaddefs trampver
ucs-normalize shell pcomplete comint ansi-color ring parse-time
format-spec html5-schema cus-edit cus-start cus-load wid-edit vc-git
diff-mode filecache rng-xsd xsd-regexp rng-cmpct rng-nxml rng-valid
rng-loc rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn
nxml-ns nxml-mode nxml-outln nxml-rap sgml-mode subr-x dom nxml-util
nxml-enc xmltok server time-date noutline outline easy-mmode flyspell
ispell checkdoc thingatpt load-dir elec-pair reveal autoinsert
proof-site proof-autoloads cl pg-vars savehist minibuf-eldef disp-table
compact-docstrings kotl-loaddefs advice info realgud-recursive-autoloads
finder-inf url-auth package epg-config url-handlers url-parse
auth-source eieio eieio-core cl-macs eieio-loaddefs password-cache
url-vars seq byte-opt gv bytecomp byte-compile cl-extra help-mode
easymenu cconv cl-loaddefs pcase cl-lib bbdb-autoloads mule-util tooltip
eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript case-table epa-hook jka-cmpr-hook help
simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs button
faces cus-face macroexp files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 8 616108 100562)
 (symbols 24 37145 0) (miscs 20 6978 1294) (strings 16 104997 17664)
 (string-bytes 1 2882737)
 (vectors 8 85773) (vector-slots 4 2368868 93788) (floats 8 1285 1070)
 (intervals 28 18695 3557)
 (buffers 520 64))

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

[Mark Oteiza]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Package: Emacs
> Version: 26.0.50
>
> In a buffer with more than one hunk, if I'm in the middle of hunk number
> N, diff-hunk-prev (usually bound to M-p) jumps to the header of hunk
> number N-1 rather than to the header of hunk N.
>
> This is contrary to the usual behavior of Emacs's navigation commands.

Implemented in 2c8a7e5 (bug#17544). Apparently it was intended.

https://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00222.html

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25105

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

[Stefan Monnier]

>> In a buffer with more than one hunk, if I'm in the middle of hunk number
>> N, diff-hunk-prev (usually bound to M-p) jumps to the header of hunk
>> number N-1 rather than to the header of hunk N.
>> 
>> This is contrary to the usual behavior of Emacs's navigation commands.

> Implemented in 2c8a7e5 (bug#17544). Apparently it was intended.

> https://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00222.html

> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25105

Maybe it's OK to change the behavior of `diff-hunk-prev` this way when
called from Elisp (I still find it odd, but it might work better for
the callers), but it's clearly not a good idea for M-p to behave
this way.

As pointed out elsewhere, it's particularly obnoxious from EOB (in which
case, you're not really "within N" but you're virtually on "the header
of the non-existent hunk N+1", so going to the header of N-1 is really
wrong).

I also dislike the fact that M-n doesn't let me get to EOB.


        Stefan

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

[Tino Calancha]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> In a buffer with more than one hunk, if I'm in the middle of hunk number
> N, diff-hunk-prev (usually bound to M-p) jumps to the header of hunk
> number N-1 rather than to the header of hunk N.
>
> This is contrary to the usual behavior of Emacs's navigation commands.
>As pointed out elsewhere, it's particularly obnoxious from EOB (in which
>case, you're not really "within N" but you're virtually on "the header
>of the non-existent hunk N+1", so going to the header of N-1 is really
>wrong).
>
>I also dislike the fact that M-n doesn't let me get to EOB.

Following patch reverts commit 2c8a7e5.  Then it fixes dots 1. and 2.
described in the commit message of 2c8a7e5, i.e., Bug#17544.
This patch preserves the original definitions for 'diff-hunk-prev'
and 'diff-hunk-next'.

After applying locally this patch, you might want to do:
git diff 2c8a7e5^ HEAD lisp/vc/diff-mode.el
to see more clearly how it solves Bug#17544.

Regards,
Tino
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From a2dfaf390f4069b4e948dd0df84450359e7a0d92 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Wed, 11 Jan 2017 13:20:04 +0900
Subject: [PATCH] Fix Bugs 25105 and 25400

Revert 2c8a7e5 and implement a new fix for Bug#17544.
This patch satisfies 1. and 2. in 2c8a7e5 commit message.
The original definitions for 'diff-hunk-prev' and 'diff-hunk-next'
are preserved.
* 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 | 241 ++++++++++++++++++---------------------------------
 1 file changed, 83 insertions(+), 158 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..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
@@ -551,124 +584,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 +614,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.
@@ -731,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.
@@ -771,7 +701,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
@@ -789,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."
@@ -1373,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)
@@ -1764,9 +1695,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 +1705,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.
@@ -1838,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.
@@ -1883,15 +1814,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 +1896,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 +1990,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

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

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

[Mark Oteiza]

On 11/01/17 at 01:38pm, Tino Calancha wrote:
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
> > In a buffer with more than one hunk, if I'm in the middle of hunk number
> > N, diff-hunk-prev (usually bound to M-p) jumps to the header of hunk
> > number N-1 rather than to the header of hunk N.
> >
> > This is contrary to the usual behavior of Emacs's navigation commands.
> >As pointed out elsewhere, it's particularly obnoxious from EOB (in which
> >case, you're not really "within N" but you're virtually on "the header
> >of the non-existent hunk N+1", so going to the header of N-1 is really
> >wrong).
> >
> >I also dislike the fact that M-n doesn't let me get to EOB.
> 
> Following patch reverts commit 2c8a7e5.  Then it fixes dots 1. and 2.
> described in the commit message of 2c8a7e5, i.e., Bug#17544.
> This patch preserves the original definitions for 'diff-hunk-prev'
> and 'diff-hunk-next'.
> 
> After applying locally this patch, you might want to do:
> git diff 2c8a7e5^ HEAD lisp/vc/diff-mode.el
> to see more clearly how it solves Bug#17544.

Works for me AFAICT.  Thanks for working on it.

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

[Dmitry Gutov]

Hi Tino,

Thanks for doing this. However, 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.

Thanks.

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

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

[Dmitry Gutov]

On 12.01.2017 06:53, Tino Calancha wrote:

> Definitely.  Thanks for the suggestion!

Could you re-send the patches at attachments? Or just push them to a 
branch. I wish the first one could be produced with a simple 'git 
revert', but that leads to a conflict.

I get "Can't find the text to patch" with the second one, though.

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

[Tino Calancha]

On Fri, 13 Jan 2017, Dmitry Gutov wrote:

> Could you re-send the patches at attachments? Or just push them to a branch. 
I have pushed those changes into following new branch:
scratch/calancha-revert-2c8a7e5

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

[Dmitry Gutov]

On 13.01.2017 06:55, Tino Calancha wrote:

> I have pushed those changes into following new branch:
> scratch/calancha-revert-2c8a7e5

I've tried it out (but not the changes discussed in this thread later), 
and the behavior is much better. Thanks!

Please apply it to master sooner rather than later.

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

[Tino Calancha]

On Sat, 14 Jan 2017, Dmitry Gutov wrote:

> On 13.01.2017 06:55, Tino Calancha wrote:
>
>> I have pushed those changes into following new branch:
>> scratch/calancha-revert-2c8a7e5
>
> I've tried it out (but not the changes discussed in this thread later), and 
> the behavior is much better. Thanks!
>
> Please apply it to master sooner rather than later.
Pushed the fix to master branch as commits:
1508b538fd8f8c2e00aadcea42ac36013fad02e3
e5e42cefd7f2eb47d2c8660a7a317e8b08d36a82

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

[Dmitry Gutov]

On 21.01.2017 06:02, Tino Calancha wrote:

> Pushed the fix to master branch as commits:
> 1508b538fd8f8c2e00aadcea42ac36013fad02e3
> e5e42cefd7f2eb47d2c8660a7a317e8b08d36a82

Thank you very much. I can use the master branch again. :)

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

[Tino Calancha]

On Fri, 13 Jan 2017, Dmitry Gutov wrote:

> I wish the first one could be produced with a simple 'git revert', but that 
> leads to a conflict.
Sorry for the confusin.  I made the revert manually.
In addition to commit 2c8a7e5,  i've reverted following
other related commits:

e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97
6b6abe0dba6a9a2e5f78aac3814421886e7a184f
73349822cbd6e50526eda9c75453584d73dfca83
a283d655db88cdcc8cb53d8e2578e1cdf751c84b
61c6a10e3110490dadac4577cc540053341ff25c
2c8a7e50d24daf19ea7d86f1cfeaa98a41c56085


In scratch/calancha-revert-2c8a7e5 branch,
diff-mode.el history is as follows:

##### _before_ my changes #####
;; initial state for diff-mode.el is as in commit:
1f5592572887fe15e5b660bc60e66a7ab7c624cd ; w/ copyright year 2016
;; Update copyright year and we are at 
;; 3e30cda89474209716c6e16a1a81d02877c95a2b
;; i.e., first commit in my new branch).

;; then, second commit (0beb7d2968ab76878eb3be26f2d749977fdcaa2f)
;; add my fix for Bug#17544.

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

[npostavs]

merge 25400 25105
tags 25400 patch
quit

I merged the bugs, and dropped #25400 from the cc list, because I'm
getting duplicate emails everytime.  While I'm on a meta topic, sorry
for being hasty with merging the original patch that changed diff
movement like that.  I didn't fully grasp it, nor tested like I should
have done.

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

>
> ;; then, second commit (0beb7d2968ab76878eb3be26f2d749977fdcaa2f)
> ;; add my fix for Bug#17544.

I think the commit summary could be a bit friendlier, otherwise it looks
good to me.

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

[Tino Calancha]

On Fri, 13 Jan 2017, npostavs@users.sourceforge.net wrote:

> merge 25400 25105
> tags 25400 patch
> quit
>
> I merged the bugs, and dropped #25400 from the cc list, because I'm
> getting duplicate emails everytime.
Thank you.  Sorry for the duplicated e-mails :-(
> While I'm on a meta topic, sorry
> for being hasty with merging the original patch that changed diff
> movement like that.  I didn't fully grasp it, nor tested like I should
> have done.
No problem.  Indeed you are helping a lot with the bugs.  Thank you very 
much.
>> ;; then, second commit (0beb7d2968ab76878eb3be26f2d749977fdcaa2f)
>> ;; add my fix for Bug#17544.
>
> I think the commit summary could be a bit friendlier, otherwise it looks
> good to me.
Thanks.  I agree: if i read my own commit after some months, probably i 
will not fully understand it.
I propose following more verbose one:

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; return non-nil when point
is inside a hunk header, a file header, or within a line
matching diff-file-junk-re.
(diff-beginning-of-hunk): Use it.
Check if the point is inside a diff header, in the middle of a hunk,
or before the first hunk.
(diff-apply-hunk): Call diff-beginning-of-hunk with non-nil arg
before apply the hunk.
(diff-hunk-kill, diff-file-kill):
Call diff-beginning-of-hunk with non-nil arg after kill the hunks.
(diff-post-command-hook): Call diff-beginning-of-hunk with non-nil argument.

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

[npostavs]

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

>>
>> I merged the bugs, and dropped #25400 from the cc list, because I'm
>> getting duplicate emails everytime.
> Thank you.  Sorry for the duplicated e-mails :-(

Hmm, I'm still getting duplicates.  Maybe I shouldn't have merged?  Oh well.

>>
>> I think the commit summary could be a bit friendlier, otherwise it looks
>> good to me.
> Thanks.  I agree: if i read my own commit after some months, probably
> i will not fully understand it.
> I propose following more verbose one:
>
> Fix Bug#17544

I was thinking more about this summary line.  And maybe add some higher
level explanation before the ChangeLog entry about the problems being
solved?

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

[Tino Calancha]

On Fri, 13 Jan 2017, npostavs@users.sourceforge.net wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>>>
>>> I merged the bugs, and dropped #25400 from the cc list, because I'm
>>> getting duplicate emails everytime.
>> Thank you.  Sorry for the duplicated e-mails :-(
>
> Hmm, I'm still getting duplicates.  Maybe I shouldn't have merged?  Oh well.
Could be because i answered the e-mail to you and CC to Bug#25105? If you
are registered to that bug you might get 2 e-mails.
>>> I think the commit summary could be a bit friendlier, otherwise it looks
>>> good to me.
>> Thanks.  I agree: if i read my own commit after some months, probably
>> i will not fully understand it.
>> I propose following more verbose one:
>>
>> Fix Bug#17544
>
> I was thinking more about this summary line.  And maybe add some higher
> level explanation before the ChangeLog entry about the problems being
> solved?
How about copy verbatim the comentary from Dima in 2c8a7e5? He explained
very clearly the problem, and i am solving the same thing.
Mention to the regressions should must appear in the previous commit, 
where i do the actual revert.

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

[npostavs]

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

> On Fri, 13 Jan 2017, npostavs@users.sourceforge.net wrote:
>
>> Hmm, I'm still getting duplicates.  Maybe I shouldn't have merged?  Oh well.
> Could be because i answered the e-mail to you and CC to Bug#25105? If you
> are registered to that bug you might get 2 e-mails.
I think that's the normal case, and usually I don't get duplicated
mails.

>> I was thinking more about this summary line.  And maybe add some higher
>> level explanation before the ChangeLog entry about the problems being
>> solved?
> How about copy verbatim the comentary from Dima in 2c8a7e5? He explained
> very clearly the problem, and i am solving the same thing.
> Mention to the regressions should must appear in the previous commit,
> where i do the actual revert.

Yes, that looks good, though if I understand correctly, the important
difference is that the navigation commands are not changed in your
patch, only the manipulation ones, right?

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

[Tino Calancha]

On Fri, 13 Jan 2017, npostavs@users.sourceforge.net wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>> How about copy verbatim the comentary from Dima in 2c8a7e5? He explained
>> very clearly the problem, and i am solving the same thing.
>> Mention to the regressions should must appear in the previous commit,
>> where i do the actual revert.
>
> Yes, that looks good, though if I understand correctly, the important
> difference is that the navigation commands are not changed in your
> patch, only the manipulation ones, right?
That's right.
Essentially, my patch fix 1. and 2. in the commit message of 2c8a7e5.
I am going to prepare one draft for the commit message based on Dima's 
message by Monday or Tuesday: i will drop mentions to the navigation 
commands.  Then i will show it here so that people in this thread can
give me comments/corrections to make the message clear for everyone.

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

[Tino Calancha]

On Sat, 14 Jan 2017, Tino Calancha wrote:

> Essentially, my patch fix 1. and 2. in the commit message of 2c8a7e5.
> I am going to prepare one draft for the commit message based on Dima's 
> message by Monday or Tuesday: i will drop mentions to the navigation 
> commands.  Then i will show it here so that people in this thread can
> give me comments/corrections to make the message clear for everyone.
OK, below is my draft for the second commit.
Please don't hesitate to send me comments to make it more clear or/and
fix some broken grammar.
;; For the first commit, the revert of 2c8a7e5, i propose to keep the same 
;; as before.  Let me know in case you want to change it as well.  Maybe 
;; you want this commit list all the commits that it reverts?
;; Currently it just mentions 2c8a7e5, but it doesn't say a word about 
;; the 'children commits':
;; e5ef59b8, 6b6abe0d, 73349822, a283d655 and 61c6a10e.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Improve diff-mode manipulation commands

Fix Bug#17544.
The use of `diff-apply-hunk' and `diff-hunk-kill' had several annoying
corner cases that this patch fixes.  These corner cases were largely due
to inconsistent treatment of diff headers.
Consider following diff:

  diff --git a/foo b/bar
  index 44556ddd4a..3fc4713f0f 100644
  --- a/foo
  +++ b/bar
  @@ -52,7 +52,7 @@
  hunk1
  @@ -74,7 +74,7 @@
  hunk2
  diff --git a/baz b/qux
  index a8c3fcca2f..6b316c4073 100644
  --- a/baz
  +++ b/qux
  @@ -608,6 +608,6 @@
  hunk3
  @@ -654,7 +654,7 @@
  hunk4

The lines starting and ending with '@@' are the hunk headers.
The lines starting with '---' and '+++' are the file headers.
In addition, Git diffs add extra lines as those starting with
'diff' and 'index' above.  Let's call these lines Git header.
From now on, these three headers are referred together as the diff header,
that is,
diff header = 'Git + file + hunk' headers.

Commands `diff-apply-hunk' and `diff-hunk-kill' behaves differently
depending on the point position within a diff header.

1. If point is at hunk1 header, '@@ -52,7 +52,7 @@' above, then
`diff-apply-hunk' moves point to hunk2 header,
i.e., '@@ -74,7 +74,7 @@' line.
If point is at '--- a/foo' line, then `diff-apply-hunk' moves point
to hunk1 header.

2. If point is at hunk3 header or its file header, then
`diff-hunk-kill' deletes hunk3.
If point is at the beginning of its Git header,
i.e., 'diff --git a/baz b/qux' line, then `diff-hunk-kill' deletes hunk2.

After this patch the behaviour of these commands is independent of the
point position in the diff header.  Then, it's possible to apply hunks
in order.  It's also possible to press M-k repeatedly to kill hunks in
the order they appear in the buffer.

* 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.
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

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

[Dmitry Gutov]

On 16.01.2017 09:26, Tino Calancha wrote:

> OK, below is my draft for the second commit.
> Please don't hesitate to send me comments to make it more clear or/and
> fix some broken grammar.

Here's a fresh idea. If we're not going to worry about (not) showing 
Dima's commit message in the generated change log, maybe the new message 
should talk about tweaking the code, and not retell the original message.

Or the users will read it twice, basically. But only those users that 
read change logs, of course.

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

[Tino Calancha]

On Wed, 18 Jan 2017, Dmitry Gutov wrote:

> On 16.01.2017 09:26, Tino Calancha wrote:
>
>> OK, below is my draft for the second commit.
>> Please don't hesitate to send me comments to make it more clear or/and
>> fix some broken grammar.
>
> Here's a fresh idea. If we're not going to worry about (not) showing Dima's 
> commit message in the generated change log, maybe the new message should talk 
> about tweaking the code, and not retell the original message.
>
> Or the users will read it twice, basically. But only those users that read 
> change logs, of course.
OK, it's reasonable.
See my new draft for commit 0beb7d2968ab76878eb3be26f2d749977fdcaa2f
in branch scratch/calancha-revert-2c8a7e5:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Make diff-apply-hunk independent of point within a diff header

Make diff-apply-hunk and diff-hunk-kill independent of the point
position in a diff header (Bug#17544).
This change allows to apply hunks in order.  It also makes possible to
press M-k repeatedly to kill hunks in the order they appear in the buffer.

* lisp/vc/diff-mode.el (diff-file-junk-re):
Move definition before it's used.
(diff--at-diff-header-p): New predicate; return non-nil when point
is inside a hunk header, a file header, or within a line
matching diff-file-junk-re.
(diff-beginning-of-hunk): Use it.
Check if the point is inside a diff header, in the middle of a hunk,
or before the first hunk.
(diff-apply-hunk): Call diff-beginning-of-hunk with non-nil arg
before apply the hunk.
(diff-hunk-kill, diff-file-kill):
Call diff-beginning-of-hunk with non-nil arg after kill the hunks.
(diff-post-command-hook): Call diff-beginning-of-hunk with non-nil argument.
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

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

[Dmitry Gutov]

On 13.01.2017 07:25, Tino Calancha wrote:

> Sorry for the confusin.  I made the revert manually.
> In addition to commit 2c8a7e5,  i've reverted following
> other related commits:
>
> e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97
> 6b6abe0dba6a9a2e5f78aac3814421886e7a184f
> 73349822cbd6e50526eda9c75453584d73dfca83
> a283d655db88cdcc8cb53d8e2578e1cdf751c84b
> 61c6a10e3110490dadac4577cc540053341ff25c
> 2c8a7e50d24daf19ea7d86f1cfeaa98a41c56085

On that subject, maybe reverting each of them separately would be better.

Although I've tried how that affects 'git blame' in practice, and it 
makes no difference.

Maybe our Changelog-generating script could take reverts into account 
(and skip commits that have been explicitly reverted later), if it 
doesn't do that already.