bug#81504: [PATCH] Restore current buffer after clone-indirect-buffer-hook errors

Donjuanplatinum [30/Jul  9:35pm +08] wrote:
> Sean Whitton <[email protected]> writes:
>
>> Donjuanplatinum via "Bug reports for GNU Emacs, the Swiss army knife of text editors" [28/Jul  1:47am +08] wrote:
>>> Version: 31.0.50
>>> Base-Commit: 810e9d675ba484dabade1a9f30201d2848c5e2ea
>>>
>>> In `make-indirect-buffer` with clone argument, if
>>> `clone-indirect-buffer-hook` return an error, `current-buffer` will not
>>> be restored.
>>>
>>> To make the error in emacs -Q:
>>>
>>>
>>>    (progn
>>>       (generate-new-buffer "base")
>>>       (with-current-buffer "base" (insert "text"))
>>>       (set-buffer "base")
>>>       (let ((clone-indirect-buffer-hook
>>>              (list (lambda () (error "hook error")))))
>>>         (condition-case nil
>>>             (make-indirect-buffer "base" "ind" t)
>>>           (error nil)))
>>>       (buffer-name (current-buffer)))
>>>
>>> Expected: "base"
>>> But get: "ind"
>>>
>>> The Clone path in Fmake_indirect_buffer use set_buffer_internal_1 to
>>> restore the current-buffer, but when clone-indirect-buffer-hook return
>>> an error, it will skip the set_buffer_internal_1.
>>>
>>> The attached patch replaces it with record_unwind_current_buffer so that
>>> it will following the standard specpdl pattern to restore the current-buffer.
>>
>> Should we be restoring the current buffer here?  I'm not sure there is
>> any particular reason why we should, and calling code can always use
>> with-current-buffer or save-current-buffer if it needs to ensure that
>> it's restored?
> Thank you for taking a look.
>
> I think the function already intends to restore the current buffer on
> the normal path. Otherwise, why does it save old_b and call
> set_buffer_internal_1 (old_b) before returning?
>
> My patch only makes the error path behave same with the normal path.

I see what you mean.

I can't install the full patch because I don't think you've assigned
copyright for contributions to Emacs to the FSF.  If you'd be interested
in doing that we can start that process?

In the meantime I've installed the C changes, because it does not seem
essential to have a test for this.  But after copyright assignment we
can install the test anyway.

-- 
Sean Whitton