bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha <tino.calancha@gmail.com>]

On Sat, 7 Jan 2017, Dima Kogan wrote:

Dear Dima,
i appreciate very much your efforts to improve diff-mode.
In particular, i like 1. and 2. in your commit message.
That are useful goals.  I just disagree with the implementation.
In summary, i believe a better approach is to get 1., 2.
without affecting the behaviour of `n', `p'.

>    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.
Your patch is invasive: it should be possible to get 1) an 2) above without
redefining a long standing behaviour for `diff-hunk-next' and `diff-hunk-prev'.
In addition, that change in `n' and `p' would deserve a more prominent explanation
in the commit message, f.i., a new dot 3., so people would be aware of
this subtle change.

>Let's look at the commands required to apply hunks 1 and 2 in a buffer
>versus just hunk 2. Assuming we're at bob, and assuming we're using the
>new code.
>
>  Applying hunks 1 and 2:  C-c C-a   C-c C-a; i.e. apply 1, apply 2
>  Applying hunk 2 only:    M-n       C-c C-a; i.e. skip 1,  apply 2
>
>If M-n works the way it did before, then you need to invoke M-n twice to
>apply hunk 2 only. I claim this is weird, since it should require only
>one command to "skip 1". This is what is meant by a "consistent"
>behavior.
Well, not everyone follow that logic.  I like to read carefully the hunks
of a patch _before_ decide to apply then.  I can do that with easy using `n' and `p'
keys.  If the commit message is long, as in your commit 2c8a7e5, then an user need
`n' to jump to the first hunk and read it; but after your patch the first hunk
is skipped.  Then the user need to be a believer and apply it without seeing
it using `C-c C-a'; non believer users, might prefer to rewind and read the hunk
as follows:
M-! git show 2c8a7e5 RET
C-o C-x C-q
M-x diff-mode RET
n
;; we are at 2nd hunk, i.e., at line:
@@ -570,7 +570,102 @@ diff--auto-refine-data
;; Assuming no split window, so we can see clearly the first hunk;
;; after looking at it carefully, we decided is good, so let's apply it:
p ; jump to 1st hunk: this is _extra_ compared with Emacs-25!
C-c C-a

So, it might be argued that this patch force users to gratuitously push
_always_ an extra `p' to read/apply the first hunk of _every_ patch.
That makes hunk navigation much less pleasant.

>Furthermore, I'd like to get some feedback from more than just the few
>people active on this thread. If there's a lopsided preference to the
>old approach, we can simply revert and move on.
As a rule of thumb, if i get N people here uncomfortable with one of my
patches on master branch, i believe that can easily be translated
to (* 100 N) people once the patch is in a stable release (at least).

>I'm fine with a switch that lets you pick what you
>want. As stated above, I don't think a hybrid approach makes sense,
>since it takes one weird thing and converts it to another thing that's
>weird in a different way.
FWIW, I am working in an alternative patch: it would satisfy your 1. and 2.
while respecting `n' and `p'.  Stay tune!

Best regards,
Tino