bug#32991: 27.0.50; diff-auto-refine-mode a no-op

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> commit f8b1e40fb63b0a6bc6692cc0bc84e5f5e65c2644
> Author: Stefan Monnier <[hidden email]>
> Date:   Tue Jul 10 22:52:21 2018 -0400

>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
   
>     Remove redundant :group arguments.
>     (diff-font-lock-refine): New var.
>     (diff--refine-hunk): New function, extracted from diff-refine-hunk.
>     (diff-refine-hunk): Use it.
>     (diff--font-lock-refine--refresh): New function.
>     (diff--font-lock-refined): New function.
>     (diff-font-lock-keywords): Use it.

Looks like this commit makes diff-auto-refine-mode a no-op, since
hunks are now always refined.  Is that the intention?

Also, with automatic refinement always on, does the following part of
diff-mode.el still require refining the hunk?  It must be repeating
font lock's job (unless font lock is switched off, of course).

  (easy-mmode-define-navigation
   diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
   (when diff-auto-refine-mode
     (unless (prog1 diff--auto-refine-data
               (setq diff--auto-refine-data
                     (cons (current-buffer) (point-marker))))
       (run-at-time 0.0 nil
                    (lambda ()
                      (when diff--auto-refine-data
                        (let ((buffer (car diff--auto-refine-data))
                              (point (cdr diff--auto-refine-data)))
                          (setq diff--auto-refine-data nil)
                          (with-local-quit
                            (when (buffer-live-p buffer)
                              (with-current-buffer buffer
                                (save-excursion
                                  (goto-char point)
                                  (diff-refine-hunk))))))))))))



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
>>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
> Looks like this commit makes diff-auto-refine-mode a no-op, since
> hunks are now always refined.  Is that the intention?

Yes.  Of course, it depends on the value of diff-font-lock-refine.

> Also, with automatic refinement always on, does the following part of
> diff-mode.el still require refining the hunk?

I believe this is the same question.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Mon, 08 Oct 2018 17:00:31 -0400
>
> >>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
> > Looks like this commit makes diff-auto-refine-mode a no-op, since
> > hunks are now always refined.  Is that the intention?
>
> Yes.  Of course, it depends on the value of diff-font-lock-refine.

Ok.  In that case, do we still need diff-auto-refine-mode?

> > Also, with automatic refinement always on, does the following part of
> > diff-mode.el still require refining the hunk?
>
> I believe this is the same question.

I'm not sure I understand.  If the default value of
diff-font-lock-refine results in hunks being automatically refined by
font-lock, why should the navigation function possibly re-do the
refining already done by font-lock?



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
>> >>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
>> > Looks like this commit makes diff-auto-refine-mode a no-op, since
>> > hunks are now always refined.  Is that the intention?
>> Yes.  Of course, it depends on the value of diff-font-lock-refine.
> Ok.  In that case, do we still need diff-auto-refine-mode?

It should probably be merged with diff-font-lock-refine (e.g. have
3 possible values).

>> > Also, with automatic refinement always on, does the following part of
>> > diff-mode.el still require refining the hunk?
>> I believe this is the same question.
> I'm not sure I understand.

I'm saying that this is the same question as that discussed in the
previous paragraph: one looks at the config var, and the other looks at
the code implementing the functionality related to the config var, but
the two are inextricably linked.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Tue, 09 Oct 2018 15:54:55 -0400
>
> >> >>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
> >> > Looks like this commit makes diff-auto-refine-mode a no-op, since
> >> > hunks are now always refined.  Is that the intention?
> >> Yes.  Of course, it depends on the value of diff-font-lock-refine.
> > Ok.  In that case, do we still need diff-auto-refine-mode?
>
> It should probably be merged with diff-font-lock-refine (e.g. have
> 3 possible values).

So diff-font-lock-refine could have 3 possible values, t and nil as it
has now, and 'auto for doing the refinement as you navigate to hunks
with "n" or "p".

While we're on this point, what is the use case for offering automatic
refining during navigation if we can now offer "just-in-time"
highlighting via font-lock?  The new, instant refining seems more
logical and less surprising to the user.  It could be even better if
C-c C-b could interactively toggle the refining of the hunk at point
(for those times when the refining turns out to be an eye-sore).

Another advantage of the new font-lock based approach is that with a
little change we might be able to customize how much highlighting is
done in a diff-mode buffer via "font-lock-maximum-decoration".



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
> So diff-font-lock-refine could have 3 possible values, t and nil as it
> has now, and 'auto for doing the refinement as you navigate to hunks
> with "n" or "p".

I don't see what the navigation-triggered refinement has of
"auto"matism, compared to font-lock, so I wouldn't use `auto` here.
I'd rather go with something like `nil`, `font-lock`, or `navigation`
(and default to `font-lock`).

> While we're on this point, what is the use case for offering automatic
> refining during navigation if we can now offer "just-in-time"
> highlighting via font-lock?

Good question.  Maybe it's just not worth it and we should simply get
rid of the old navigation-triggered refinement.  This said, the new
font-lock thingy can be problematic if the diff takes too much time
since it happens within jit-lock with inhibit-quit set to a non-nil
value, so it completely freezes your Emacs session, whereas if it
happens during `n` you can stop it with C-g.

Hopefully, we can fix this problem by calling `diff` asynchronously so
it can't block Emacs.

> It could be even better if C-c C-b could interactively toggle the
> refining of the hunk at point (for those times when the refining turns
> out to be an eye-sore).

Sounds good (but note that diff-refine-hunk can also be useful to
*refresh* the fine highlighting, e.g. after manually editing a hunk).

> Another advantage of the new font-lock based approach is that with a
> little change we might be able to customize how much highlighting is
> done in a diff-mode buffer via "font-lock-maximum-decoration".

font-lock-maximum-decoration is fundamentally flawed in that it is
unidimensional (you can only have more or less) whereas often some users
may want more of one kind of info and less of another.

E.g. how would you order the "decoration levels" between:

    basic
    basic + refine
    basic + prettify
    basic + prettify + refine

The first and last are easy, but there's no natural ordering between the
middle two.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Wed, 10 Oct 2018 15:21:27 -0400
>
> I don't see what the navigation-triggered refinement has of
> "auto"matism, compared to font-lock, so I wouldn't use `auto` here.
> I'd rather go with something like `nil`, `font-lock`, or `navigation`
> (and default to `font-lock`).

Sounds good.

> > While we're on this point, what is the use case for offering automatic
> > refining during navigation if we can now offer "just-in-time"
> > highlighting via font-lock?
>
> Good question.  Maybe it's just not worth it and we should simply get
> rid of the old navigation-triggered refinement.  This said, the new
> font-lock thingy can be problematic if the diff takes too much time
> since it happens within jit-lock with inhibit-quit set to a non-nil
> value, so it completely freezes your Emacs session, whereas if it
> happens during `n` you can stop it with C-g.
>
> Hopefully, we can fix this problem by calling `diff` asynchronously so
> it can't block Emacs.

I was able to produce such a case where Emacs froze for 30 seconds on
opening a 2000-line (junk) diff containing one large hunk.

It would indeed be convenient to have "diff" called asynchronously
(assuming this is the "diff" that runs in smerge-refine-regions).  Is
it a matter of using "start-process" instead of "call-process"?

> > It could be even better if C-c C-b could interactively toggle the
> > refining of the hunk at point (for those times when the refining turns
> > out to be an eye-sore).
>
> Sounds good (but note that diff-refine-hunk can also be useful to
> *refresh* the fine highlighting, e.g. after manually editing a hunk).

Ok, then switching off the refining might fit better on a prefix
argument, or in a new command.

> font-lock-maximum-decoration is fundamentally flawed in that it is
> unidimensional (you can only have more or less) whereas often some users
> may want more of one kind of info and less of another.
>
> E.g. how would you order the "decoration levels" between:
>
>     basic
>     basic + refine
>     basic + prettify
>     basic + prettify + refine
>
> The first and last are easy, but there's no natural ordering between the
> middle two.

Good point.  Maybe we could start with an ordering of just
"basic"/"basic + refine", since the diff-font-lock-prettify option is
brand new and seems to be more about hiding text than decorating it,
IIUC.



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
>> Good question.  Maybe it's just not worth it and we should simply get
>> rid of the old navigation-triggered refinement.  This said, the new
>> font-lock thingy can be problematic if the diff takes too much time
>> since it happens within jit-lock with inhibit-quit set to a non-nil
>> value, so it completely freezes your Emacs session, whereas if it
>> happens during `n` you can stop it with C-g.
>> Hopefully, we can fix this problem by calling `diff` asynchronously so
>> it can't block Emacs.
> I was able to produce such a case where Emacs froze for 30 seconds on
> opening a 2000-line (junk) diff containing one large hunk.

I bumped into one a week ago where I killed `diff` after several minutes.

> Is it a matter of using "start-process" instead of "call-process"?

That's the starting point, yes, but it also involves moving the
"subsequent" code to the process's sentinel.

>> > It could be even better if C-c C-b could interactively toggle the
>> > refining of the hunk at point (for those times when the refining turns
>> > out to be an eye-sore).
>> Sounds good (but note that diff-refine-hunk can also be useful to
>> *refresh* the fine highlighting, e.g. after manually editing a hunk).
> Ok, then switching off the refining might fit better on a prefix
> argument, or in a new command.

Could be, tho maybe you can make it DWIM enough (i.e. turn off if
there's nothing to refresh).

> Good point.  Maybe we could start with an ordering of just
> "basic"/"basic + refine", since the diff-font-lock-prettify option is
> brand new and seems to be more about hiding text than decorating it,
> IIUC.

Since I find font-lock-maximum-decoration fundamentally flawed, I'm
rather in the business of deprecating it than increasing its use.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
In reply to this post by Stefan Monnier
> From: Stefan Monnier <[hidden email]>
> Date: Wed, 10 Oct 2018 15:21:27 -0400
>
> > So diff-font-lock-refine could have 3 possible values, t and nil as it
> > has now, and 'auto for doing the refinement as you navigate to hunks
> > with "n" or "p".
>
> I don't see what the navigation-triggered refinement has of
> "auto"matism, compared to font-lock, so I wouldn't use `auto` here.
> I'd rather go with something like `nil`, `font-lock`, or `navigation`
> (and default to `font-lock`).

After revisiting this, the values "nil", "font-lock", and "navigation"
sound fine, though if we end up using those we might want to change
the name of "diff-font-lock-refine" to just "diff-refine" (since
"font-lock" may not be involved in the refinement).



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> Date: Sun, 13 Jan 2019 15:36:48 +0100
> From: [hidden email] (Charles A. Roelli)
>
> After revisiting this, the values "nil", "font-lock", and "navigation"
> sound fine, though if we end up using those we might want to change
> the name of "diff-font-lock-refine" to just "diff-refine" (since
> "font-lock" may not be involved in the refinement).

Here is a possible implementation:

* etc/NEWS (Diff mode): Explain renamed 'diff-refine' variable,
mention removal of 'diff-auto-refine-mode', and explain
user-visible changes to 'diff-hunk-next' and 'diff-hunk-prev'.
* lisp/vc/diff-mode.el (diff-font-lock-refine): Rename to
'diff-refine' and allow choices nil, 'font-lock' and 'navigation'.
(diff-auto-refine-mode): Remove as it has been superseded by the
user option 'diff-refine'.
(diff-navigate-maybe-refine): New function for use in the new
functions 'diff-hunk-next' and 'diff-hunk-prev'.
(diff-hunk-next, diff-hunk-prev): Rewrite as defuns instead of
using easy-mmode-define-navigation.  Add a new optional argument
'interactive', which is needed to prevent other callers from
inadvertently causing refinement to happen when 'diff-refine' is
set to 'navigation'.  Leave out the ability of these commands to
change user narrowing and recenter the window, as this does not
match the behavior of other general movement commands.
(diff--font-lock-refined): Change it to only trigger when the new
variable 'diff-refine' has the value 'font-lock'.
* lisp/vc/smerge-mode.el (smerge-next, smerge-prev): Check
that 'diff-refine' is set to 'navigation' instead of checking
'diff-auto-refine-mode' when deciding whether to refine a
conflict.
------

diff --git a/etc/NEWS b/etc/NEWS
index bb214f2..3163577 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -428,8 +428,14 @@ values.
 and compares their entire trees.
 
 ** Diff mode
-*** Hunks are now automatically refined by default.
-To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
+*** Hunks are now automatically refined by font-lock.
+To disable refinement, set the new defcustom 'diff-refine' to nil.
+To get back the old behavior where hunks are refined as you navigate
+through a diff, set 'diff-refine' to the symbol 'navigate'.
+*** diff-auto-refine-mode has been removed.
+*** diff-hunk-next and diff-hunk-prev
+These commands no longer recenter the window or change the narrowing
+of the buffer.
 
 +++
 *** Better syntax highlighting of Diff hunks.
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 5d6cc6f..2d77f6a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -94,10 +94,18 @@ diff-mode-hook
   :type 'hook
   :options '(diff-delete-empty-files diff-make-unified))
 
-(defcustom diff-font-lock-refine t
-  "If non-nil, font-lock highlighting includes hunk refinement."
+(defcustom diff-refine 'font-lock
+  "If non-nil, enable hunk refinement.
+
+The value `font-lock' means to refine during font-lock.
+The value `navigation' means to refine each hunk as you visit it
+with `diff-hunk-next' or `diff-hunk-prev'.
+
+You can always manually refine a hunk with `diff-refine-hunk'."
   :version "27.1"
-  :type 'boolean)
+  :type '(choice (const :tag "Don't refine hunks" nil)
+                 (const :tag "Refine hunks during font-lock" font-lock)
+                 (const :tag "Refine hunks during navigation" navigation)))
 
 (defcustom diff-font-lock-prettify nil
   "If non-nil, font-lock will try and make the format prettier."
@@ -254,18 +262,6 @@ diff-minor-mode-prefix
   `((,diff-minor-mode-prefix . ,diff-mode-shared-map))
   "Keymap for `diff-minor-mode'.  See also `diff-mode-shared-map'.")
 
-(define-minor-mode diff-auto-refine-mode
-  "Toggle automatic diff hunk finer highlighting (Diff Auto Refine mode).
-
-Diff Auto Refine mode is a buffer-local minor mode used with
-`diff-mode'.  When enabled, Emacs automatically highlights
-changes in detail as the user visits hunks.  When transitioning
-from disabled to enabled, it tries to refine the current hunk, as
-well."
-  :group 'diff-mode :init-value t :lighter nil ;; " Auto-Refine"
-  (when diff-auto-refine-mode
-    (condition-case-unless-debug nil (diff-refine-hunk) (error nil))))
-
 ;;;;
 ;;;; font-lock support
 ;;;;
@@ -619,26 +615,59 @@ diff-end-of-file
 
 (defvar diff--auto-refine-data nil)
 
-;; Define diff-{hunk,file}-{prev,next}
-(easy-mmode-define-navigation
- diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
- (when diff-auto-refine-mode
-   (unless (prog1 diff--auto-refine-data
-             (setq diff--auto-refine-data
-                   (cons (current-buffer) (point-marker))))
-     (run-at-time 0.0 nil
-                  (lambda ()
-                    (when diff--auto-refine-data
-                      (let ((buffer (car diff--auto-refine-data))
-                            (point (cdr diff--auto-refine-data)))
-                        (setq diff--auto-refine-data nil)
-                        (with-local-quit
-                          (when (buffer-live-p buffer)
-                            (with-current-buffer buffer
-                              (save-excursion
-                                (goto-char point)
-                                (diff-refine-hunk))))))))))))
-
+(defun diff-navigate-maybe-refine ()
+  (when (eq diff-refine 'navigation)
+    (unless (prog1 diff--auto-refine-data
+              (setq diff--auto-refine-data
+                    (cons (current-buffer) (point-marker))))
+      (run-at-time 0.0 nil
+                   (lambda ()
+                     (when diff--auto-refine-data
+                       (let ((buffer (car diff--auto-refine-data))
+                             (point (cdr diff--auto-refine-data)))
+                         (setq diff--auto-refine-data nil)
+                         (with-local-quit
+                           (when (buffer-live-p buffer)
+                             (with-current-buffer buffer
+                               (save-excursion
+                                 (goto-char point)
+                                 (diff-refine-hunk))))))))))))
+
+(defun diff-hunk-next (&optional count interactive)
+  "Go to the next COUNT'th hunk.
+
+Interactively, COUNT is the prefix numeric argument, and defaults to 1.
+
+If INTERACTIVE is non-nil and `diff-refine' is set to
+`navigation', refine the hunk moved to."
+  (interactive "p\np")
+  (unless count (setq count 1))
+  (if (< count 0)
+      (diff-hunk-prev (- count))
+    (if (looking-at diff-hunk-header-re) (setq count (1+ count)))
+    (if (not (re-search-forward diff-hunk-header-re nil t count))
+        (if (looking-at diff-hunk-header-re)
+            (goto-char (diff-end-of-hunk))
+          (user-error "No next hunk"))
+      (goto-char (match-beginning 0))
+      (if interactive (diff-navigate-maybe-refine)))))
+
+(defun diff-hunk-prev (&optional count interactive)
+  "Go to the previous COUNT'th hunk.
+
+Interactively, COUNT is the prefix numeric argument, and defaults to 1.
+
+If INTERACTIVE is non-nil and `diff-refine' is set to
+`navigation', refine the hunk moved to."
+  (interactive "p\np")
+  (unless count (setq count 1))
+  (if (< count 0)
+      (diff-hunk-next (- count))
+    (unless (re-search-backward diff-hunk-header-re nil t count)
+      (user-error "No previous hunk"))
+    (if interactive (diff-navigate-maybe-refine))))
+
+;; Define diff-file-{prev,next}
 (easy-mmode-define-navigation
  diff-file diff-file-header-re "file" diff-end-of-file)
 
@@ -2125,7 +2154,7 @@ diff--refine-hunk
 
 (defun diff--font-lock-refined (max)
   "Apply hunk refinement from font-lock."
-  (when diff-font-lock-refine
+  (when (eq diff-refine 'font-lock)
     (when (get-char-property (point) 'diff--font-lock-refined)
       ;; Refinement works over a complete hunk, whereas font-lock limits itself
       ;; to highlighting smallish chunks between point..max, so we may be
diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index 569797e..e700e32 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -44,7 +44,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
-(require 'diff-mode)                    ;For diff-auto-refine-mode.
+(require 'diff-mode)                    ;For diff-refine.
 (require 'newcomment)
 
 ;;; The real definition comes later.
@@ -264,7 +264,7 @@ font-lock-keywords
 
 ;; Define smerge-next and smerge-prev
 (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil
-  (if diff-auto-refine-mode
+  (if (eq diff-refine 'navigation)
       (condition-case nil (smerge-refine) (error nil))))
 
 (defconst smerge-match-names ["conflict" "upper" "base" "lower"])





Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
> -(defcustom diff-font-lock-refine t
> -  "If non-nil, font-lock highlighting includes hunk refinement."
> +(defcustom diff-refine 'font-lock
> +  "If non-nil, enable hunk refinement.
> +
> +The value `font-lock' means to refine during font-lock.
> +The value `navigation' means to refine each hunk as you visit it
> +with `diff-hunk-next' or `diff-hunk-prev'.
> +
> +You can always manually refine a hunk with `diff-refine-hunk'."
>    :version "27.1"
> -  :type 'boolean)
> +  :type '(choice (const :tag "Don't refine hunks" nil)
> +                 (const :tag "Refine hunks during font-lock" font-lock)
> +                 (const :tag "Refine hunks during navigation" navigation)))

Good.

> -(define-minor-mode diff-auto-refine-mode
> -  "Toggle automatic diff hunk finer highlighting (Diff Auto Refine mode).
> -
> -Diff Auto Refine mode is a buffer-local minor mode used with
> -`diff-mode'.  When enabled, Emacs automatically highlights
> -changes in detail as the user visits hunks.  When transitioning
> -from disabled to enabled, it tries to refine the current hunk, as
> -well."
> -  :group 'diff-mode :init-value t :lighter nil ;; " Auto-Refine"
> -  (when diff-auto-refine-mode
> -    (condition-case-unless-debug nil (diff-refine-hunk) (error nil))))

I think for backward compatiblity reason, we should keep this minor
mode, tho mark it obsolete and change its code to set diff-refine to
`navigation`.

> -;; Define diff-{hunk,file}-{prev,next}
> -(easy-mmode-define-navigation
> - diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
[...]
> +(defun diff-hunk-next (&optional count interactive)
[...]
> +(defun diff-hunk-prev (&optional count interactive)

This change seems unrelated.  I'd rather we stick to the refinement itself.

>  ;; Define smerge-next and smerge-prev
>  (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil
> -  (if diff-auto-refine-mode
> +  (if (eq diff-refine 'navigation)
>        (condition-case nil (smerge-refine) (error nil))))

Hmm... I want to set my `diff-refined` to `font-lock` yet I also want my
smerge conflicts to be refined when I navigate to them.
Maybe the test here should be just whether diff-refine is non-nil?


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Sun, 13 Jan 2019 18:33:15 -0500
>
> > -(define-minor-mode diff-auto-refine-mode
> > -[...]
>
> I think for backward compatiblity reason, we should keep this minor
> mode, tho mark it obsolete and change its code to set diff-refine to
> `navigation`.

Good point, this is in the amended change below.
 
> > -;; Define diff-{hunk,file}-{prev,next}
> > -(easy-mmode-define-navigation
> > - diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
> [...]
> > +(defun diff-hunk-next (&optional count interactive)
> [...]
> > +(defun diff-hunk-prev (&optional count interactive)
>
> This change seems unrelated.  I'd rather we stick to the refinement itself.

Without this change, other functions in diff-mode (such as
diff--font-lock-syntax) calling diff-hunk-next accidentally trigger
hunk refinement if 'diff-refine' is 'navigation'.  Hence we need a
reliable way to tell whether the navigation has been caused by the
user (i.e., with argument 'interactive' set to t) or the program
('interactive' set to nil).

Incidentally, I left out the auto-recentering and buffer
restriction-changing parts of the old diff-hunk-next and
diff-hunk-prev, since these behaviors do not match other navigation
commands.

> >  ;; Define smerge-next and smerge-prev
> >  (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil
> > -  (if diff-auto-refine-mode
> > +  (if (eq diff-refine 'navigation)
> >        (condition-case nil (smerge-refine) (error nil))))
>
> Hmm... I want to set my `diff-refined` to `font-lock` yet I also want my
> smerge conflicts to be refined when I navigate to them.
> Maybe the test here should be just whether diff-refine is non-nil?

Yes, I've fixed that below.  Thanks for reviewing.


* etc/NEWS (Diff mode): Explain renamed 'diff-refine'
variable, mention deprecation and disabling of
'diff-auto-refine-mode', and explain user-visible changes to
'diff-hunk-next' and 'diff-hunk-prev'.
* lisp/vc/diff-mode.el (diff-font-lock-refine): Rename to
'diff-refine' and allow choices nil, 'font-lock' and 'navigation'.
(diff-auto-refine-mode): Disable it by default and make it set
'diff-refine' appropriately to keep backward compatibility.
(diff-navigate-maybe-refine): New function for use in the new
functions 'diff-hunk-next' and 'diff-hunk-prev'.
(diff-hunk-next, diff-hunk-prev): Rewrite as defuns instead of
using easy-mmode-define-navigation.  Add a new optional argument
'interactive', which is needed to prevent other callers from
inadvertently causing refinement to happen when 'diff-refine' is
set to 'navigation'.  Leave out the ability of these commands to
change user narrowing and recenter the window, as this does not
match the behavior of other general movement commands.
(diff--font-lock-refined): Change it to only trigger when the new
variable 'diff-refine' has the value 'font-lock'.
* lisp/vc/smerge-mode.el (smerge-next, smerge-prev): Check
that 'diff-refine' is set instead of checking
'diff-auto-refine-mode' when deciding whether to refine a
conflict.

-----
diff --git a/etc/NEWS b/etc/NEWS
index bb214f2..86e89fd 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -428,8 +428,15 @@ values.
 and compares their entire trees.
 
 ** Diff mode
-*** Hunks are now automatically refined by default.
-To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
+*** Hunks are now automatically refined by font-lock.
+To disable refinement, set the new defcustom 'diff-refine' to nil.
+To get back the old behavior where hunks are refined as you navigate
+through a diff, set 'diff-refine' to the symbol 'navigate'.
+*** 'diff-auto-refine-mode' is deprecated in favor of 'diff-refine'.
+It is no longer active by default.
+*** 'diff-hunk-next' and 'diff-hunk-prev'
+These commands no longer recenter the window or change the narrowing
+of the buffer.
 
 +++
 *** Better syntax highlighting of Diff hunks.
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 5d6cc6f..b5bed98 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -94,10 +94,18 @@ diff-mode-hook
   :type 'hook
   :options '(diff-delete-empty-files diff-make-unified))
 
-(defcustom diff-font-lock-refine t
-  "If non-nil, font-lock highlighting includes hunk refinement."
+(defcustom diff-refine 'font-lock
+  "If non-nil, enable hunk refinement.
+
+The value `font-lock' means to refine during font-lock.
+The value `navigation' means to refine each hunk as you visit it
+with `diff-hunk-next' or `diff-hunk-prev'.
+
+You can always manually refine a hunk with `diff-refine-hunk'."
   :version "27.1"
-  :type 'boolean)
+  :type '(choice (const :tag "Don't refine hunks" nil)
+                 (const :tag "Refine hunks during font-lock" font-lock)
+                 (const :tag "Refine hunks during navigation" navigation)))
 
 (defcustom diff-font-lock-prettify nil
   "If non-nil, font-lock will try and make the format prettier."
@@ -262,9 +270,15 @@ diff-auto-refine-mode
 changes in detail as the user visits hunks.  When transitioning
 from disabled to enabled, it tries to refine the current hunk, as
 well."
-  :group 'diff-mode :init-value t :lighter nil ;; " Auto-Refine"
-  (when diff-auto-refine-mode
-    (condition-case-unless-debug nil (diff-refine-hunk) (error nil))))
+  :group 'diff-mode :init-value nil :lighter nil ;; " Auto-Refine"
+  (if diff-auto-refine-mode
+      (progn
+        (customize-set-variable 'diff-refine 'navigation)
+        (condition-case-unless-debug nil (diff-refine-hunk) (error nil)))
+    (customize-set-variable 'diff-refine nil)))
+(make-obsolete 'diff-auto-refine-mode "use `diff-refine' instead." "27.1")
+(make-obsolete-variable 'diff-auto-refine-mode
+                        "use `diff-refine' instead." "27.1")
 
 ;;;;
 ;;;; font-lock support
@@ -619,26 +633,60 @@ diff-end-of-file
 
 (defvar diff--auto-refine-data nil)
 
-;; Define diff-{hunk,file}-{prev,next}
-(easy-mmode-define-navigation
- diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
- (when diff-auto-refine-mode
-   (unless (prog1 diff--auto-refine-data
-             (setq diff--auto-refine-data
-                   (cons (current-buffer) (point-marker))))
-     (run-at-time 0.0 nil
-                  (lambda ()
-                    (when diff--auto-refine-data
-                      (let ((buffer (car diff--auto-refine-data))
-                            (point (cdr diff--auto-refine-data)))
-                        (setq diff--auto-refine-data nil)
-                        (with-local-quit
-                          (when (buffer-live-p buffer)
-                            (with-current-buffer buffer
-                              (save-excursion
-                                (goto-char point)
-                                (diff-refine-hunk))))))))))))
-
+(defun diff-navigate-maybe-refine ()
+  "If `diff-refine' is set to `navigation', refine current hunk."
+  (when (eq diff-refine 'navigation)
+    (unless (prog1 diff--auto-refine-data
+              (setq diff--auto-refine-data
+                    (cons (current-buffer) (point-marker))))
+      (run-at-time 0.0 nil
+                   (lambda ()
+                     (when diff--auto-refine-data
+                       (let ((buffer (car diff--auto-refine-data))
+                             (point (cdr diff--auto-refine-data)))
+                         (setq diff--auto-refine-data nil)
+                         (with-local-quit
+                           (when (buffer-live-p buffer)
+                             (with-current-buffer buffer
+                               (save-excursion
+                                 (goto-char point)
+                                 (diff-refine-hunk))))))))))))
+
+(defun diff-hunk-next (&optional count interactive)
+  "Go to the next COUNT'th hunk.
+
+Interactively, COUNT is the prefix numeric argument, and defaults to 1.
+
+If INTERACTIVE is non-nil and `diff-refine' is set to
+`navigation', refine the hunk moved to."
+  (interactive "p\np")
+  (unless count (setq count 1))
+  (if (< count 0)
+      (diff-hunk-prev (- count))
+    (if (looking-at diff-hunk-header-re) (setq count (1+ count)))
+    (if (not (re-search-forward diff-hunk-header-re nil t count))
+        (if (looking-at diff-hunk-header-re)
+            (goto-char (diff-end-of-hunk))
+          (user-error "No next hunk"))
+      (goto-char (match-beginning 0))
+      (if interactive (diff-navigate-maybe-refine)))))
+
+(defun diff-hunk-prev (&optional count interactive)
+  "Go to the previous COUNT'th hunk.
+
+Interactively, COUNT is the prefix numeric argument, and defaults to 1.
+
+If INTERACTIVE is non-nil and `diff-refine' is set to
+`navigation', refine the hunk moved to."
+  (interactive "p\np")
+  (unless count (setq count 1))
+  (if (< count 0)
+      (diff-hunk-next (- count))
+    (unless (re-search-backward diff-hunk-header-re nil t count)
+      (user-error "No previous hunk"))
+    (if interactive (diff-navigate-maybe-refine))))
+
+;; Define diff-file-{prev,next}
 (easy-mmode-define-navigation
  diff-file diff-file-header-re "file" diff-end-of-file)
 
@@ -2125,7 +2173,7 @@ diff--refine-hunk
 
 (defun diff--font-lock-refined (max)
   "Apply hunk refinement from font-lock."
-  (when diff-font-lock-refine
+  (when (eq diff-refine 'font-lock)
     (when (get-char-property (point) 'diff--font-lock-refined)
       ;; Refinement works over a complete hunk, whereas font-lock limits itself
       ;; to highlighting smallish chunks between point..max, so we may be
diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index 569797e..7c1cd4f 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -44,7 +44,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
-(require 'diff-mode)                    ;For diff-auto-refine-mode.
+(require 'diff-mode)                    ;For diff-refine.
 (require 'newcomment)
 
 ;;; The real definition comes later.
@@ -264,7 +264,7 @@ font-lock-keywords
 
 ;; Define smerge-next and smerge-prev
 (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil
-  (if diff-auto-refine-mode
+  (if diff-refine
       (condition-case nil (smerge-refine) (error nil))))
 
 (defconst smerge-match-names ["conflict" "upper" "base" "lower"])



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Juri Linkov-2
In reply to this post by Stefan Monnier
>>  ;; Define smerge-next and smerge-prev
>>  (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil
>> -  (if diff-auto-refine-mode
>> +  (if (eq diff-refine 'navigation)
>>        (condition-case nil (smerge-refine) (error nil))))
>
> Hmm... I want to set my `diff-refined` to `font-lock` yet I also want my
> smerge conflicts to be refined when I navigate to them.
> Maybe the test here should be just whether diff-refine is non-nil?

A similar option `font-lock` also makes sense for a new
customizable variable with a name like `smerge-refine`
that could automatically refine all smerge conflicts.

The problem is that it's difficult to remember and type such key sequences
`C-c ^ n` (smerge-next) and `C-c ^ R` (smerge-refine)  It would be easier
if smerge-mode refined all them automatically.



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
> A similar option `font-lock` also makes sense for a new
> customizable variable with a name like `smerge-refine`
> that could automatically refine all smerge conflicts.

FWIW, here's the reason why I haven't even looked into doing it
automatically for smerge: IMO 2-way conflicts are worthless so I only
care about 3-way conflicts, and for those I don't know how to display
all 3 different "refinements" at the same time.  IOW I too often need to
use the cycling behavior of smerge-refine for a "font-lock" version to
be sufficient.  So if we implement a "font-lock" version of
smerge-refine, we'll need to make sure it interacts well with subsequent
manual smerge-refine cycling.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Fri, 01 Feb 2019 02:38:51 -0500
>
> > A similar option `font-lock` also makes sense for a new
> > customizable variable with a name like `smerge-refine`
> > that could automatically refine all smerge conflicts.
>
> FWIW, here's the reason why I haven't even looked into doing it
> automatically for smerge: IMO 2-way conflicts are worthless so I only
> care about 3-way conflicts, and for those I don't know how to display
> all 3 different "refinements" at the same time.  IOW I too often need to
> use the cycling behavior of smerge-refine for a "font-lock" version to
> be sufficient.  So if we implement a "font-lock" version of
> smerge-refine, we'll need to make sure it interacts well with subsequent
> manual smerge-refine cycling.

Why are 2-way conflicts worthless?  Automatically refining them could
be helpful in some situations.

By the way, does the updated change I sent in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32991#38 look okay to
you?



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
> Why are 2-way conflicts worthless?

Because they give less info than 3-way conflicts and they're generated
from 3 files so you can "always" get the superior 3-way conflicts
instead of the inferior 2-way ones.

> By the way, does the updated change I sent in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32991#38 look okay
> to you?

Haven't had a chance to look at it yet,


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Sun, 03 Feb 2019 07:37:36 -0500
>
> > Why are 2-way conflicts worthless?
>
> Because they give less info than 3-way conflicts and they're generated
> from 3 files so you can "always" get the superior 3-way conflicts
> instead of the inferior 2-way ones.

Oh, like the ones you can get by setting Git's "merge.conflictStyle"
to "diff3".  Makes sense.

> > By the way, does the updated change I sent in
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32991#38 look okay
> > to you?
>
> Haven't had a chance to look at it yet,
>
>
>         Stefan

Okay, no rush.



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
In reply to this post by Charles A. Roelli
>> This change seems unrelated.  I'd rather we stick to the refinement itself.
> Without this change, other functions in diff-mode (such as
> diff--font-lock-syntax) calling diff-hunk-next accidentally trigger
> hunk refinement if 'diff-refine' is 'navigation'.

Ah, right, makes sense.  Could we fix this more directly by using
`called-interactively` instead?

> Incidentally, I left out the auto-recentering and buffer
> restriction-changing parts of the old diff-hunk-next and
> diff-hunk-prev, since these behaviors do not match other navigation
> commands.

Indeed, I see several changes in there, which is why I'd rather we
separate them into another patch.

[ FWIW I never used the restriction (I coded it up only to mach the
  featureset of some earlier diff-mode I'd found somewhere), but I would
  miss the recentering.  More to the point, rather than removing the
  recentering, I'd like to improve it (so it tries harder to make the
  whole hunk visible when possible).  ]

Other than that, LGTM.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
In reply to this post by Charles A. Roelli
> Oh, like the ones you can get by setting Git's "merge.conflictStyle"
> to "diff3".  Makes sense.

Is this setting still necessary?  The mind boggles!


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
In reply to this post by Stefan Monnier
> From: Stefan Monnier <[hidden email]>
> Date: Mon, 11 Feb 2019 15:14:12 -0500
>
> >> This change seems unrelated.  I'd rather we stick to the refinement itself.
> > Without this change, other functions in diff-mode (such as
> > diff--font-lock-syntax) calling diff-hunk-next accidentally trigger
> > hunk refinement if 'diff-refine' is 'navigation'.
>
> Ah, right, makes sense.  Could we fix this more directly by using
> `called-interactively` instead?

I think so, though I avoided that function because of the warnings in
its documentation.  Future callers of diff-hunk-prev/diff-hunk-next
also have more flexibility if they can choose whether the call should
be considered interactive or not.

> Indeed, I see several changes in there, which is why I'd rather we
> separate them into another patch.
>
> [ FWIW I never used the restriction (I coded it up only to mach the
>   featureset of some earlier diff-mode I'd found somewhere), but I would
>   miss the recentering.  More to the point, rather than removing the
>   recentering, I'd like to improve it (so it tries harder to make the
>   whole hunk visible when possible).  ]

For a bit of background: I sometimes enable a minor mode in the
ChangeLog buffer which narrows *vc-diff* (if displayed) to the changes
of the file at point, like an rmail summary buffer with its
corresponding message buffer.  I'd like to eventually add this feature
to add-log.el.  The current diff-hunk-prev/diff-hunk-next change the
narrowing of the buffer and thus cause some friction with this minor
mode.

That said, I could still add this new feature if I could disable the
re-narrowing by, say, setting a buffer-local variable.  I will update
the patch to bring back the re-narrowing by default, and then add a
defcustom for it in a second patch (ditto for the re-centering).

> Other than that, LGTM.

Thanks for the review.



12