Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

[Tino Calancha]

Stefan Monnier <address@hidden> writes:

>> +(defun occur--parse-occur-buffer()
>> +  "Retrieve a list of the form (BEG END ORIG-LINE BUFFER).
>> +BEG and END define the region.
>> +ORIG-LINE and BUFFER are the line and the buffer from which
>> +the user called `occur'."
>> +  (save-excursion
>> +    (goto-char (point-min))
>> +    (let ((buffer (get-text-property (point-at-bol) 'occur-title))
>> +          (beg-pos (get-text-property (point-at-bol) 'region-start))
>> +          (end-pos (get-text-property (point-at-bol) 'region-end))
>> +          (orig-line (get-text-property (point-at-bol) 'current-line))
>> +          beg-line end-line)
>> +      (list beg-pos end-pos orig-line buffer))))
>
> I'm curious here:
> - Why use (point-at-bol) since we just moved to (point-min) and hence we
>   know that (point-at-bol) == (point-min) == (point)?
In a previous patch I saved the info in one particular position other
that (point-min); finally I saved the info in the entire line as it is
done with the buffer.
> - why store this info in text-properties rather than in buffer-local 
> variables?
I realized the buffer was already saved there with property
`occur-title'; I didn't wanted to scatter around related info.
>> +    (let* ((region (occur--parse-occur-buffer))
>> +           (region-start (nth 0 region))
>> +           (region-end (nth 1 region))
>> +           (orig-line (nth 2 region))
>> +           (buffer (nth 3 region))
>> +           (regexp (car occur-revert-arguments)))
>
> Here you could have used pcase-let:
>
>     (pcase-let ((`(,region-start ,region-end ,orig-line ,buffer)
>                  (occur--parse-occur-buffer))
>                 (regexp (car occur-revert-arguments)))
Nicer. Thank you!

>> +      (with-current-buffer buffer
>> +        (when (wholenump orig-line)
>> +          (goto-char 1)
>
> I'd recommend `point-min` instead of 1 here.
OK.  I always remember the discussion at
https://lists.gnu.org/archive/html/emacs-devel/2009-08/msg00520.html
but I forgot which was the encouraged practice there: in these cases
I follow the 50% rule to reject 0% success ratio (I know, it brings
50% failure ratio: life is full of injustice).

>> +          (forward-line (1- orig-line)))
>> +        (save-excursion
>> +          (if region
>> +              (occur regexp nil (list (cons region-start region-end)))
>> +            (apply 'occur-1 (append occur-revert-arguments (list 
>> (buffer-name))))))))))
>
> `region` is the list which holds (among other things) `buffer`, so if we
> successfully entered (with-current-buffer buffer ...) I can't see how
> `region` could be nil.
Good catch!
It looks like my typo.  Probably I wanted here to check if we have
called `occur' before with a non-nil region, i.e., this check:
+          (if (or region-start region-end)

I can push following amend commit:
--8<-----------------------------cut here---------------start------------->8---
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1213,29 +1213,25 @@ occur--parse-occur-buffer
 the user called `occur'."
   (save-excursion
     (goto-char (point-min))
-    (let ((buffer (get-text-property (point-at-bol) 'occur-title))
-          (beg-pos (get-text-property (point-at-bol) 'region-start))
-          (end-pos (get-text-property (point-at-bol) 'region-end))
-          (orig-line (get-text-property (point-at-bol) 'current-line))
-          beg-line end-line)
+    (let ((buffer (get-text-property (point) 'occur-title))
+          (beg-pos (get-text-property (point) 'region-start))
+          (end-pos (get-text-property (point) 'region-end))
+          (orig-line (get-text-property (point) 'current-line)))
       (list beg-pos end-pos orig-line buffer))))
 
 (defun occur-revert-function (_ignore1 _ignore2)
   "Handle `revert-buffer' for Occur mode buffers."
   (if (cdr (nth 2 occur-revert-arguments)) ; multi-occur
       (apply 'occur-1 (append occur-revert-arguments (list (buffer-name))))
-    (let* ((region (occur--parse-occur-buffer))
-           (region-start (nth 0 region))
-           (region-end (nth 1 region))
-           (orig-line (nth 2 region))
-           (buffer (nth 3 region))
-           (regexp (car occur-revert-arguments)))
+    (pcase-let ((`(,region-start ,region-end ,orig-line ,buffer)
+                 (occur--parse-occur-buffer))
+                (regexp (car occur-revert-arguments)))
       (with-current-buffer buffer
         (when (wholenump orig-line)
-          (goto-char 1)
+          (goto-char (point-min))
           (forward-line (1- orig-line)))
         (save-excursion
-          (if region
+          (if (or region-start region-end)
               (occur regexp nil (list (cons region-start region-end)))
             (apply 'occur-1 (append occur-revert-arguments (list 
(buffer-name))))))))))
 

--8<-----------------------------cut here---------------end--------------->8---