bug#46268: 27.1.91; Error in occur-rename-buffer

bug#46268: 27.1.91; Error in occur-rename-buffer

[Juri Linkov]

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

Tags: patch
X-Debbugs-Cc: Stefan Monnier <monnier@iro.umontreal.ca>

After evaluating (add-hook 'occur-hook 'occur-rename-buffer)
then selecting a region, and running 'M-x occur' with any string
in the active region, signals the error:

  (wrong-type-argument bufferp #<overlay from 1 to 72 in *scratch*>)

It seems the attached patch is the right way to fix this.

Also Cc:ing Stefan with a question about occur--garbage-collect-revert-args:
trying to revert the Occur buffer created from an active region
removes the source buffer name from the Occur buffer name.
For example, (after applying the attached patch) 'M-x occur'
on the active region in *scratch* creates the buffer *Occur: *scratch**.

But reverting the Occur buffer with 'g' removes the *scratch* buffer name,
and renames the Occur buffer to *Occur: *.

This is because of two lines in occur-1:

          (occur--garbage-collect-revert-args)
	  (setq occur-revert-arguments (list regexp nlines bufs))

where occur--garbage-collect-revert-args deletes the overlay
with the buffer name *scratch*, and later occur-rename-buffer
can't get the original buffer name from occur-revert-arguments.
And I don't know how to fix this.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: occur-rename-buffer-overlay.patch --]
[-- Type: text/x-diff, Size: 1167 bytes --]

diff --git a/lisp/replace.el b/lisp/replace.el
index f13d27aff8..c4c80118b4 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1544,11 +1544,17 @@ occur-rename-buffer
   (interactive "P\np")
   (with-current-buffer
       (if (eq major-mode 'occur-mode) (current-buffer) (get-buffer "*Occur*"))
-    (rename-buffer (concat "*Occur: "
-                           (mapconcat #'buffer-name
-                                      (car (cddr occur-revert-arguments)) "/")
-                           "*")
-                   (or unique-p (not interactive-p)))))
+    (rename-buffer
+     (concat "*Occur: "
+             (mapconcat (lambda (boo)
+                          (or (and (buffer-live-p boo)
+                                   (buffer-name boo))
+                              (and (overlayp boo)
+                                   (buffer-live-p (overlay-buffer boo))
+                                   (buffer-name (overlay-buffer boo)))))
+                        (car (cddr occur-revert-arguments)) "/")
+             "*")
+     (or unique-p (not interactive-p)))))
 
 ;; Region limits when `occur' applies on a region.
 (defvar occur--final-pos nil)

bug#46268: 27.1.91; Error in occur-rename-buffer

[Stefan Monnier]

> Also Cc:ing Stefan with a question about occur--garbage-collect-revert-args:

[ Side note: Not sure why, I'm not familiar with this code.  ]

> trying to revert the Occur buffer created from an active region
> removes the source buffer name from the Occur buffer name.
> For example, (after applying the attached patch) 'M-x occur'
> on the active region in *scratch* creates the buffer *Occur: *scratch**.
>
> But reverting the Occur buffer with 'g' removes the *scratch* buffer name,
> and renames the Occur buffer to *Occur: *.
>
> This is because of two lines in occur-1:
>
>           (occur--garbage-collect-revert-args)
> 	  (setq occur-revert-arguments (list regexp nlines bufs))
>
> where occur--garbage-collect-revert-args deletes the overlay
> with the buffer name *scratch*, and later occur-rename-buffer
> can't get the original buffer name from occur-revert-arguments.
> And I don't know how to fix this.

AFAICT, `occur--garbage-collect-revert-args` aims to throw away the
overlays when they're not needed, but we will need them again if we want
to `revert-buffer`, so we shouldn't call this function until we're sure
`revert-buffer` won't be called.  IOW, I think we should just remove
the above call.

[ I just took a look at the history of this code, and I see that it
  claims I'm the one who added that function and that call 2 years
  ago.  I say this is fake news  ;-)  ]


        Stefan "not sure it's a good idea to joke about the
                re-birth of nazism"

bug#46268: 27.1.91; Error in occur-rename-buffer

[Stefan Monnier]

> +             (mapconcat (lambda (boo)
> +                          (or (and (buffer-live-p boo)
> +                                   (buffer-name boo))

`buffer-name` used to be the function that provided the functionality of
`buffer-live-p`, so you can replace

    (and (buffer-live-p boo)
         (buffer-name boo))

with just

    (buffer-live-p boo)

By definition of a buffer is live iff it has a name.

> +                              (and (overlayp boo)
> +                                   (buffer-live-p (overlay-buffer boo))
> +                                   (buffer-name (overlay-buffer boo)))))

And here you can further simplify because `overlay-buffer` only returns
non-nil if the buffer is live.


        Stefan

bug#46268: 27.1.91; Error in occur-rename-buffer

[Stefan Monnier]

> AFAICT, `occur--garbage-collect-revert-args` aims to throw away the
> overlays when they're not needed, but we will need them again if we want
> to `revert-buffer`, so we shouldn't call this function until we're sure
> `revert-buffer` won't be called.  IOW, I think we should just remove
> the above call.

I now see what's going on: this call is supposed to throw away the
overlay used by the previous invocation of `occur`.  So when we revert,
we don't want to throw them away but when it's a fresh new invocation,
we do.

Maybe we should add a (unless (eq bufs (nth 2 occur-revert-arguments)))?


        Stefan

bug#46268: 27.1.91; Error in occur-rename-buffer

[Juri Linkov]

>> AFAICT, `occur--garbage-collect-revert-args` aims to throw away the
>> overlays when they're not needed, but we will need them again if we want
>> to `revert-buffer`, so we shouldn't call this function until we're sure
>> `revert-buffer` won't be called.  IOW, I think we should just remove
>> the above call.
>
> I now see what's going on: this call is supposed to throw away the
> overlay used by the previous invocation of `occur`.  So when we revert,
> we don't want to throw them away but when it's a fresh new invocation,
> we do.
>
> Maybe we should add a (unless (eq bufs (nth 2 occur-revert-arguments)))?

Yep, this fixed the problem:

diff --git a/lisp/replace.el b/lisp/replace.el
index f13d27aff8..babae7fed9 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1779,7 +1783,8 @@ occur-1
 			       42)
 			    (window-width))
 			 "" (occur-regexp-descr regexp))))
-          (occur--garbage-collect-revert-args)
+          (unless (eq bufs (nth 2 occur-revert-arguments))
+            (occur--garbage-collect-revert-args))
 	  (setq occur-revert-arguments (list regexp nlines bufs))
           (if (= count 0)
               (kill-buffer occur-buf)

bug#46268: 27.1.91; Error in occur-rename-buffer

[Juri Linkov]

>> +             (mapconcat (lambda (boo)
>> +                          (or (and (buffer-live-p boo)
>> +                                   (buffer-name boo))
>
> `buffer-name` used to be the function that provided the functionality of
> `buffer-live-p`, so you can replace
>
>     (and (buffer-live-p boo)
>          (buffer-name boo))
>
> with just
>
>     (buffer-live-p boo)
>
> By definition of a buffer is live iff it has a name.
>
>> +                              (and (overlayp boo)
>> +                                   (buffer-live-p (overlay-buffer boo))
>> +                                   (buffer-name (overlay-buffer boo)))))
>
> And here you can further simplify because `overlay-buffer` only returns
> non-nil if the buffer is live.

Maybe then this should be enough to handle all cases:

diff --git a/lisp/replace.el b/lisp/replace.el
index f13d27aff8..d320542d62 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1545,7 +1545,10 @@ occur-rename-buffer
   (with-current-buffer
       (if (eq major-mode 'occur-mode) (current-buffer) (get-buffer "*Occur*"))
     (rename-buffer (concat "*Occur: "
-                           (mapconcat #'buffer-name
+                           (mapconcat (lambda (boo)
+                                        (buffer-name (if (overlayp boo)
+                                                         (overlay-buffer boo)
+                                                       boo)))
                                       (car (cddr occur-revert-arguments)) "/")
                            "*")
                    (or unique-p (not interactive-p)))))

bug#46268: 27.1.91; Error in occur-rename-buffer

[Stefan Monnier]

> -                           (mapconcat #'buffer-name
> +                           (mapconcat (lambda (boo)
> +                                        (buffer-name (if (overlayp boo)
> +                                                         (overlay-buffer boo)
> +                                                       boo)))

LGTM,


        Stefan