GNU bug report logs - #32991
27.0.50; diff-auto-refine-mode a no-op

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32991 in the body.
You can then email your comments to 32991 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: bug-gnu-emacs <at> gnu.org
Cc: monnier <at> iro.umontreal.ca
Subject: 27.0.50; diff-auto-refine-mode a no-op
Date: Mon, 08 Oct 2018 20:30:42 +0200

> commit f8b1e40fb63b0a6bc6692cc0bc84e5f5e65c2644
> Author: Stefan Monnier <monnier <at> iro.umontreal.ca>
> 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))))))))))))

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
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.

> 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

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
Date: Tue, 09 Oct 2018 21:15:39 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> 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?

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
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).

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

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
Date: Wed, 10 Oct 2018 20:31:52 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> 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".

Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
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`).

> 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

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
Date: Sat, 13 Oct 2018 15:42:54 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> 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.

Message #26 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: 27.0.50; diff-auto-refine-mode a no-op
Date: Sat, 13 Oct 2018 14:51:00 -0400

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

Message #29 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 13 Jan 2019 15:36:48 +0100

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> 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).

Message #32 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: monnier <at> iro.umontreal.ca, 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 13 Jan 2019 21:03:29 +0100

> Date: Sun, 13 Jan 2019 15:36:48 +0100
> From: charles <at> aurox.ch (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"])

Message #35 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 13 Jan 2019 18:33:15 -0500

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

Message #38 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Tue, 15 Jan 2019 21:25:12 +0100

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> 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"])

Message #41 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: "Charles A. Roelli" <charles <at> aurox.ch>, 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Wed, 30 Jan 2019 23:04:22 +0200

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

Message #44 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Juri Linkov <juri <at> linkov.net>
Cc: "Charles A. Roelli" <charles <at> aurox.ch>, 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
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.


        Stefan

Message #47 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 32991 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 03 Feb 2019 12:42:24 +0100

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> 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?

Message #50 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
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.

> 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

Message #53 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 32991 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 03 Feb 2019 15:19:04 +0100

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> 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.

Message #56 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
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?

> 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

Message #59 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Mon, 11 Feb 2019 15:15:27 -0500

> 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

Message #62 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Mon, 18 Feb 2019 20:06:35 +0100

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> 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.

Message #65 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Mon, 18 Feb 2019 15:44:09 -0500

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

Yes, it's better to avoid it.  But in this case, it's just a temporary
situation in order to cut the patch into two chunks, so it's perfectly fine.

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

As I said, I don't care much about this restriction "feature" of
diff-hunk-prev/diff-hunk-next, so I think it's OK to just drop it.
If some users then complain, we can see how to re-add it.

> defcustom for it in a second patch (ditto for the re-centering).

But the recentering is more important, IMO.  And I don't quite see why
you'd need a defcustom for it (I can see why you might only do the
recentering when the function is called interactively, tho).


        Stefan

Message #68 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 24 Feb 2019 17:12:13 +0100

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Date: Mon, 18 Feb 2019 15:44:09 -0500
> 
> Yes, it's better to avoid it.  But in this case, it's just a temporary
> situation in order to cut the patch into two chunks, so it's perfectly fine.

Right, here is the first part.  I noticed another use of
diff-auto-refine-mode (in admin/gitmerge.el) and its info
documentation, so I've updated those too.


From 2f393b95ad8912dc1f7908365766b200fc59cfe9 Mon Sep 17 00:00:00 2001
From: "Charles A. Roelli" <charles <at> aurox.ch>
Date: Sun, 24 Feb 2019 16:13:13 +0100
Subject: [PATCH] Merge diff-font-lock-refine and diff-auto-refine-mode into
 diff-refine

This change was discussed in Bug#32991.

* admin/gitmerge.el (gitmerge-resolve): Bind 'diff-refine'
instead of 'diff-auto-refine-mode' to nil.
* doc/emacs/files.texi (Diff Mode): Explain 'diff-refine'
instead of 'diff-auto-refine-mode' in the documentation of
'diff-hunk-next' and 'diff-hunk-prev'.  Mention in the
documentation of 'diff-refine-hunk' that refining is already
done by default.
* etc/NEWS (Diff mode): Explain renamed 'diff-refine' variable
and mention deprecation and disabling of
'diff-auto-refine-mode'.
* 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, make it
obsolete and make it set 'diff-refine' appropriately to keep
backward compatibility.
(diff-hunk-next, diff-hunk-prev): Adapt to rename of
diff-auto-refine-mode and ensure that refining only happens
when calling these commands interactively.
(diff--font-lock-refined): Adapt to rename of
diff-font-lock-refine.
* 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.
---
 admin/gitmerge.el      |  2 +-
 doc/emacs/files.texi   | 28 +++++++++++++---------------
 etc/NEWS               | 11 +++++++++--
 lisp/vc/diff-mode.el   | 30 ++++++++++++++++++++++--------
 lisp/vc/smerge-mode.el |  4 ++--
 5 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/admin/gitmerge.el b/admin/gitmerge.el
index 4854862..edf4379 100644
--- a/admin/gitmerge.el
+++ b/admin/gitmerge.el
@@ -294,7 +294,7 @@ gitmerge-resolve
            ((derived-mode-p 'change-log-mode)
             ;; Fix up dates before resolving the conflicts.
             (goto-char (point-min))
-            (let ((diff-auto-refine-mode nil))
+            (let ((diff-refine nil))
               (while (re-search-forward smerge-begin-re nil t)
                 (smerge-match-conflict)
                 (smerge-ensure-match 3)
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index d12d1ed..a574282 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1461,26 +1461,19 @@ Diff Mode
 Move to the next hunk-start (@code{diff-hunk-next}).  With prefix
 argument @var{n}, move forward to the @var{n}th next hunk.
 
-@findex diff-auto-refine-mode
-@cindex mode, Diff Auto-Refine
-@cindex Diff Auto-Refine mode
-This command has a side effect: it @dfn{refines} the hunk you move to,
-highlighting its changes with better granularity.  To disable this
-feature, type @kbd{M-x diff-auto-refine-mode} to toggle off the minor
-mode Diff Auto-Refine mode.  To disable Diff Auto-Refine mode by
-default, add this to your init file (@pxref{Hooks}):
-
-@example
-(add-hook 'diff-mode-hook
-          (lambda () (diff-auto-refine-mode -1)))
-@end example
+@vindex diff-refine
+By default, Diff mode @dfn{refines} hunks as Emacs displays them,
+highlighting their changes with better granularity.  Alternatively, if
+you set @code{diff-refine} to the symbol @code{navigation}, Diff mode
+only refines the hunk you move to with this command or with
+@code{diff-hunk-prev}.
 
 @item M-p
 @findex diff-hunk-prev
 Move to the previous hunk-start (@code{diff-hunk-prev}).  With prefix
 argument @var{n}, move back to the @var{n}th previous hunk.  Like
-@kbd{M-n}, this has the side-effect of refining the hunk you move to,
-unless you disable Diff Auto-Refine mode.
+@kbd{M-n}, this command refines the hunk you move to if you set
+@code{diff-refine} to the symbol @code{navigation}.
 
 @item M-@}
 @findex diff-file-next
@@ -1518,6 +1511,11 @@ Diff Mode
 (@code{diff-refine-hunk}).  This allows you to see exactly which parts
 of each changed line were actually changed.
 
+@vindex diff-refine
+By default, Diff mode refines hunks as Emacs displays them, so you may
+find this command useful if you customize @code{diff-refine} to a
+non-default value.
+
 @item C-c C-c
 @findex diff-goto-source
 @vindex diff-jump-to-old-file
diff --git a/etc/NEWS b/etc/NEWS
index 253da49..653ea43 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -458,8 +458,15 @@ and compares their entire trees.
 to hg revert.
 
 ** 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 enabled by default and binding it no longer has any
+effect.
 
 +++
 *** Better syntax highlighting of Diff hunks.
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index bad5639..b5ef209 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 "set `diff-refine' instead." "27.1")
+(make-obsolete-variable 'diff-auto-refine-mode
+                        "set `diff-refine' instead." "27.1")
 
 ;;;;
 ;;;; font-lock support
@@ -622,7 +636,7 @@ diff--auto-refine-data
 ;; 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
+ (when (and (eq diff-refine 'navigation) (called-interactively-p 'interactive))
    (unless (prog1 diff--auto-refine-data
              (setq diff--auto-refine-data
                    (cons (current-buffer) (point-marker))))
@@ -2145,7 +2159,7 @@ diff--iterate-hunks
 
 (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 02cee44..6b1df66 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"])
-- 
2.9.4

> As I said, I don't care much about this restriction "feature" of
> diff-hunk-prev/diff-hunk-next, so I think it's OK to just drop it.
> If some users then complain, we can see how to re-add it.
> 
> > defcustom for it in a second patch (ditto for the re-centering).
> 
> But the recentering is more important, IMO.  And I don't quite see why
> you'd need a defcustom for it (I can see why you might only do the
> recentering when the function is called interactively, tho).

Okay, I'm fine with removing the re-narrowing too if there are no
objections.  I'd still like to add a defcustom for the recentering, to
allow navigation in Diff mode buffers that respects scroll-related
variables.  But I think I will open a different bug for these issues,
since this one is nearly fixed.

Message #71 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Wed, 27 Feb 2019 10:04:35 -0500

> Right, here is the first part.  I noticed another use of
> diff-auto-refine-mode (in admin/gitmerge.el) and its info
> documentation, so I've updated those too.

LGTM.

> Okay, I'm fine with removing the re-narrowing too if there are no
> objections.

No objection from me.

> I'd still like to add a defcustom for the recentering, to allow
> navigation in Diff mode buffers that respects scroll-related
> variables.  But I think I will open a different bug for these issues,
> since this one is nearly fixed.

Yes, let's please first treat it as a bug.  Maybe we won't be able to
fix it without disabling recentering here, in which case we'll add
a defcustom for it, but maybe we can make it recenter *and* obey your
scroll settings at the same time.


        Stefan

Message #74 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Sun, 03 Mar 2019 21:51:55 +0100

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Date: Wed, 27 Feb 2019 10:04:35 -0500
> 
> > Right, here is the first part.  I noticed another use of
> > diff-auto-refine-mode (in admin/gitmerge.el) and its info
> > documentation, so I've updated those too.
> 
> LGTM.

Thanks, I've pushed it to master.
 
> > I'd still like to add a defcustom for the recentering, to allow
> > navigation in Diff mode buffers that respects scroll-related
> > variables.  But I think I will open a different bug for these issues,
> > since this one is nearly fixed.
> 
> Yes, let's please first treat it as a bug.  Maybe we won't be able to
> fix it without disabling recentering here, in which case we'll add
> a defcustom for it, but maybe we can make it recenter *and* obey your
> scroll settings at the same time.

Ok, I've sent a bug for the recentering.

Message #81 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: Stefan Monnier <monnier <at> IRO.UMontreal.CA>, 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Tue, 05 Mar 2019 23:38:01 +0200

> Thanks, I've pushed it to master.

Do you think bug#32460 is also fixed by your change?

Message #84 received at 32991 <at> debbugs.gnu.org (full text, mbox):

From: charles <at> aurox.ch (Charles A. Roelli)
To: Juri Linkov <juri <at> linkov.net>
Cc: monnier <at> IRO.UMontreal.CA, 32991 <at> debbugs.gnu.org
Subject: Re: bug#32991: 27.0.50; diff-auto-refine-mode a no-op
Date: Thu, 07 Mar 2019 20:23:24 +0100

> From: Juri Linkov <juri <at> linkov.net>
> Date: Tue, 05 Mar 2019 23:38:01 +0200
> 
> Do you think bug#32460 is also fixed by your change?

I think so.  It looks like that bug was caused by an unfortunate
interaction of diff-font-lock-refine and diff-auto-refine-mode.  I'll
close it for now, and thank you for pointing this out.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.