all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
@ 2016-11-29 12:07 Tino Calancha
  2016-11-30  0:03 ` Noam Postavsky
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Tino Calancha @ 2016-11-29 12:07 UTC (permalink / raw)
  To: Dima Kogan; +Cc: tino.calancha, emacs-devel



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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha
@ 2016-11-30  0:03 ` Noam Postavsky
  2016-11-30 14:22 ` Dmitry Gutov
  2016-12-06  2:03 ` Mark Oteiza
  2 siblings, 0 replies; 23+ messages in thread
From: Noam Postavsky @ 2016-11-30  0:03 UTC (permalink / raw)
  To: Tino Calancha; +Cc: Emacs developers, Dima Kogan

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha
  2016-11-30  0:03 ` Noam Postavsky
@ 2016-11-30 14:22 ` Dmitry Gutov
  2016-11-30 14:31   ` Stefan Monnier
  2016-12-10  1:27   ` Dima Kogan
  2016-12-06  2:03 ` Mark Oteiza
  2 siblings, 2 replies; 23+ messages in thread
From: Dmitry Gutov @ 2016-11-30 14:22 UTC (permalink / raw)
  To: Tino Calancha, Dima Kogan; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-30 14:22 ` Dmitry Gutov
@ 2016-11-30 14:31   ` Stefan Monnier
  2016-11-30 14:34     ` Dmitry Gutov
  2016-12-10  1:27   ` Dima Kogan
  1 sibling, 1 reply; 23+ messages in thread
From: Stefan Monnier @ 2016-11-30 14:31 UTC (permalink / raw)
  To: emacs-devel

> 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




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-30 14:31   ` Stefan Monnier
@ 2016-11-30 14:34     ` Dmitry Gutov
  2016-11-30 15:07       ` Stefan Monnier
  0 siblings, 1 reply; 23+ messages in thread
From: Dmitry Gutov @ 2016-11-30 14:34 UTC (permalink / raw)
  To: Stefan Monnier, emacs-devel

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?



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-30 14:34     ` Dmitry Gutov
@ 2016-11-30 15:07       ` Stefan Monnier
  0 siblings, 0 replies; 23+ messages in thread
From: Stefan Monnier @ 2016-11-30 15:07 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha
  2016-11-30  0:03 ` Noam Postavsky
  2016-11-30 14:22 ` Dmitry Gutov
@ 2016-12-06  2:03 ` Mark Oteiza
  2016-12-07  7:29   ` Dima Kogan
  2016-12-07  7:29   ` Dima Kogan
  2 siblings, 2 replies; 23+ messages in thread
From: Mark Oteiza @ 2016-12-06  2:03 UTC (permalink / raw)
  To: Tino Calancha; +Cc: emacs-devel, Dima Kogan


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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#25105: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-06  2:03 ` Mark Oteiza
  2016-12-07  7:29   ` Dima Kogan
@ 2016-12-07  7:29   ` Dima Kogan
  1 sibling, 0 replies; 23+ messages in thread
From: Dima Kogan @ 2016-12-07  7:29 UTC (permalink / raw)
  To: Mark Oteiza; +Cc: 25105, Tino Calancha, emacs-devel

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

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-06  2:03 ` Mark Oteiza
@ 2016-12-07  7:29   ` Dima Kogan
  2017-01-06  2:58     ` Mark Oteiza
  2017-01-06  2:58     ` bug#25105: " Mark Oteiza
  2016-12-07  7:29   ` Dima Kogan
  1 sibling, 2 replies; 23+ messages in thread
From: Dima Kogan @ 2016-12-07  7:29 UTC (permalink / raw)
  To: Mark Oteiza; +Cc: 25105, emacs-devel, Tino Calancha

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

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-11-30 14:22 ` Dmitry Gutov
  2016-11-30 14:31   ` Stefan Monnier
@ 2016-12-10  1:27   ` Dima Kogan
  2016-12-10 10:14     ` Dmitry Gutov
  1 sibling, 1 reply; 23+ messages in thread
From: Dima Kogan @ 2016-12-10  1:27 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: emacs-devel, Tino Calancha

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?



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-10  1:27   ` Dima Kogan
@ 2016-12-10 10:14     ` Dmitry Gutov
  2016-12-10 17:27       ` Dima Kogan
  0 siblings, 1 reply; 23+ messages in thread
From: Dmitry Gutov @ 2016-12-10 10:14 UTC (permalink / raw)
  To: Dima Kogan; +Cc: emacs-devel, Tino Calancha

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-10 10:14     ` Dmitry Gutov
@ 2016-12-10 17:27       ` Dima Kogan
  2016-12-11 11:07         ` Dmitry Gutov
  0 siblings, 1 reply; 23+ messages in thread
From: Dima Kogan @ 2016-12-10 17:27 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: emacs-devel, Tino Calancha

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?




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-10 17:27       ` Dima Kogan
@ 2016-12-11 11:07         ` Dmitry Gutov
  2016-12-12  7:28           ` Dima Kogan
  0 siblings, 1 reply; 23+ messages in thread
From: Dmitry Gutov @ 2016-12-11 11:07 UTC (permalink / raw)
  To: Dima Kogan; +Cc: Tino Calancha, emacs-devel

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-11 11:07         ` Dmitry Gutov
@ 2016-12-12  7:28           ` Dima Kogan
  2016-12-16  1:05             ` Dmitry Gutov
  2016-12-20  2:22             ` Tino Calancha
  0 siblings, 2 replies; 23+ messages in thread
From: Dima Kogan @ 2016-12-12  7:28 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Tino Calancha, emacs-devel

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-12  7:28           ` Dima Kogan
@ 2016-12-16  1:05             ` Dmitry Gutov
  2016-12-20  2:22             ` Tino Calancha
  1 sibling, 0 replies; 23+ messages in thread
From: Dmitry Gutov @ 2016-12-16  1:05 UTC (permalink / raw)
  To: Dima Kogan; +Cc: emacs-devel, Tino Calancha

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-12  7:28           ` Dima Kogan
  2016-12-16  1:05             ` Dmitry Gutov
@ 2016-12-20  2:22             ` Tino Calancha
  2016-12-20  7:31               ` Dima Kogan
  2016-12-25  6:52               ` Dima Kogan
  1 sibling, 2 replies; 23+ messages in thread
From: Tino Calancha @ 2016-12-20  2:22 UTC (permalink / raw)
  To: Dima Kogan; +Cc: emacs-devel, Tino Calancha, Dmitry Gutov

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-20  2:22             ` Tino Calancha
@ 2016-12-20  7:31               ` Dima Kogan
  2016-12-25  6:52               ` Dima Kogan
  1 sibling, 0 replies; 23+ messages in thread
From: Dima Kogan @ 2016-12-20  7:31 UTC (permalink / raw)
  To: Tino Calancha; +Cc: emacs-devel, Dmitry Gutov

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-20  2:22             ` Tino Calancha
  2016-12-20  7:31               ` Dima Kogan
@ 2016-12-25  6:52               ` Dima Kogan
  2016-12-25  9:48                 ` Tino Calancha
  1 sibling, 1 reply; 23+ messages in thread
From: Dima Kogan @ 2016-12-25  6:52 UTC (permalink / raw)
  To: Tino Calancha; +Cc: emacs-devel, Dmitry Gutov

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-25  6:52               ` Dima Kogan
@ 2016-12-25  9:48                 ` Tino Calancha
  2016-12-25  9:58                   ` Tino Calancha
  0 siblings, 1 reply; 23+ messages in thread
From: Tino Calancha @ 2016-12-25  9:48 UTC (permalink / raw)
  To: Dima Kogan; +Cc: Dmitry Gutov, Tino Calancha, emacs-devel

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-25  9:48                 ` Tino Calancha
@ 2016-12-25  9:58                   ` Tino Calancha
  2016-12-25 14:10                     ` Dmitry Gutov
  0 siblings, 1 reply; 23+ messages in thread
From: Tino Calancha @ 2016-12-25  9:58 UTC (permalink / raw)
  To: Dima Kogan; +Cc: Tino Calancha, emacs-devel, Dmitry Gutov

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-25  9:58                   ` Tino Calancha
@ 2016-12-25 14:10                     ` Dmitry Gutov
  0 siblings, 0 replies; 23+ messages in thread
From: Dmitry Gutov @ 2016-12-25 14:10 UTC (permalink / raw)
  To: Tino Calancha, Dima Kogan; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#25105: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-07  7:29   ` Dima Kogan
  2017-01-06  2:58     ` Mark Oteiza
@ 2017-01-06  2:58     ` Mark Oteiza
  1 sibling, 0 replies; 23+ messages in thread
From: Mark Oteiza @ 2017-01-06  2:58 UTC (permalink / raw)
  To: Dima Kogan; +Cc: 25105, Tino Calancha, emacs-devel

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.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation
  2016-12-07  7:29   ` Dima Kogan
@ 2017-01-06  2:58     ` Mark Oteiza
  2017-01-06  2:58     ` bug#25105: " Mark Oteiza
  1 sibling, 0 replies; 23+ messages in thread
From: Mark Oteiza @ 2017-01-06  2:58 UTC (permalink / raw)
  To: Dima Kogan; +Cc: 25105, emacs-devel, Tino Calancha

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2017-01-06  2:58 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha
2016-11-30  0:03 ` Noam Postavsky
2016-11-30 14:22 ` Dmitry Gutov
2016-11-30 14:31   ` Stefan Monnier
2016-11-30 14:34     ` Dmitry Gutov
2016-11-30 15:07       ` Stefan Monnier
2016-12-10  1:27   ` Dima Kogan
2016-12-10 10:14     ` Dmitry Gutov
2016-12-10 17:27       ` Dima Kogan
2016-12-11 11:07         ` Dmitry Gutov
2016-12-12  7:28           ` Dima Kogan
2016-12-16  1:05             ` Dmitry Gutov
2016-12-20  2:22             ` Tino Calancha
2016-12-20  7:31               ` Dima Kogan
2016-12-25  6:52               ` Dima Kogan
2016-12-25  9:48                 ` Tino Calancha
2016-12-25  9:58                   ` Tino Calancha
2016-12-25 14:10                     ` Dmitry Gutov
2016-12-06  2:03 ` Mark Oteiza
2016-12-07  7:29   ` Dima Kogan
2017-01-06  2:58     ` Mark Oteiza
2017-01-06  2:58     ` bug#25105: " Mark Oteiza
2016-12-07  7:29   ` Dima Kogan

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.