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

[Charles A. Roelli]

> From: Stefan Monnier <monnier@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"])