GNU bug report logs - #17544
24.3; [PATCH] Improved diff-mode navigation/manipulation

Previous Next

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

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

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

From: Dima Kogan <lists <at> dima.secretsauce.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3; [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 21 May 2014 05:21:28 -0700

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

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.

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From 27d926293402c74eb3d83124e0287dd49cb4543a Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima <at> secretsauce.net>
Date: Thu, 15 May 2014 00:24:00 -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 | 77 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 11 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 923de9a..d334307 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -593,6 +593,58 @@ next hunk if TRY-HARDER is non-nil; otherwise signal an error."
 (easy-mmode-define-navigation
  diff-file diff-file-header-re "file" diff-end-of-file)
 
+(defun diff--wrap-hunk-navigation (isinteractive name orig count)
+  "I override the default diff-hunk-next/prev implementation by
+skipping any lines that are associated with this hunk 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 isinteractive)
+      (funcall orig count)
+
+    (let ((start (point)))
+      (let ((hunk-bounds (diff-bounds-of-hunk)))
+        (goto-char (car hunk-bounds)))
+
+      (funcall orig count)
+
+      (when (not (looking-at diff-hunk-header-re))
+        (goto-char start)
+        (user-error (format "No %s hunk" name))))))
+
+(setq diff--hunk-prev-internal (symbol-function 'diff-hunk-prev))
+(defun diff-hunk-prev (&optional count)
+  "Go to the previous COUNT'th hunk"
+  (interactive "p")
+  (diff--wrap-hunk-navigation
+   (called-interactively-p 'interactive)
+   "prev"
+   diff--hunk-prev-internal
+   count))
+
+(setq diff--hunk-next-internal (symbol-function 'diff-hunk-next))
+(defun diff-hunk-next (&optional count)
+  "Go to the next COUNT'th hunk"
+  (interactive "p")
+  (diff--wrap-hunk-navigation
+   (called-interactively-p 'interactive)
+   "next"
+   diff--hunk-next-internal
+   count))
+
+
+
+
 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
 The return value is a list (BEG END), which are the hunk's start
@@ -602,12 +654,13 @@ point is in a file header, return the bounds of the next 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.
@@ -1683,8 +1736,9 @@ SRC and DST are the two variants of text as returned by `diff-hunk-text'.
 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),
@@ -1693,7 +1747,7 @@ NOPROMPT, if non-nil, means not to prompt the user."
 	   ;; 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.
@@ -1802,7 +1856,7 @@ With a prefix argument, REVERSE the hunk."
       (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
       (diff-hunk-status-msg line-offset (diff-xor switched reverse) nil)
       (when diff-advance-after-apply-hunk
-	(diff-hunk-next))))))
+	(call-interactively 'diff-hunk-next))))))
 
 
 (defun diff-test-hunk (&optional reverse)
@@ -1873,14 +1927,15 @@ For use in `add-log-current-defun-function'."
 (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"))
@@ -1967,8 +2022,8 @@ For use in `add-log-current-defun-function'."
   (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-change)))
@@ -1976,7 +2031,7 @@ For use in `add-log-current-defun-function'."
            (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.0.0.rc0

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dima Kogan <lists <at> dima.secretsauce.net>
Cc: 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 24 Feb 2016 13:33:03 +1100

Dima Kogan <lists <at> dima.secretsauce.net> writes:

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

I think this makes sense, and the patch looks good to me.  Does it still
apply to the Emacs trunk?  Anybody got any objections to installing it?

It should also have an entry in NEWS, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Dima Kogan <dima <at> secretsauce.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 03 Sep 2016 02:17:19 -0700

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> I think this makes sense, and the patch looks good to me. Does it
> still apply to the Emacs trunk? Anybody got any objections to
> installing it?
>
> It should also have an entry in NEWS, I think.

This is a gentle ping. Can we install this?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dima Kogan <dima <at> secretsauce.net>
Cc: larsi <at> gnus.org, 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 03 Sep 2016 13:14:24 +0300

> From: Dima Kogan <dima <at> secretsauce.net>
> Date: Sat, 03 Sep 2016 02:17:19 -0700
> Cc: 17544 <at> debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> 
> > I think this makes sense, and the patch looks good to me. Does it
> > still apply to the Emacs trunk? Anybody got any objections to
> > installing it?
> >
> > It should also have an entry in NEWS, I think.
> 
> This is a gentle ping. Can we install this?

I'd like to hear Stefan's comments.

Regardless, the doc string of diff--wrap-hunk-navigation is not
according to our coding style, so please fix it.  Also, I don't quite
understand why you need changes like this one:

  -	(diff-hunk-next))))))
  +	(call-interactively 'diff-hunk-next))))))

and the whole issue of testing called-interactively-p that goes with
it.  Can you explain?

Thanks.

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

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Dima Kogan <lists <at> dima.secretsauce.net>
Cc: 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 03 Sep 2016 13:05:27 +0200

On Mai 21 2014, Dima Kogan <lists <at> dima.secretsauce.net> wrote:

> +(setq diff--hunk-prev-internal (symbol-function 'diff-hunk-prev))
> +(defun diff-hunk-prev (&optional count)

This will break if diff-mode is reloaded.

Andreas.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

From: Dima Kogan <dima <at> secretsauce.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Andreas Schwab <schwab <at> linux-m68k.org>, 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 03 Sep 2016 14:24:20 -0700

I'd like to get high-level consensus before making any changes. So ...


Eli Zaretskii <eliz <at> gnu.org> writes:

> Regardless, the doc string of diff--wrap-hunk-navigation is not
> according to our coding style, so please fix it.

OK.


> Also, I don't quite understand why you need changes like this one:
>
>   -	(diff-hunk-next))))))
>   +	(call-interactively 'diff-hunk-next))))))

It's been quite a while since I wrote this, and looking at it just now I
can't tell why this is necessary. So let's say one can take out this
hunk


> and the whole issue of testing called-interactively-p that goes with
> it.  Can you explain?

I'm guessing the interactivity checking in diff-hunk-next and
diff-hunk-prev was intended to keep scripts working as before. Again, it
has been too long to remember specifically.


Andreas Schwab <schwab <at> linux-m68k.org> writes:

> On Mai 21 2014, Dima Kogan <lists <at> dima.secretsauce.net> wrote:
>
>> +(setq diff--hunk-prev-internal (symbol-function 'diff-hunk-prev))
>> +(defun diff-hunk-prev (&optional count)
>
> This will break if diff-mode is reloaded.

Will it? diff-hunk-next and -prev are defined just above in the

    (easy-mmode-define-navigation
     diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view

block. If one reloads diff-mode, wouldn't this
easy-mmode-define-navigation block define the old diff-hunk-prev, which
then gets redefined by the code in the patch? It would be nicer to
define these functions properly the first time instead of (effectively)
advising them. But (easy-mmode-define-navigation) allows arbitrary code
to be called after the template, but I need new stuff BEFORE it. Better
ideas welcome.

dima

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 03 Sep 2016 23:27:50 -0400

Dima Kogan <dima <at> secretsauce.net> writes:

>
>> and the whole issue of testing called-interactively-p that goes with
>> it.  Can you explain?
>
> I'm guessing the interactivity checking in diff-hunk-next and
> diff-hunk-prev was intended to keep scripts working as before. Again, it
> has been too long to remember specifically.

IMO, it would make more sense to just define your new commands directly,
something like:

    (defun diff-hunk-next-command (&optional count)
      "<A useful description goes here>."
      (interactive "p")
      (let ((start (point)))
        (let ((hunk-bounds (diff-bounds-of-hunk)))
          (goto-char (car hunk-bounds)))
        (diff-hunk-next count)
        (when (not (looking-at diff-hunk-header-re))
          (goto-char start)
          (user-error "No next hunk"))))   

And then just give the *binding* of `diff-hunk-next' to
`diff-hunk-next-command'.  No need to make a higher order wrapper
function just for defining 2 functions.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 07 Sep 2016 00:14:56 -0700

npostavs <at> users.sourceforge.net writes:

> Dima Kogan <dima <at> secretsauce.net> writes:
>
>>
>>> and the whole issue of testing called-interactively-p that goes with
>>> it.  Can you explain?
>>
>> I'm guessing the interactivity checking in diff-hunk-next and
>> diff-hunk-prev was intended to keep scripts working as before. Again, it
>> has been too long to remember specifically.
>
> IMO, it would make more sense to just define your new commands directly,
> something like:
>
>     (defun diff-hunk-next-command (&optional count)
>       "<A useful description goes here>."
>       (interactive "p")
>       (let ((start (point)))
>         (let ((hunk-bounds (diff-bounds-of-hunk)))
>           (goto-char (car hunk-bounds)))
>         (diff-hunk-next count)
>         (when (not (looking-at diff-hunk-header-re))
>           (goto-char start)
>           (user-error "No next hunk"))))   
>
> And then just give the *binding* of `diff-hunk-next' to
> `diff-hunk-next-command'.  No need to make a higher order wrapper
> function just for defining 2 functions.

I've no problems with this. If I address this and the other comments, we
can talk about merging this?

Thanks

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Mon, 12 Sep 2016 20:56:22 -0700

npostavs <at> users.sourceforge.net writes:

> Dima Kogan <dima <at> secretsauce.net> writes:
c>
>>
>>> and the whole issue of testing called-interactively-p that goes with
>>> it.  Can you explain?
>>
>> I'm guessing the interactivity checking in diff-hunk-next and
>> diff-hunk-prev was intended to keep scripts working as before. Again, it
>> has been too long to remember specifically.
>
> IMO, it would make more sense to just define your new commands directly,
> something like:
>
>     (defun diff-hunk-next-command (&optional count)
>       "<A useful description goes here>."
>       (interactive "p")
>       (let ((start (point)))
>         (let ((hunk-bounds (diff-bounds-of-hunk)))
>           (goto-char (car hunk-bounds)))
>         (diff-hunk-next count)
>         (when (not (looking-at diff-hunk-header-re))
>           (goto-char start)
>           (user-error "No next hunk"))))   
>
> And then just give the *binding* of `diff-hunk-next' to
> `diff-hunk-next-command'.  No need to make a higher order wrapper
> function just for defining 2 functions.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 14 Sep 2016 15:31:10 -0700

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

Here's a revised patch. I've been running with this for a few days, and
it feels delightful. Changes from the last time:

- Removed the unnecessary (call-interactively ...) pointed out by Eli

- Instead of using (easy-mmode-define-navigation) to create
  diff-{hunk,file}-{next,prev} and then overriding that definition, I
  now have (easy-mmode-define-navigation) create
  diff--internal-{hunk,file}-{next,prev}, and then my own
  (diff-{hunk,file}-{next,prev}) call those functions

- I now handle file navigation in addition to hunk navigation

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From 8070ca643f5467c3dcfeb8d45b76a897b347d518 Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan <at> 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 | 110 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 98 insertions(+), 12 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..6716f39 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -551,7 +551,7 @@ next hunk if TRY-HARDER is non-nil; otherwise signal an error."
 
 ;; 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 @@ next hunk if TRY-HARDER is non-nil; otherwise signal an error."
                                 (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 (isinteractive
+                              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 isinteractive)
+      (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)
+  "Go to the previous COUNT'th hunk"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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)
+  "Go to the next COUNT'th hunk"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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)
+  "Go to the previous COUNT'th file"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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)
+  "Go to the next COUNT'th file"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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 @@ point is in a file header, return the bounds of the next 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 @@ SRC and DST are the two variants of text as returned by `diff-hunk-text'.
 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 @@ NOPROMPT, if non-nil, means not to prompt the user."
 	   ;; 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.
@@ -1784,6 +1867,8 @@ With a prefix argument, REVERSE the hunk."
       (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
       (diff-hunk-status-msg line-offset (diff-xor switched reverse) nil)
       (when diff-advance-after-apply-hunk
+        ;; old option:
+        ;; 	(call-interactively 'diff-hunk-next))))))
 	(diff-hunk-next))))))
 
 
@@ -1865,14 +1950,15 @@ For use in `add-log-current-defun-function'."
 (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 +2045,8 @@ For use in `add-log-current-defun-function'."
   (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 +2054,7 @@ For use in `add-log-current-defun-function'."
            (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.1

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Fri, 23 Sep 2016 00:22:38 -0700

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

Here's yet another revised patch. The (call-interactively) at the end of
(diff-apply-hunk) was important, it turns out. This would force it to
use the new logic to move to the next hunk, instead of the legacy logic.
I purposely left the behavior of (diff-next-hunk) unchanged from before
when running non-interactively, and here I explicitly want the new
behavior.

This patch puts the (call-interactively) back in, and explains the
rationale.

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From 21964781a87b615cdab65ab2643deaac8dbaa406 Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan <at> 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 13 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..b37ceec 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 (isinteractive
+                              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 isinteractive)
+      (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)
+  "Go to the previous COUNT'th hunk"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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)
+  "Go to the next COUNT'th hunk"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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)
+  "Go to the previous COUNT'th file"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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)
+  "Go to the next COUNT'th file"
+  (interactive "p")
+  (diff--wrap-navigation
+   (called-interactively-p 'interactive)
+   "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,13 @@ 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 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))))))
+        (call-interactively 'diff-hunk-next))))))
 
 
 (defun diff-test-hunk (&optional reverse)
@@ -1865,14 +1953,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 +2048,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 +2057,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

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 22 Oct 2016 11:47:29 -0400

Dima Kogan <dima <at> secretsauce.net> writes:

> Here's yet another revised patch. The (call-interactively) at the end of
> (diff-apply-hunk) was important, it turns out. This would force it to
> use the new logic to move to the next hunk, instead of the legacy logic.
> I purposely left the behavior of (diff-next-hunk) unchanged from before
> when running non-interactively, and here I explicitly want the new
> behavior.

If both behaviours are needed, it would be much better if lisp code
could choose between them without having to use call-interactively,
that's quite an awkward interface.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 22 Oct 2016 18:44:15 -0700

npostavs <at> users.sourceforge.net writes:

> Dima Kogan <dima <at> secretsauce.net> writes:
>
>> Here's yet another revised patch. The (call-interactively) at the end of
>> (diff-apply-hunk) was important, it turns out. This would force it to
>> use the new logic to move to the next hunk, instead of the legacy logic.
>> I purposely left the behavior of (diff-next-hunk) unchanged from before
>> when running non-interactively, and here I explicitly want the new
>> behavior.
>
> If both behaviours are needed, it would be much better if lisp code
> could choose between them without having to use call-interactively,
> that's quite an awkward interface.

Hi. I'm open to suggestions. The goal was to retain the previous logic
for any existing code, but to provide improved user-facing behavior.
Given this, it doesn't seem to me to be too awkward to pass-on the
"interactive-p" state to child functions. Am I wrong to want to preserve
existing behavior for elisp code? If so, then the entire old path can
simply go away unconditionally.

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 22 Oct 2016 22:49:21 -0400

Dima Kogan <dima <at> secretsauce.net> writes:

> npostavs <at> users.sourceforge.net writes:
>
>> Dima Kogan <dima <at> secretsauce.net> writes:
>>
>>> Here's yet another revised patch. The (call-interactively) at the end of
>>> (diff-apply-hunk) was important, it turns out. This would force it to
>>> use the new logic to move to the next hunk, instead of the legacy logic.
>>> I purposely left the behavior of (diff-next-hunk) unchanged from before
>>> when running non-interactively, and here I explicitly want the new
>>> behavior.
>>
>> If both behaviours are needed, it would be much better if lisp code
>> could choose between them without having to use call-interactively,
>> that's quite an awkward interface.
>
> Hi. I'm open to suggestions. The goal was to retain the previous logic
> for any existing code, but to provide improved user-facing behavior.
> Given this, it doesn't seem to me to be too awkward to pass-on the
> "interactive-p" state to child functions.
>

But you're synthesizing interactiveness in diff-apply-hunk, so it looks
like interactiveness is not what you actually care about.  You could do
something like this instead:

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

Then instead of (call-interactively 'diff-hunk-next) you can just do
(diff-hunk-next nil t)

> Am I wrong to want to preserve
> existing behavior for elisp code? If so, then the entire old path can
> simply go away unconditionally.

Maybe.  What about the other calls to diff-hunk-next?  Was there a
reason you kept them the same?

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sun, 06 Nov 2016 18:26:54 -0800

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

npostavs <at> users.sourceforge.net writes:

> Dima Kogan <dima <at> secretsauce.net> writes:
>
>> Hi. I'm open to suggestions. The goal was to retain the previous logic
>> for any existing code, but to provide improved user-facing behavior.

Ack! So this is what happens when you submit a patch, and then talk
about it more than 2 years later. Turns out preserving compatibility
with existing code wasn't my goal at all. Rather, the goal was to avoid
an infinite loop that results if (diff-hunk-next) unconditionally uses
the new logic:

    diff-hunk-next         calls
    diff--wrap-navigation  calls
    diff-bounds-of-hunk    calls
    diff-beginning-of-hunk calls
    diff-hunk-next


>  it looks like interactiveness is not what you actually care about.
> You could do something like this instead:
>
> (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))
>
> Then instead of (call-interactively 'diff-hunk-next) you can just do
> (diff-hunk-next nil t)

Yes. Agreed. Revised patch attached. I'll try to respond to any comments
quickly so that we all can remember what this is about.

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From 4cd4f839dc840493ca14ff13a4f27cedca12a562 Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan <at> 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 13 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..16d3f46 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,13 @@ 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 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))))))
 
 
 (defun diff-test-hunk (&optional reverse)
@@ -1865,14 +1953,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 +2048,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 +2057,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.10.1

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Mon, 14 Nov 2016 22:31:17 -0500

Dima Kogan <dima <at> secretsauce.net> writes:

> npostavs <at> users.sourceforge.net writes:
>
>> Dima Kogan <dima <at> secretsauce.net> writes:
>>
>>> Hi. I'm open to suggestions. The goal was to retain the previous logic
>>> for any existing code, but to provide improved user-facing behavior.
>
> Ack! So this is what happens when you submit a patch, and then talk
> about it more than 2 years later. Turns out preserving compatibility
> with existing code wasn't my goal at all. Rather, the goal was to avoid
> an infinite loop that results if (diff-hunk-next) unconditionally uses
> the new logic:
>
>     diff-hunk-next         calls
>     diff--wrap-navigation  calls
>     diff-bounds-of-hunk    calls
>     diff-beginning-of-hunk calls
>     diff-hunk-next
>
> 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.

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

>  	 (hunk (delete-and-extract-region
> -		(point) (save-excursion (diff-end-of-hunk) (point))))
> +                (point) (cadr hunk-bounds)))

Indentation looks off here.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 16 Nov 2016 20:15:34 -0800

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

New patch attached.

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From b02085abd998d2c1e1519973964295fb5b798e0c Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan <at> 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

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

npostavs <at> users.sourceforge.net writes:

> Dima Kogan <dima <at> secretsauce.net> writes:
>
>> npostavs <at> 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.

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 16 Nov 2016 23:33:23 -0500

Dima Kogan <dima <at> secretsauce.net> writes:

> New patch attached.
>
[...]
> +
> +      ;; 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))))))

Can you mention somewhere about avoiding an infinite loop that you were
talking about before? (that's what I meant when I said to update this
comment, but if it actually makes more sense to mention that somewhere
else, please do so)
Is it really a "legacy" behavior (considering that we *need* the "legacy"
behavior in order to function correctly)?

Also, I believe usual comment style is to use "we" not "I", and you
didn't end the last sentence with a period.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Thu, 17 Nov 2016 00:05:38 -0800

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

New patch attached.

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From 769915004c4eb523d9e39da7ac96ec2174a213f9 Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan <at> 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 | 130 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 117 insertions(+), 13 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..2a4c3d9 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,101 @@ 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))))))
+
+;; 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))
+
+
+
 
 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
@@ -581,12 +675,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 +1760,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 +1771,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 +1879,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)
+
+      ;; We 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 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 +1968,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 +2063,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 +2072,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

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

npostavs <at> users.sourceforge.net writes:

> Can you mention somewhere about avoiding an infinite loop that you were
> talking about before? (that's what I meant when I said to update this
> comment, but if it actually makes more sense to mention that somewhere
> else, please do so)
> Is it really a "legacy" behavior (considering that we *need* the "legacy"
> behavior in order to function correctly)?

Added comment, changed nomenclature.


> Also, I believe usual comment style is to use "we" not "I", and you
> didn't end the last sentence with a period.

Changed the I/we. I'm omitting the trailing . on purpose to make sure it
doesn't get confused with the URL.

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sat, 19 Nov 2016 21:37:20 -0500

Dima Kogan <dima <at> secretsauce.net> writes:

> +(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

The first line of a docstring should be a single sentence.

> +this hunk/file but precede the hunk-start marker. For instance, a

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

Double space end of sentence.  No need to mention "I" here, it should be
phrased imperatively, as in "Override the default...", and "Move the point...".

> +executing the default diff-hunk-next/prev implementation to move
> +to the NEXT marker"

End sentences with a period.

> +      ;; I trap the error

End sentences with a period, no need to mention "I".

> +;; 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"

> +(defun diff-hunk-next (&optional count skip-hunk-start)
> +  "Go to the next COUNT'th hunk"

> +(defun diff-file-prev (&optional count skip-hunk-start)
> +  "Go to the previous COUNT'th file"

> +(defun diff-file-next (&optional count skip-hunk-start)
> +  "Go to the next COUNT'th file"

> +	    ;; There's no next hunk, so just take the one we have

End sentences with a period.

> +	    (t (list beg end))))))
> +      ;; applying hunks too.  See http://debbugs.gnu.org/17544

>> Also, I believe usual comment style is to use "we" not "I", and you
>> didn't end the last sentence with a period.
>
> Changed the I/we. I'm omitting the trailing . on purpose to make sure it
> doesn't get confused with the URL.

I suggest ";; applying hunks too (see http://debbugs.gnu.org/17544).",
referencing the bug with "Bug #17544" instead of a full URL could also
work.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Sun, 20 Nov 2016 23:23:04 -0800

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

npostavs <at> users.sourceforge.net writes:

> The first line of a docstring should be a single sentence.
>
> Double space end of sentence.  No need to mention "I" here, it should be
> phrased imperatively, as in "Override the default...", and "Move the point...".
>
> End sentences with a period.
>
> I suggest ";; applying hunks too (see http://debbugs.gnu.org/17544).",
> referencing the bug with "Bug #17544" instead of a full URL could also
> work.

Attached.

[0001-Improved-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

From d954bf87907ed2e3f94347c2249b305c7e231832 Mon Sep 17 00:00:00 2001
From: Dima Kogan <Dmitriy.Kogan <at> 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 | 131 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 118 insertions(+), 13 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..c9a5f89 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,102 @@ 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)
+  "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))))))
+
+;; 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))
+
+
+
 
 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
@@ -581,12 +676,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 +1761,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 +1772,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 +1880,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)
+
+      ;; 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 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 +1969,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 +2064,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 +2073,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

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Tue, 22 Nov 2016 19:42:03 -0500

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

The commit message should also have double spaced sentences and should
end with a ChangeLog style entry.  Apart from that I think is ready to
push to master.

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

From: Dima Kogan <dima <at> secretsauce.net>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Wed, 23 Nov 2016 13:11:23 -0800

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

npostavs <at> users.sourceforge.net writes:

> The commit message should also have double spaced sentences and should
> end with a ChangeLog style entry.  Apart from that I think is ready to
> push to master.

[0001-Improve-diff-mode-navigation-manipulation.patch (text/x-diff, inline)]

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

This is Bug #17544.

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 (diff--wrap-navigation): New function to add better
navigation logic to diff-{hunk,file}-{next,prev}.

lisp/vc/diff-mode.el (diff-hunk-next, diff-hunk-prev, diff-file-next,
diff-file-prev): Better navigation logic if skip-hunk-start is true,
which happens when called interactively.

lisp/vc/diff-mode.el (diff-bounds-of-hunk, diff-find-source-location,
diff-apply-hunk, diff-current-defun, diff-refine-hunk): Small tweaks to
improve hunk navigation.
---
 lisp/vc/diff-mode.el | 131 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 118 insertions(+), 13 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 58498cb..c9a5f89 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,102 @@ 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)
+  "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))))))
+
+;; 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))
+
+
+
 
 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
@@ -581,12 +676,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 +1761,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 +1772,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 +1880,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)
+
+      ;; 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 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 +1969,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 +2064,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 +2073,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

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

From: npostavs <at> users.sourceforge.net
To: Dima Kogan <dima <at> secretsauce.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>,
 17544 <at> debbugs.gnu.org
Subject: Re: bug#17544: 24.3;
 [PATCH] Improved diff-mode navigation/manipulation
Date: Mon, 28 Nov 2016 23:10:05 -0500

close 17544 26.1
quit

Dima Kogan <dima <at> secretsauce.net> writes:

> npostavs <at> users.sourceforge.net writes:
>
>> The commit message should also have double spaced sentences and should
>> end with a ChangeLog style entry.  Apart from that I think is ready to
>> push to master.
>
>>From d3950fca6cb1f3aa097cec0f524e38b9a7f05303 Mon Sep 17 00:00:00 2001
> From: Dima Kogan <Dmitriy.Kogan <at> jpl.nasa.gov>
> Date: Wed, 14 Sep 2016 15:25:06 -0700
> Subject: [PATCH] Improve diff-mode navigation/manipulation

Pushed as 2c8a7e50d24daf19ea7d86f1cfeaa98a41c56085.

[...]
>
> lisp/vc/diff-mode.el (diff--wrap-navigation): New function to add better
> navigation logic to diff-{hunk,file}-{next,prev}.
>
> lisp/vc/diff-mode.el (diff-hunk-next, diff-hunk-prev, diff-file-next,
> diff-file-prev): Better navigation logic if skip-hunk-start is true,
> which happens when called interactively.
>
> lisp/vc/diff-mode.el (diff-bounds-of-hunk, diff-find-source-location,
> diff-apply-hunk, diff-current-defun, diff-refine-hunk): Small tweaks to
> improve hunk navigation.
> ---

I fixed the formatting of the ChangeLog entry.

Previous Next

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