bug#25105: 26.0.50; diff navigation is broken

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

I am guessing this is a consequence of bug#17544. From -Q:

1. Do C-x v d RET = in a repository with a bunch of worktree changes
2. Hit n. Point is now at the top of the SECOND hunk
3. Go to end of buffer.
4. Hit p. Point is now at the top of the PENULTIMATE hunk

Further, from -Q:

1. Find a file generated by git format-patch. I had on hand the v2 patches
from bug#24966.
2. Hit M-n. Emacs complains "Can’t find the beginning of the file"

These are all regressions from Emacs 25.1

In GNU Emacs 26.0.50.1 (x86_64-unknown-linux-gnu, X toolkit, Xaw scroll bars)
 of 2016-12-04 built on logos
Repository revision: 35a86f0b6fe5634e94212964657c538739743d72

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --without-gconf --with-modules
 --with-x-toolkit=lucid 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe
 -fstack-protector-strong -g -fvar-tracking-assignments -g
 -fvar-tracking-assignments' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS
LUCID X11 MODULES LIBSYSTEMD

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

bug#25105: 26.0.50; diff navigation is broken

[npostavs]

Mark Oteiza <mvoteiza@udel.edu> writes:

> I am guessing this is a consequence of bug#17544. From -Q:

Yes, probably.  There were some other problems pointed out at:
http://lists.gnu.org/archive/html/emacs-devel/2016-11/msg00738.html

Dima, any ideas?

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

npostavs@users.sourceforge.net writes:

> Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> I am guessing this is a consequence of bug#17544. From -Q:
>
> Yes, probably.  There were some other problems pointed out at:
> http://lists.gnu.org/archive/html/emacs-devel/2016-11/msg00738.html

Hi. These are intentional. Justification:

When you're at the beginning of the buffer, you're logically already at
the first hunk. A justification of THIS is that if you try to apply the
hunk at point from beginning-of-buffer (C-c C-a) then the first hunk is
applied, and the point moves to the second hunk. This is the behavior
both before and after my changes.

I think the changes make diff-mode more consistent. If you disagree,
let's move the discussion to emacs-devel.

dima

bug#25105: 26.0.50; diff navigation is broken

[Dmitry Gutov]

On 05.12.2016 17:38, Dima Kogan wrote:

> Hi. These are intentional. Justification:

Which are intentional? I've described some legitimate problems in the 
emacs-devel email.

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

On December 5, 2016 7:53:41 AM PST, Dmitry Gutov 
<dgutov@yandex.ru> wrote:
>On 05.12.2016 17:38, Dima Kogan wrote:
>
>> Hi. These are intentional. Justification:
>
>Which are intentional? I've described some legitimate problems in the 
>emacs-devel email.

Sorry I wasn't clear. The points in the bug report were intentional. The auto-refinement breakage you described on the list was not, and I'll look at fixing it later today or tomorrow. My mail farted, and the message to the list to that effect apparently didn't make it

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

On 05/12/16 at 08:33am, Dima Kogan wrote:
> On December 5, 2016 7:53:41 AM PST, Dmitry Gutov 
> <dgutov@yandex.ru> wrote:
> >On 05.12.2016 17:38, Dima Kogan wrote:
> >
> >> Hi. These are intentional. Justification:
> >
> >Which are intentional? I've described some legitimate problems in the 
> >emacs-devel email.
> 
> Sorry I wasn't clear. The points in the bug report were intentional.
> The auto-refinement breakage you described on the list was not, and
> I'll look at fixing it later today or tomorrow. My mail farted, and
> the message to the list to that effect apparently didn't make it

I find it very hard to believe that the behavior in the second recipe
is intentional.

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

Mark Oteiza <mvoteiza@udel.edu> writes:

> I find it very hard to believe that the behavior in the second recipe
> is intentional.

I missed that part of the bug report. Sorry about that. That is a bug
that I haven't seen at all earlier. Will look at fixing it today or
tomorrow. Thanks for reporting.

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

Dima Kogan <dima@secretsauce.net> writes:

> Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> I find it very hard to believe that the behavior in the second recipe
>> is intentional.
>
> I missed that part of the bug report. Sorry about that. That is a bug
> that I haven't seen at all earlier. Will look at fixing it today or
> tomorrow. Thanks for reporting.

I pushed the fix to this. What do yall want to do about the other logic?
Are any of yall planning to revisit this? If not, let's close this bug.

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

On 24/12/16 at 10:57pm, Dima Kogan wrote:
> Dima Kogan <dima@secretsauce.net> writes:
> 
> > Mark Oteiza <mvoteiza@udel.edu> writes:
> >
> >> I find it very hard to believe that the behavior in the second recipe
> >> is intentional.
> >
> > I missed that part of the bug report. Sorry about that. That is a bug
> > that I haven't seen at all earlier. Will look at fixing it today or
> > tomorrow. Thanks for reporting.
> 
> I pushed the fix to this. What do yall want to do about the other logic?
> Are any of yall planning to revisit this? If not, let's close this bug.

I will be revisiting this, just haven't gotten around to it.

bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha]

Mark Oteiza <mvoteiza@udel.edu> writes:

> I am guessing this is a consequence of bug#17544. From -Q:
>
> 1. Do C-x v d RET = in a repository with a bunch of worktree changes
> 2. Hit n. Point is now at the top of the SECOND hunk
> 3. Go to end of buffer.
> 4. Hit p. Point is now at the top of the PENULTIMATE hunk
FWIW, to me this behaviour is very annoying and it has being
around already a while without a fix (4 months).
I would propose to revert the commit causing this misbehaviour.
Then, once the patch is mature enough, it can be pushed again without
affecting users.

Dima Kogan <dima@secretsauce.net> writes:

> I pushed the fix to this. What do yall want to do about the other logic?
If for 'the other logic' you mean Mark's recipe above, then what i want
is:
above recipe behaves exactly as it does in Emacs-25; that behaviour is
very convenient to read diffs.

Tino

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

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

> Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> I am guessing this is a consequence of bug#17544. From -Q:
>>
>> 1. Do C-x v d RET = in a repository with a bunch of worktree changes
>> 2. Hit n. Point is now at the top of the SECOND hunk
>> 3. Go to end of buffer.
>> 4. Hit p. Point is now at the top of the PENULTIMATE hunk
> FWIW, to me this behaviour is very annoying and it has being
> around already a while without a fix (4 months).
> I would propose to revert the commit causing this misbehaviour.
> Then, once the patch is mature enough, it can be pushed again without
> affecting users.

Hi.

This isn't a misbehavior, it's the whole point of the patch. We can
argue about whether it's an improvement or not, but if this is a "bug",
then the solution is a full revert.

The behavior I want is to always have a consistent idea of which hunk we
are currently on. In the recipe above, between steps 1 and 2 the point
is at bob. Both before and after the patch, the codes agree that we are
on the hunk 1. When you press 'n', I thus argue you should end up at
hunk 2. Similary, when you press C-c C-a; it should apply the first
hunk, and move to the second. And so on. What would you like?

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

Dima Kogan <dima@secretsauce.net> writes:

> The behavior I want is to always have a consistent idea of which hunk we
> are currently on.

Some more behaviors that I think are desirable are described in the
commit message of the main patch:

  https://github.com/emacs-mirror/emacs/commit/2c8a7e50d24daf19e

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

On 05/01/17 at 05:27pm, Dima Kogan wrote:
> Dima Kogan <dima@secretsauce.net> writes:
> 
> > The behavior I want is to always have a consistent idea of which hunk we
> > are currently on.
> 
> Some more behaviors that I think are desirable are described in the
> commit message of the main patch:
> 
>   https://github.com/emacs-mirror/emacs/commit/2c8a7e50d24daf19e

The only mention of the changes to navigation is "Better navigation
logic".  Not documented in NEWS, and no tests for the corner cases.
I fail to see how fixing corner cases in diff-apply-hunk has anything to
do with diff-{file,hunk}-{next-prev}

At first glance, it looks like the following patch would restore the
previous behavior, however it completely breaks auto refinement.

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 9dfcd944bb..3442b01d12 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -625,7 +625,7 @@ diff--wrap-navigation
 ;; 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))
+  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
   (diff--wrap-navigation
    skip-hunk-start
    "prev hunk"
@@ -636,7 +636,7 @@ diff-hunk-prev
 
 (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))
+  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
   (diff--wrap-navigation
    skip-hunk-start
    "next hunk"
@@ -647,7 +647,7 @@ diff-hunk-next
 
 (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))
+  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
   (diff--wrap-navigation
    skip-hunk-start
    "prev file"
@@ -658,7 +658,7 @@ diff-file-prev
 
 (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))
+  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
   (diff--wrap-navigation
    skip-hunk-start
    "next file"

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

On 05/01/17 at 05:20pm, Dima Kogan wrote:
> Tino Calancha <tino.calancha@gmail.com> writes:
> 
> > Mark Oteiza <mvoteiza@udel.edu> writes:
> >
> >> I am guessing this is a consequence of bug#17544. From -Q:
> >>
> >> 1. Do C-x v d RET = in a repository with a bunch of worktree changes
> >> 2. Hit n. Point is now at the top of the SECOND hunk
> >> 3. Go to end of buffer.
> >> 4. Hit p. Point is now at the top of the PENULTIMATE hunk
> > FWIW, to me this behaviour is very annoying and it has being
> > around already a while without a fix (4 months).
> > I would propose to revert the commit causing this misbehaviour.
> > Then, once the patch is mature enough, it can be pushed again without
> > affecting users.
> 
> <snip>  if this is a "bug",
> then the solution is a full revert.

With the number of actual bugs (email/format-patch/pre-diff content, and
auto refinement) the initial patch caused, perhaps this is best.

bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha]

On Thu, 5 Jan 2017, Mark Oteiza wrote:

> At first glance, it looks like the following patch would restore the
> previous behavior, however it completely breaks auto refinement.
>
> diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
> index 9dfcd944bb..3442b01d12 100644
> --- a/lisp/vc/diff-mode.el
> +++ b/lisp/vc/diff-mode.el
> @@ -625,7 +625,7 @@ diff--wrap-navigation
> ;; 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))
> +  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
>   (diff--wrap-navigation
>    skip-hunk-start
>    "prev hunk"
> @@ -636,7 +636,7 @@ diff-hunk-prev
>
> (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))
> +  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
>   (diff--wrap-navigation
>    skip-hunk-start
>    "next hunk"
> @@ -647,7 +647,7 @@ diff-hunk-next
>
> (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))
> +  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
>   (diff--wrap-navigation
>    skip-hunk-start
>    "prev file"
> @@ -658,7 +658,7 @@ diff-file-prev
>
> (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))
> +  (interactive (list (prefix-numeric-value current-prefix-arg) nil))
>   (diff--wrap-navigation
>    skip-hunk-start
>    "next file"
Hi Mark,

i have checked out your patch and your right: it recovered the Emacs-25
behaviour in your snippet code.
Tino

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

Mark Oteiza <mvoteiza@udel.edu> writes:

> On 05/01/17 at 05:27pm, Dima Kogan wrote:
>> Dima Kogan <dima@secretsauce.net> writes:
>>
>> > The behavior I want is to always have a consistent idea of which hunk we
>> > are currently on.
>>
>> Some more behaviors that I think are desirable are described in the
>> commit message of the main patch:
>>
>>   https://github.com/emacs-mirror/emacs/commit/2c8a7e50d24daf19e
>
> The only mention of the changes to navigation is "Better navigation
> logic".  Not documented in NEWS, and no tests for the corner cases.

I just re-read the commit message, and I think it's clear that it
touches navigation. I'm not a seasoned contributor to this project, so I
don't know what the policy is regarding NEWS. In either case, that is
irrelevant: we'd still be having this conversation.

Tests would be good, as always. However the diff-mode prior to this
patch had no tests, and I've been using this code every day for years.
And again, this doesn't matter. The issues in question aren't unintended
bugs, and they would have passed any tests I would have written.



> I fail to see how fixing corner cases in diff-apply-hunk has anything
> to do with diff-{file,hunk}-{next-prev}

The issues being fixed are making anything that operates on hunks more
consistent, so diff-{file,hunk}-{next-prev} are relevant.


> At first glance, it looks like the following patch would restore the
> previous behavior, however it completely breaks auto refinement.
>
> <snip>

If you want to restore the previous behavior, wouldn't a revert be
better? Or are you trying to restore only a subset of the previous
behavior?


> With the number of actual bugs (email/format-patch/pre-diff content,
> and auto refinement) the initial patch caused, perhaps this is best.

The email/format-patch issue has nothing to do with me; it has been a
problem for years. The way to "fix" auto-refinement is to invoke
auto-refinement in a diff-mode-hook, as suggested earlier. The bug
reporter didn't like that, and I don't know what they want. I'm not sure
where the pre-diff content issue came from. Likely it came up because
the patch that was in the BTS for years wasn't what ended up being
merged, so I haven't sufficiently tested it. Lesson learned.

I consider the current behavior a significant improvement in usability,
but if there's a consensus that it's a step backward, then I'll go back
to carrying this patch in my local tree. Let me ask the few people I
know who would be using this code at all to get at least anecdotal
feedback.

bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha]

Hi Dima,

thanks for your prompt replay.

i am sorry, i didn't follow the discussion in Bug#17544, so i just
realized its effects once the patch was pushed; those effects
affect my everyday use of Emacs.
(Added as CC the people who joined Bug#17544).

On Thu, 5 Jan 2017, Dima Kogan wrote:
> This isn't a misbehavior, it's the whole point of the patch. We can
> argue about whether it's an improvement or not
My point is that Bug#17544 is not a bug, it's a feature.  Your fix
just breaks the feature.
> if this is a "bug", then the solution is a full revert.
It might be the right thing to do.  I was actually very happy with how
Emacs-25 deal with this issue.
Another posibility is the patch that Mark just sent to this thread.
> The behavior I want is to always have a consistent idea of which hunk we
> are currently on.
IMO, this is not a good idea.  Things depend of the perspective.  If you
are in one hunk or another it depends of what you want to do.  That is
part of the feature.

>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:
I disagree, ideed i found it very consistent (see below).
> --- 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.
It seems some users expect it :-)
>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.

it's totally consistent to not consider
the file header the same if you are not doing the same operation.  If i am at
--- ccc
then, i want `diff-hunk-next' bring me to the line:
@@ -608,6 +608,6 @@
and `diff-hunk-prev' bring me to the line:
@@ -74,7 +74,7 @@

To this happen, in the first case the point must be considered at hunk2,
but in the second case, the point must be considered in hunk3.
To me, this is pretty consistent with the intended (and useful) behaviour.

Regards,
Tino

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

Dima Kogan <dima@secretsauce.net> writes:

> Mark Oteiza <mvoteiza@udel.edu> writes:
>> I fail to see how fixing corner cases in diff-apply-hunk has anything
>> to do with diff-{file,hunk}-{next-prev}
>
> The issues being fixed are making anything that operates on hunks more
> consistent, so diff-{file,hunk}-{next-prev} are relevant.

and this entire thread is about the contention over changes specifically
done to diff-{file,hunk}-{next-prev}.

To quote myself: "Fixing C-c C-a to DTRT is great, thanks, but I don't
think the off-by-one navigation changes to "n" and "p" (diff-hunk-next,
diff-hunk-prev) make sense."
https://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00222.html

>> At first glance, it looks like the following patch would restore the
>> previous behavior, however it completely breaks auto refinement.
>>
>> <snip>
>
> If you want to restore the previous behavior, wouldn't a revert be
> better? Or are you trying to restore only a subset of the previous
> behavior?

I did not submit it as a solution to the problem at hand.  The fact that
the patch breaks auto-refinement means that I cannot define my own
commands to call (diff-{file-hunk}-{prev-next} ARG nil) and have it
work.

Put another way, your changes make it nigh impossible to get the
previous n,p,{,} back without breaking something.

>> With the number of actual bugs (email/format-patch/pre-diff content,
>> and auto refinement) the initial patch caused, perhaps this is best.
>
> The email/format-patch issue has nothing to do with me; it has been a
> problem for years.

Yes it did, as the second recipe in this bug and 6b6abe0d clearly show.

> The way to "fix" auto-refinement is to invoke
> auto-refinement in a diff-mode-hook, as suggested earlier. The bug
> reporter didn't like that <snip>

Probably because auto-refinement is default behavior that got broken.

> I'm not sure
> where the pre-diff content issue came from. Likely it came up because
> the patch that was in the BTS for years wasn't what ended up being
> merged, so I haven't sufficiently tested it. Lesson learned.
>
> I consider the current behavior a significant improvement in usability,
> but if there's a consensus that it's a step backward, then I'll go back
> to carrying this patch in my local tree. Let me ask the few people I
> know who would be using this code at all to get at least anecdotal
> feedback.

bug#25105: 26.0.50; diff navigation is broken

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Date: Thu, 05 Jan 2017 20:16:12 -0800
> Cc: 25105@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>,
> 	Tino Calancha <tino.calancha@gmail.com>, npostavs@users.sourceforge.net
> 
> I consider the current behavior a significant improvement in usability,
> but if there's a consensus that it's a step backward, then I'll go back
> to carrying this patch in my local tree.

Another alternative is to have a customizable option which will let
users decide what behavior they want.

Thanks.

bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha]

On Fri, 6 Jan 2017, Eli Zaretskii wrote:

>> From: Dima Kogan <dima@secretsauce.net>
>> Date: Thu, 05 Jan 2017 20:16:12 -0800
>> Cc: 25105@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>,
>> 	Tino Calancha <tino.calancha@gmail.com>, npostavs@users.sourceforge.net
>>
>> I consider the current behavior a significant improvement in usability,
>> but if there's a consensus that it's a step backward, then I'll go back
>> to carrying this patch in my local tree.
>
> Another alternative is to have a customizable option which will let
> users decide what behavior they want.
That would be OK.  I would suggest to set this option nil by default, 
i.e., disable the new feature by default for backward compatibility.

bug#25105: 26.0.50; diff navigation is broken

[Dmitry Gutov]

On 06.01.2017 11:03, Tino Calancha wrote:

>> Another alternative is to have a customizable option which will let
>> users decide what behavior they want.
> That would be OK.  I would suggest to set this option nil by default,
> i.e., disable the new feature by default for backward compatibility.

If separating behavior into two parts that are controlled by a switch 
would be feasible (I'm not sure), it might be okay.

However, the new behavior also fixes what was undoubtedly a problem:

When point is a bob in a diff-mode buffer, `C-c C-a' applies the first 
hunk, and then stops at its beginning (in Emacs 25 and earlier).

We would then give up on that fix, whereas I'd prefer to have a solution 
eventually, if not now. But if we do, I estimate we might have the "old 
fixed" behavior encroach on the "new different" behavior in certain 
respects, making the code even more complex.

I've honestly thought that Dima's patch's main purpose was to fix that 
bug. And everything else we now complain about are just implementation's 
side-effects.

bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha]

On Fri, 6 Jan 2017, Dmitry Gutov wrote:

> However, the new behavior also fixes what was undoubtedly a problem:
>
> When point is a bob in a diff-mode buffer, `C-c C-a' applies the first hunk, 
> and then stops at its beginning (in Emacs 25 and earlier).
Honestly, i wasn't aware of `C-c C-a' functionality so i didn't 
realized that aim of the patch.

> We would then give up on that fix, whereas I'd prefer to have a solution 
> eventually, if not now. But if we do, I estimate we might have the "old 
> fixed" behavior encroach on the "new different" behavior in certain respects, 
> making the code even more complex.
I agree, `C-c C-a' or `M-k' is a good thing to have fixed.  Eventually, i 
would like to use such features.

> I've honestly thought that Dima's patch's main purpose was to fix that bug. 
> And everything else we now complain about are just implementation's 
> side-effects.
It seems you are right.  IMO, we must aim to have the `C-c C-a' stuff 
fixed, but preserving those behaviours that we are complaining here.
Until this aim is fulfilled, i would like to pospone this patch, or 
to have a temporary solution as Mark's one in this thread.

bug#25105: 26.0.50; diff navigation is broken

[Mark Oteiza]

On 07/01/17 at 10:54am, Tino Calancha wrote:
> On Fri, 6 Jan 2017, Dmitry Gutov wrote:
> 
> > I've honestly thought that Dima's patch's main purpose was to fix that
> > bug. And everything else we now complain about are just implementation's
> > side-effects.
> 
> It seems you are right.  IMO, we must aim to have the `C-c C-a' stuff fixed,
> but preserving those behaviours that we are complaining here.
> Until this aim is fulfilled, i would like to pospone this patch, or to have
> a temporary solution as Mark's one in this thread.

I don't consider my patch a solution, as it basically disables diff
refinement due to the design of Dima's patch.

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06.01.2017 11:03, Tino Calancha wrote:
>
> I've honestly thought that Dima's patch's main purpose was to fix that
> bug. And everything else we now complain about are just implementation's
> side-effects.

The goals are described in the git commit message.

The way these goals were met was to make everything consistent, with my
interpretation of what that means.

Taking the new behavior for M-k, C-c C-a and keeping the old behavior
for M-n/M-p is weird. Example:

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.

To reinterate, my desired outcome is to have the current code in place,
and to hook auto-refinement in a diff-mode-hook to make that work in a
way that makes sense. 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.

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.

dima

bug#25105: 26.0.50; diff navigation is broken

[Tino Calancha]

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

bug#25105: 26.0.50; diff navigation is broken

[Dima Kogan]

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

> On Sat, 7 Jan 2017, Dima Kogan wrote:
>
>>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.

This clearly shows that it would be useful to have some binding that
jumps to the beginning of the current hunk. But that's not the same as
moving to the 'next' hunk, which is what 'n' ostensibly is supposed to
do.


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

Right. The choice is whether to revert the new behavior entirely, or to
leave it in with a switch. Since we're looking at a small sample here,
it's not clear which is the right move.


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

Great!

dima

bug#25105: 26.0.50; diff navigation is broken

[Dmitry Gutov]

On 08.01.2017 01:16, Dima Kogan wrote:
> This clearly shows that it would be useful to have some binding that
> jumps to the beginning of the current hunk. But that's not the same as
> moving to the 'next' hunk, which is what 'n' ostensibly is supposed to
> do.

`n' is the easiest key to press, so it should perform navigation that 
the users will find most useful, in general.

If the behavior does not correspond to the command name, it would be 
better to rename the command, or change its docstring, or etc.

But I don't think `diff-hunk-next' jumping to the beginning of the first 
hunk when invoked at the file header, is in any way odd. After all, 
point wasn't inside a hunk before.

> Right. The choice is whether to revert the new behavior entirely, or to
> leave it in with a switch. Since we're looking at a small sample here,
> it's not clear which is the right move.

Note, however, that nobody else has stepped forward to explicitly 
support the new behavior.

If you get around to polling Emacs users at your workplace, or a meetup, 
or so on, please let us know.