[Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Tino Calancha]

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

Hi Dima,

after the patch variable start is not used.  You might want to rename
this var as _start, to avoid:
diff-mode.el:2086:48:Warning: Unused lexical variable ‘start’

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Noam Postavsky]

On Tue, Nov 29, 2016 at 7:07 AM, Tino Calancha <tino.calancha@gmail.com> wrote:
>
>
> @@ -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))))
>
> Hi Dima,
>
> after the patch variable start is not used.  You might want to rename
> this var as _start, to avoid:
> diff-mode.el:2086:48:Warning: Unused lexical variable ‘start’
>

Ah, I probably should have caught that in review. I even had the
warning in my *compilation* buffer, I just didn't look. Fixed now.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dmitry Gutov]

Hi all,

I appreciate the navigation fixes, but some of these changes made 
diff-auto-refine-mode work worse. In particular:

- If there's just one hunk, I routinely pressed `n' to take advantage of 
the auto-refine behavior. And point moved to the end of the hunk.

Now, pressing `n' gives me a ding at the top of the window, then some 
time passes (if the hunk is of a significant size), and auto-refine is 
applied. The point doesn't move.

- Another example: there are several hunks. I press `n', and the first 
one doesn't get refined. It only gets refined if I press `p' to jump 
back over it.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Stefan Monnier]

> I appreciate the navigation fixes, but some of these changes made
> diff-auto-refine-mode work worse. In particular:

BTW, diff-mode would really benefit from having diff-refine hooked into
font-lock since the hunks are just always refined without having to do
anything at all.


        Stefan

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dmitry Gutov]

On 30.11.2016 16:31, Stefan Monnier wrote:

> BTW, diff-mode would really benefit from having diff-refine hooked into
> font-lock since the hunks are just always refined without having to do
> anything at all.

Couldn't a slow font-lock rule make normal navigation worse?

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Stefan Monnier]

>> BTW, diff-mode would really benefit from having diff-refine hooked into
>> font-lock since the hunks are just always refined without having to do
>> anything at all.
> Couldn't a slow font-lock rule make normal navigation worse?

Oh yes, which is why it shouldn't be slow: it should be asynchronous,
instead (which is also why I haven't written it yet: doing it
synchronous and slow would be easy).


        Stefan

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

Hi.


Dmitry Gutov <dgutov@yandex.ru> writes:

> I appreciate the navigation fixes, but some of these changes made
> diff-auto-refine-mode work worse. In particular:
>
> - If there's just one hunk, I routinely pressed `n' to take advantage of
> the auto-refine behavior. And point moved to the end of the hunk.
>
> Now, pressing `n' gives me a ding at the top of the window, then some
> time passes (if the hunk is of a significant size), and auto-refine is
> applied. The point doesn't move.

I'm unclear about what the complaint is. Both the old and new behaviors
made the auto-refinement do its thing. Is the complaint that the point
no longer moves to the end (and if so, why is that "right"?) or is the
"ding" the "problem?


> - Another example: there are several hunks. I press `n', and the first
> one doesn't get refined. It only gets refined if I press `p' to jump
> back over it.

OK. The logic in place is to auto-refine the hunk at point after a
motion, which is why you're seeing this behavior. Are you seeing this
issue only with the first hunk in a buffer and only when you first load
such a buffer? If so, an auto-refinement call from a diff-mode-hook
would solve this. Sounds reasonable, or are such things frowned-upon?

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dmitry Gutov]

On 10.12.2016 03:27, Dima Kogan wrote:

> I'm unclear about what the complaint is. Both the old and new behaviors
> made the auto-refinement do its thing. Is the complaint that the point
> no longer moves to the end (and if so, why is that "right"?) or is the
> "ding" the "problem?

Sorry, let me clarify.

The first complaint is that, yes, point doesn't move to the end anymore. 
Foremost, I'm simply used to the old behavior. Second, I'm not sure how 
to implement on-demand refining of the first hunk in a sane fashion 
without it, see below.

Third, it's simply handy if the first hunk is taller than the height of 
the window, I would navigate to its end to examine it. Fourth, I'd see 
the end of the buffer myself before the next pressing of `n' invokes a 
"ding".

The second complaint is that the command does a "ding" (informing me 
that I did something wrong), and then proceeds to do something useful: 
refining the hunk.

> OK. The logic in place is to auto-refine the hunk at point after a
> motion, which is why you're seeing this behavior. Are you seeing this
> issue only with the first hunk in a buffer and only when you first load
> such a buffer?

Yes.

> If so, an auto-refinement call from a diff-mode-hook
> would solve this. Sounds reasonable, or are such things frowned-upon?

What if the first hunk is big and refining takes a lot of time?

As it is now, I can make the choice to refine or not myself. If 
`diff-mode-hook' does that, I won't even see the diff before refining is 
done.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

On December 10, 2016 2:14:41 AM PST, Dmitry Gutov <dgutov@yandex.ru> wrote:
>On 10.12.2016 03:27, Dima Kogan wrote:
>
>> I'm unclear about what the complaint is. Both the old and new
>behaviors
>> made the auto-refinement do its thing. Is the complaint that the
>point
>> no longer moves to the end (and if so, why is that "right"?) or is
>the
>> "ding" the "problem?
>
>Sorry, let me clarify.
>
>The first complaint is that, yes, point doesn't move to the end
>anymore. 
>Foremost, I'm simply used to the old behavior. Second, I'm not sure how
>
>to implement on-demand refining of the first hunk in a sane fashion 
>without it, see below.
>
>Third, it's simply handy if the first hunk is taller than the height of
>
>the window, I would navigate to its end to examine it. Fourth, I'd see 
>the end of the buffer myself before the next pressing of `n' invokes a 
>"ding".
>
>The second complaint is that the command does a "ding" (informing me 
>that I did something wrong), and then proceeds to do something useful: 
>refining the hunk.
>
>> OK. The logic in place is to auto-refine the hunk at point after a
>> motion, which is why you're seeing this behavior. Are you seeing this
>> issue only with the first hunk in a buffer and only when you first
>load
>> such a buffer?
>
>Yes.
>
>> If so, an auto-refinement call from a diff-mode-hook
>> would solve this. Sounds reasonable, or are such things frowned-upon?
>
>What if the first hunk is big and refining takes a lot of time?
>
>As it is now, I can make the choice to refine or not myself. If 
>`diff-mode-hook' does that, I won't even see the diff before refining
>is 
>done.

It sounds like "auto" refinement isn't what you really want. You want to ask emacs to refine the hunk you are on by invoking diff-next even though diff-refine-hunk makes more sense.

For the other concerns, I can special-case the last hunk, and move to eob for diff-next, and to ding only if we're already at eob to begin with. That will give you the legacy behavior if there's only a single hunk, I think. Seems reasonable?

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dmitry Gutov]

On 10.12.2016 19:27, Dima Kogan wrote:

> It sounds like "auto" refinement isn't what you really want. You want to ask emacs to refine the hunk you are on by invoking diff-next even though diff-refine-hunk makes more sense.

Why bother with two different bindings when one can do? Up until now, 
it's worked nicely.

> For the other concerns, I can special-case the last hunk, and move to eob for diff-next, and to ding only if we're already at eob to begin with.

Sounds good.

> That will give you the legacy behavior if there's only a single hunk, I think. Seems reasonable?

Yes, thank you.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 10.12.2016 19:27, Dima Kogan wrote:
>
>> For the other concerns, I can special-case the last hunk, and move to
>> eob for diff-next, and to ding only if we're already at eob to begin
>> with.

Hi. I played around with this, and I now really don't like this idea
because it would mean that diff-hunk-next no longer always moves to the
next hunk. At the last hunk, diff-hunk-next would stay on the SAME hunk
if this was implemented. It would really make more sense if

- diff-hunk-next moves to next hunk
- if this isn't possible, the point stays where it is, and an error is
  signalled

This is what the behavior in git intends to do.

Furthermore, I think invoking auto-refinement in diff-mode-hook makes
sense. The auto-refinement logic is written (not by me) to auto-refine
the hunk at point when it is visited. And this is triggered not just
with functions such as diff-hunk-next but also with actions such as
turning on auto-refine-mode: diff-auto-refine-mode refines the hunk at
point. Given this, I think it makes sense for us to do what
diff-auto-refine-mode does if we enter diff-mode and auto-refinement is
already on.

And I think that this should happen even if the first hunk is large.
This would indeed be slow, but this is AUTO refinement. If you want to
decide when refinement should or should not happen, auto-refinement is
not what you want. We can add a variable to block auto-refinement for
hunks larger than some size, which probably would be a good idea anyway.

I fixed the incorrect behavior where auto-refinement was kicking in even
when the navigation failed and we signalled an error.

Not attaching any patches yet. Let's agree on an approach, and then talk
implementation.

dima

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dmitry Gutov]

On 12.12.2016 09:28, Dima Kogan wrote:

> Hi. I played around with this, and I now really don't like this idea
> because it would mean that diff-hunk-next no longer always moves to the
> next hunk.

We have other commands that behave this way. beginning-of-defun and 
end-of-defun, for instance. You could say they're named differently, but 
I think the uniformity of behavior is more important.

> At the last hunk, diff-hunk-next would stay on the SAME hunk
> if this was implemented.

It would go beyond the last hunk, wouldn't it?

> Furthermore, I think invoking auto-refinement in diff-mode-hook makes
> sense.

 From a strict and logical point of view, maybe. But not from the user 
convenience point of view. I don't think we should exchange the latter 
for the former.

> And I think that this should happen even if the first hunk is large.
> This would indeed be slow, but this is AUTO refinement.

Again, words are important, but it's up to us as developers to choose 
how to interpret them for maximum user benefit.

> If you want to
> decide when refinement should or should not happen, auto-refinement is
> not what you want.

It's worked quite well before.

> We can add a variable to block auto-refinement for
> hunks larger than some size, which probably would be a good idea anyway.

That sounds finicky. It would be hard to find the value that's good for 
all kinds of diffs, users and differently performing computers.

> I fixed the incorrect behavior where auto-refinement was kicking in even
> when the navigation failed and we signalled an error.

That's something, I guess.

> Not attaching any patches yet. Let's agree on an approach, and then talk
> implementation.

I'm sorry for not proposing a particular course of action. If nobody has 
a good suggestion aside from a revert (which we obviously want to 
avoid), I'll try to dig in sometime later (January at the earliest).

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Tino Calancha]

Dima Kogan <dima@secretsauce.net> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> On 10.12.2016 19:27, Dima Kogan wrote:
>>
>>> For the other concerns, I can special-case the last hunk, and move to
>>> eob for diff-next, and to ding only if we're already at eob to begin
>>> with.
>
> Hi. I played around with this, and I now really don't like this idea
> because it would mean that diff-hunk-next no longer always moves to the
> next hunk. At the last hunk, diff-hunk-next would stay on the SAME hunk
> if this was implemented. It would really make more sense if
>
> - diff-hunk-next moves to next hunk
> - if this isn't possible, the point stays where it is, and an error is
>   signalled
>
> This is what the behavior in git intends to do.
Hi Dima,
after commit 2c8a7e5 the following receipt doesn't work:

M-! git show HEAD RET
C-x o
C-x C-q
M-x diff-mode RET
n
;; Signal error:
diff-beginning-of-file-and-junk: Can’t find the beginning of the file

I use above code very often: before commit 2c8a7e5, it skips the commit
message and jumps to the first hunk.
Is it possible to make above snippet working as before?

Regards,
Tino

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

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

> Hi Dima,
> after commit 2c8a7e5 the following receipt doesn't work:
>
> M-! git show HEAD RET
> C-x o
> C-x C-q
> M-x diff-mode RET
> n
> ;; Signal error:
> diff-beginning-of-file-and-junk: Can t find the beginning of the file
>
> I use above code very often: before commit 2c8a7e5, it skips the commit
> message and jumps to the first hunk.
> Is it possible to make above snippet working as before?

Hi. This was brought up earlier, and I proposed a patch here:

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

I need to test this and a few other small tweaks. Hopefully this week
I'll have an update.

dima

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

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

> Hi Dima,
> after commit 2c8a7e5 the following receipt doesn't work:
>
> M-! git show HEAD RET
> C-x o
> C-x C-q
> M-x diff-mode RET
> n
> ;; Signal error:
> diff-beginning-of-file-and-junk: Can t find the beginning of the file
>
> I use above code very often: before commit 2c8a7e5, it skips the commit
> message and jumps to the first hunk.
> Is it possible to make above snippet working as before?

Hi. I pushed to master the fix to this and the logic to invoke the
auto-refinement only if the preceding motion was successful.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Tino Calancha]

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

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> Hi Dima,
>> after commit 2c8a7e5 the following receipt doesn't work:
>>
>> M-! git show HEAD RET
>> C-x o
>> C-x C-q
>> M-x diff-mode RET
>> n
>> ;; Signal error:
>> diff-beginning-of-file-and-junk: Can t find the beginning of the file
>>
>> I use above code very often: before commit 2c8a7e5, it skips the commit
>> message and jumps to the first hunk.
>> Is it possible to make above snippet working as before?
>
> Hi. I pushed to master the fix to this and the logic to invoke the
> auto-refinement only if the preceding motion was successful.

Thank you.
I think is not working as before.
Consider following snippet code:

M-! git show SHA1 RET
C-x b *Shell Command Output* RET
C-x C-q
M-x diff-mode
n

When i use as SHA1
e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97
previous commands successfully move point to next hunk.

But if i use commit
6b6abe0dba6a9a2e5f78aac3814421886e7a184f
then they don't work; i got the following error:
user-error: No next hunk

I am wondering why the second commit behaves differently than
the first one.

Regards,
Tino

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Tino Calancha]

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

>> Hi. I pushed to master the fix to this and the logic to invoke the
>> auto-refinement only if the preceding motion was successful.
>
> Thank you.
> I think is not working as before.
> Consider following snippet code:
>
> M-! git show SHA1 RET
> C-x b *Shell Command Output* RET
> C-x C-q
> M-x diff-mode
> n
>
> When i use as SHA1
> e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97
> previous commands successfully move point to next hunk.
>
> But if i use commit
> 6b6abe0dba6a9a2e5f78aac3814421886e7a184f
> then they don't work; i got the following error:
> user-error: No next hunk
>
> I am wondering why the second commit behaves differently than
> the first one.

In Emacs-25 the commit message is not considered part of the
first hunk:
So in the previous examples, the point is set in front of
@@ -551,23 +551,7 @@ diff--auto-refine-data
(for e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97)
@@ -768,7 +768,7 @@ diff-beginning-of-file-and-junk
(for 6b6abe0dba6a9a2e5f78aac3814421886e7a184f).

After your changes the commit message is somehow part
of the first hunk; then, from the beginning of buffer
`n' will jump to the second hunk.  This is unconvenient
specially when the commit messages are long.
I prefer the way this is handled in Emacs-25.

Regards,
Tino

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dmitry Gutov]

On 25.12.2016 11:58, Tino Calancha wrote:

> In Emacs-25 the commit message is not considered part of the
> first hunk:

Can we make the file header not part of the hunk either? Then the first 
`n' would move to the first hunk, and this should solve the remaining 
auto-refine problem.

To make things work like they do now, some (all?) commands that act on 
hunks would first see if point is inside a file header, and if so, jump 
forward to the next hunk.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Mark Oteiza]

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. While it may have made fixing the issues
mentioned in the commit message easier, the changes to what "n" and "p"
do at the beginning and end of a diff are not documented, and I didn't
see any discussion of it in the associated bug.

I contend that the new behavior is inconsistent with the behavior of
other outline/thing-with-headers type things in Emacs.  outline-mode,
org-mode, and rst-mode are the first ones that come to mind.

It's also not clear how the introduced oddity with auto-refine is going
to be resolved, unless a way is found to autorefine the first hunk
without there being any user interaction.  Then opening a diff has
inconsistent auto-refining from the start.

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

Mark Oteiza <mvoteiza@udel.edu> writes:

> 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. While it may have made fixing the issues
> mentioned in the commit message easier, the changes to what "n" and "p"
> do at the beginning and end of a diff are not documented, and I didn't
> see any discussion of it in the associated bug.
>
> I contend that the new behavior is inconsistent with the behavior of
> other outline/thing-with-headers type things in Emacs.  outline-mode,
> org-mode, and rst-mode are the first ones that come to mind.

Can you give a specific example of interaction in any of these modes
that is analogous to the off-by-one behavior you're referring to? The
fundamental question is what hunk diff-mode should think the point is
looking at, when it is in some non-diff message above the first hunk.
The answer I chose for the new navigation logic is "first hunk". You
could also choose "invalid hunk, not a hunk at all", which would imply
that C-c C-a and M-ret and such shouldn't work there. Better suggestions
welcome.


> It's also not clear how the introduced oddity with auto-refine is going
> to be resolved, unless a way is found to autorefine the first hunk
> without there being any user interaction.  Then opening a diff has
> inconsistent auto-refining from the start.

I don't use auto-refinement, so didn't notice the breakage. Will look at
it in a bit.

In the meantime, I'm attaching a patch to address the 2nd point in the
bug report (25105). This serves to treat the junk before the first hunk
(i.e. commit message from 'git format-patch') as a file header. Looks
reasonable?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-mode-handle-initial-junk.patch --]
[-- Type: text/x-diff, Size: 544 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 5b48c8d..41476bd 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -768,7 +768,7 @@ diff-beginning-of-file-and-junk
         (setq prevfile nextfile))
     (if (and previndex (numberp prevfile) (< previndex prevfile))
         (setq prevfile previndex))
-    (if (and (numberp prevfile) (<= prevfile start))
+    (if (numberp prevfile)
           (progn
             (goto-char prevfile)
             ;; Now skip backward over the leading junk we may have before the

Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Mark Oteiza]

On 06/12/16 at 11:29pm, Dima Kogan wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > 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. While it may have made fixing the issues
> > mentioned in the commit message easier, the changes to what "n" and "p"
> > do at the beginning and end of a diff are not documented, and I didn't
> > see any discussion of it in the associated bug.
> >
> > I contend that the new behavior is inconsistent with the behavior of
> > other outline/thing-with-headers type things in Emacs.  outline-mode,
> > org-mode, and rst-mode are the first ones that come to mind.
> 
> Can you give a specific example of interaction in any of these modes
> that is analogous to the off-by-one behavior you're referring to?

I wrote about how your changes are make diff-mode _not_ analogous.

> The
> fundamental question is what hunk diff-mode should think the point is
> looking at, when it is in some non-diff message above the first hunk.
> The answer I chose for the new navigation logic is "first hunk". You
> could also choose "invalid hunk, not a hunk at all", which would imply
> that C-c C-a and M-ret and such shouldn't work there. Better suggestions
> welcome.

One might argue that C-c C-a and friends in a file header should apply
all hunks in a file, or perhaps that there should actually be
diff-apply-file commands, etc.

The way n and p worked was not a bug, yet you gratuitously changed them,
and broke auto-refinement.  Why do I have to now hit two keys to refine
the first hunk, and one key to refine the second?

> > It's also not clear how the introduced oddity with auto-refine is going
> > to be resolved, unless a way is found to autorefine the first hunk
> > without there being any user interaction.  Then opening a diff has
> > inconsistent auto-refining from the start.
> 
> I don't use auto-refinement, so didn't notice the breakage. Will look at
> it in a bit.

It's on by default, so this statement perplexes me.

bug#25105: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Mark Oteiza]

On 06/12/16 at 11:29pm, Dima Kogan wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > 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. While it may have made fixing the issues
> > mentioned in the commit message easier, the changes to what "n" and "p"
> > do at the beginning and end of a diff are not documented, and I didn't
> > see any discussion of it in the associated bug.
> >
> > I contend that the new behavior is inconsistent with the behavior of
> > other outline/thing-with-headers type things in Emacs.  outline-mode,
> > org-mode, and rst-mode are the first ones that come to mind.
> 
> Can you give a specific example of interaction in any of these modes
> that is analogous to the off-by-one behavior you're referring to?

I wrote about how your changes are make diff-mode _not_ analogous.

> The
> fundamental question is what hunk diff-mode should think the point is
> looking at, when it is in some non-diff message above the first hunk.
> The answer I chose for the new navigation logic is "first hunk". You
> could also choose "invalid hunk, not a hunk at all", which would imply
> that C-c C-a and M-ret and such shouldn't work there. Better suggestions
> welcome.

One might argue that C-c C-a and friends in a file header should apply
all hunks in a file, or perhaps that there should actually be
diff-apply-file commands, etc.

The way n and p worked was not a bug, yet you gratuitously changed them,
and broke auto-refinement.  Why do I have to now hit two keys to refine
the first hunk, and one key to refine the second?

> > It's also not clear how the introduced oddity with auto-refine is going
> > to be resolved, unless a way is found to autorefine the first hunk
> > without there being any user interaction.  Then opening a diff has
> > inconsistent auto-refining from the start.
> 
> I don't use auto-refinement, so didn't notice the breakage. Will look at
> it in a bit.

It's on by default, so this statement perplexes me.

bug#25105: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation

[Dima Kogan]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

Mark Oteiza <mvoteiza@udel.edu> writes:

> 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. While it may have made fixing the issues
> mentioned in the commit message easier, the changes to what "n" and "p"
> do at the beginning and end of a diff are not documented, and I didn't
> see any discussion of it in the associated bug.
>
> I contend that the new behavior is inconsistent with the behavior of
> other outline/thing-with-headers type things in Emacs.  outline-mode,
> org-mode, and rst-mode are the first ones that come to mind.

Can you give a specific example of interaction in any of these modes
that is analogous to the off-by-one behavior you're referring to? The
fundamental question is what hunk diff-mode should think the point is
looking at, when it is in some non-diff message above the first hunk.
The answer I chose for the new navigation logic is "first hunk". You
could also choose "invalid hunk, not a hunk at all", which would imply
that C-c C-a and M-ret and such shouldn't work there. Better suggestions
welcome.


> It's also not clear how the introduced oddity with auto-refine is going
> to be resolved, unless a way is found to autorefine the first hunk
> without there being any user interaction.  Then opening a diff has
> inconsistent auto-refining from the start.

I don't use auto-refinement, so didn't notice the breakage. Will look at
it in a bit.

In the meantime, I'm attaching a patch to address the 2nd point in the
bug report (25105). This serves to treat the junk before the first hunk
(i.e. commit message from 'git format-patch') as a file header. Looks
reasonable?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-mode-handle-initial-junk.patch --]
[-- Type: text/x-diff, Size: 544 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 5b48c8d..41476bd 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -768,7 +768,7 @@ diff-beginning-of-file-and-junk
         (setq prevfile nextfile))
     (if (and previndex (numberp prevfile) (< previndex prevfile))
         (setq prevfile previndex))
-    (if (and (numberp prevfile) (<= prevfile start))
+    (if (numberp prevfile)
           (progn
             (goto-char prevfile)
             ;; Now skip backward over the leading junk we may have before the