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: 2248 bytes --]

On 20.05.2020 01:21, Juri Linkov wrote:
>>> Another variant would be
>>> to add an optional choice to the existing option that allows to restore
>>> the old behavior and doesn't affect the current behavior in any way.
>>
>> FWIW, I'd consider that more of a documentation change.
>>
>> More importantly, and certainly only for Emacs 28, Juri could you remind me
>> what we'll be losing by removing the case no. 2 from
>> next-error-find-buffer?
> 
> It is used to handle such cases when navigating one next-error list
> visits a buffer that contains another next-error list - visiting
> such buffer should not switch the navigation, it should continue
> the initial navigation.

Didn't we fix changelog-mode so this doesn't happen anymore? I think as 
long as file major modes don't set next-error-last-buffer (and minor 
modes either, on initialization), we should be safe.

Can you reproduce a problematic scenario with the attached patch?

> But I'm not proud of the case no. 2, so you can drop it :)
> 
> Or better move it to a separate function and provide this function
> as an option in next-error-find-buffer-function.

Patch attached. Comments welcome.

>> The ability to switch to an arbitrary Grep buffer and start using it with
>> 'M-x next-error'? E.g. if there are several of them. That's more of
>> a backward compatibility concern, right? Or do you have scenarios in
>> mind where this will really save on keystrokes?
> 
> The ability to visit an arbitrary Grep buffer and use 'next-error' in it
> to switch navigation should always work as the most reliable and implicit
> way to switch navigation.  This is the case no. 3 in next-error-find-buffer.

Not if there are, say, two Compilation buffers (or Grep and Occur, etc), 
and you switch to the least recently used one, and press M-g n.

It's probably fine, though.

>> On another note, perhaps we could add a message to next-error-select-buffer
>> that would be shown if we suspect this command will not have the expected
>> result for the user.
> 
> Or maybe ask the user using the minibuffer to resolve ambiguity.

Resolve it how? I think we can safely guess their intention, it's just 
not trivial to execute it.

Anyway, this problem should go away with the attached patch.

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

diff --git a/lisp/simple.el b/lisp/simple.el
index 111afa69d1..31d6cbcef2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -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-buffer-try-current-outside-navigation)
                  (function :tag "Other function"))
   :group 'next-error
   :version "27.1")
@@ -240,6 +242,21 @@ next-error-buffer-on-selected-frame
     (if (eq (length window-buffers) 1)
         (car window-buffers))))
 
+(defun next-error-buffer-try-current-outside-navigation (&optional
+                                                         avoid-current
+                                                         extra-test-inclusive
+                                                         extra-test-exclusive)
+  "Maybe Return current buffer if it's next-error capable.
+But return nil if we navigated to the current buffer by the means
+of `next-error' command."
+  ;; Check that 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.
+  (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 +277,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 +294,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 +302,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)