Hi Ihor,

On 11/5/22 03:09, Ihor Radchenko wrote:
> Adam Porter <adam@alphapapa.net> writes:
> 
>> The attached patch improves the function org-get-indirect-buffer, fixing
>> a bug, clarifying the code, and adding a docstring.
> 
> Thanks! I have some comments.

Thanks for your review.  I've attached a new patch.

>> +(cl-defun org-get-indirect-buffer (&optional (buffer (current-buffer)) heading)
>> +  "Return an indirect buffer based on BUFFER.
>> +If HEADING, prepend it to the name of the new buffer."
> 
> Maybe append to the name?

Yes, thanks.  In the new patch I took the liberty of improving the 
format to make it more distinctive (i.e. it won't resemble a normal 
filename).
> 
>> +  (let* ((base-buffer (or (buffer-base-buffer buffer) buffer))
>> +         (suffix-prefix (if heading
>> +                            (concat heading "-")
>> +                          ""))
> 
> Why not pre-define the whole prefix instead?
> (prefix (format "%s-%s" (buffer-name base-buffer)
>                          (if heading (concat heading "-") "")))
> 
> then, can just say (format "%s%s" prefix n) in the loop.
> 
>> +         (buffer-name (cl-loop for n from 1 to 100
> 
> why to 100? It may fail (even though unlikely) and also unnecessary.
> Can just say for n from 1.

I chose to limit the index because IME it's better to signal an error 
than to potentially go into an infinite loop.  Although it shouldn't 
happen, it could in the case of unforeseen circumstances, one of which I 
experienced while writing the patch.

But, even better, the new patch uses the built-in function 
generate-new-buffer-name, which handles it for us, and more efficiently.

>> +      ;; FIXME: Explain why this `condition-case' is necessary.  Why
>> +      ;; could an error be signaled with the CLONE argument non-nil,
>> +      ;; and why would trying again without CLONE solve the problem?
>> +      (error (make-indirect-buffer base-buffer buffer-name)))))
> 
> I did not find why in the git logs. It looks like some ancient code. You
> can remove it in a followup patch.

Very well, the new patch omits it.

Thanks,
Adam