bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov <dgutov@yandex.ru>]

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

On 25.05.2020 18:17, Eli Zaretskii wrote:

>>> Yes, this will fix the reported problem of customizability.
>>> Maybe Eli will agree to fix this in emacs-27.
>>
>> I can post the corresponding patch, if it helps.
> 
> It's okay to add a defcustom and a new function, but the other part of
> the patch changes the default behavior, and that is less okay.  Can we
> do one without the other?

The defcustom already exists. It currently defaults to #'ignore (do 
nothing). It is called in case #1.

The attached patch moves case #2 into a new function and makes it the 
default value of the said defcustom (thus case #2 effectively moves into 
case #1). As a result, the default behavior doesn't change, but the user 
will have a much easier time turning off case #2.

> (Btw, the textual descriptions of the options both in the patch and
> those already in the code are confusingly obscure, so much so that I
> don't think I could understand what each one does.)

Knowing the subject matter somewhat, I think the descriptions are 
meaningful enough, but to make sense of them one has to understand how 
the whole feature comes together. E.g. at what times 
next-error-find-buffer is called.

> All in all, I feel (for quite some time) that this area is
> over-engineered and keeps bumping into more and more unintended
> consequences.  Maybe it's time to take a step back and rethink the
> entire subject?  (But definitely not on the release branch.)

That's what we're doing here.

If the attached patch is accepted for emacs-27, then on master we'll 
change next-error-find-buffer-function's default back to #'ignore (thus 
reverting to the previous path that I sent). And see if we find problems 
with how the result is working.

So in the end, next-error-find-buffer-function will have three possible 
values:

- Pre Emacs 27 behavior.
- Emacs 27 behavior (extracted in the attached patch).
- Simplified behavior with less buffer-local state (Emacs 28, hopefully).

[-- Attachment #2: next-error-extract-case-2-keep-current-behavior.diff --]
[-- Type: text/x-patch, Size: 4333 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 111afa69d1..a4b81719ce 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -199,7 +199,7 @@ next-error-buffer-p
 	   (and extra-test-inclusive
 		(funcall extra-test-inclusive))))))
 
-(defcustom next-error-find-buffer-function #'ignore
+(defcustom next-error-find-buffer-function #'next-error-no-navigation-try-current
   "Function called to find a `next-error' capable buffer.
 This functions takes the same three arguments as the function
 `next-error-find-buffer', and should return the buffer to be
@@ -211,6 +211,8 @@ next-error-find-buffer-function
   :type '(choice (const :tag "No default" ignore)
                  (const :tag "Single next-error capable buffer on selected frame"
                         next-error-buffer-on-selected-frame)
+                 (const :tag "Current buffer if next-error capable and outside navigation"
+                        next-error-no-navigation-try-current)
                  (function :tag "Other function"))
   :group 'next-error
   :version "27.1")
@@ -240,6 +242,22 @@ next-error-buffer-on-selected-frame
     (if (eq (length window-buffers) 1)
         (car window-buffers))))
 
+(defun next-error-no-navigation-try-current (&optional
+                                             avoid-current
+                                             extra-test-inclusive
+                                             extra-test-exclusive)
+  "Try the current buffer when outside navigation.
+But return nil if we navigated to the current buffer by the means
+of `next-error' command.  Othewise, return it if it's next-error
+capable."
+  ;; Check that next-error-buffer has no buffer-local value
+  ;; (i.e. we never navigated to the current buffer from another),
+  ;; and the current buffer is a `next-error' capable buffer.
+  (if (and (not (local-variable-p 'next-error-buffer))
+           (next-error-buffer-p (current-buffer) avoid-current
+                                extra-test-inclusive extra-test-exclusive))
+      (current-buffer)))
+
 (defun next-error-find-buffer (&optional avoid-current
 					 extra-test-inclusive
 					 extra-test-exclusive)
@@ -260,24 +278,16 @@ next-error-find-buffer
    (funcall next-error-find-buffer-function avoid-current
                                             extra-test-inclusive
                                             extra-test-exclusive)
-   ;; 2. If next-error-buffer has no buffer-local value
-   ;; (i.e. never navigated to the current buffer from another),
-   ;; and the current buffer is a `next-error' capable buffer,
-   ;; use it unconditionally, so next-error will always use it.
-   (if (and (not (local-variable-p 'next-error-buffer))
-            (next-error-buffer-p (current-buffer) avoid-current
-			         extra-test-inclusive extra-test-exclusive))
-       (current-buffer))
-   ;; 3. If next-error-last-buffer is an acceptable buffer, use that.
+   ;; 2. If next-error-last-buffer is an acceptable buffer, use that.
    (if (and next-error-last-buffer
             (next-error-buffer-p next-error-last-buffer avoid-current
                                  extra-test-inclusive extra-test-exclusive))
        next-error-last-buffer)
-   ;; 4. If the current buffer is acceptable, choose it.
+   ;; 3. If the current buffer is acceptable, choose it.
    (if (next-error-buffer-p (current-buffer) avoid-current
 			    extra-test-inclusive extra-test-exclusive)
        (current-buffer))
-   ;; 5. Look for any acceptable buffer.
+   ;; 4. Look for any acceptable buffer.
    (let ((buffers (buffer-list)))
      (while (and buffers
                  (not (next-error-buffer-p
@@ -285,7 +295,7 @@ next-error-find-buffer
 		       extra-test-inclusive extra-test-exclusive)))
        (setq buffers (cdr buffers)))
      (car buffers))
-   ;; 6. Use the current buffer as a last resort if it qualifies,
+   ;; 5. Use the current buffer as a last resort if it qualifies,
    ;; even despite AVOID-CURRENT.
    (and avoid-current
 	(next-error-buffer-p (current-buffer) nil
@@ -293,7 +303,7 @@ next-error-find-buffer
 	(progn
 	  (message "This is the only buffer with error message locations")
 	  (current-buffer)))
-   ;; 7. Give up.
+   ;; 6. Give up.
    (error "No buffers contain error message locations")))
 
 (defun next-error (&optional arg reset)