Re: master 048e72a403e 1/2: Add file/buffer-to-register (Bug#73308)

Re: master 048e72a403e 1/2: Add file/buffer-to-register (Bug#73308)

[Eshel Yaron]

Eli Zaretskii <eliz@gnu.org> writes:

> branch: master
> commit 048e72a403e22dc394b265c6ff290b8d40e806a5
> Author: Barra Ó Catháin <barra@ocathain.ie>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     Add file/buffer-to-register (Bug#73308)

[...]

> +(defun buffer-to-register (buffer register)
> +  "Insert BUFFER into REGISTER.
> +To visit the buffer, use \\[jump-to-register].
> +
> +Interactively, prompt for REGISTER using `register-read-with-preview'.
> +With a prefix-argument, prompt for BUFFER-NAME using `read-buffer',
> +With no prefix-argument, use the current buffer for BUFFER."
> +  (interactive (list (if (eq current-prefix-arg nil)
> +                         (current-buffer)
> +                       (read-buffer "Buffer: "))
> +                     (register-read-with-preview "Buffer to register: ")))
> +  (add-hook 'kill-buffer-hook 'register-buffer-to-file-query nil t)
> +  (set-register register (cons 'buffer buffer)))

Hmm, this looks like a subtle bug: the BUFFER argument can be any
buffer, but the add-hook call is always local to the current buffer.
Shouldn't that add-hook happen inside a with-current-buffer or something
like that?


Best,

Eshel

Re: master 048e72a403e 1/2: Add file/buffer-to-register (Bug#73308)

[Barra Ó Catháin via Emacs development discussions.]

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

D'oh! You're absolutely correct. I've attached a patch containing a fix
for exactly this; It adds a check to see if buffer is currently alive,
and uses with-current-buffer to ensure that it's adding to the correct
one. Let me know if I should send this somewhere else; I'm still
learning mailing list development etiquette.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-buffer-to-register-add-hook.patch --]
[-- Type: text/patch, Size: 1215 bytes --]

From 91cee4c5a72fb0fba5513d2fcab0d2596bfba684 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Barra=20=C3=93=20Cath=C3=A1in?= <barra@ocathain.ie>
Date: Mon, 7 Oct 2024 00:16:23 +0100
Subject: [PATCH] Fix buffer-to-register add-hook.

* lisp/register.el (buffer-to-register): Add with-current-buffer
  so add-hook correctly refers to buffer when given argument.
---
 lisp/register.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/register.el b/lisp/register.el
index 1b4281ae4ec..d9dbc3ab186 100644
--- a/lisp/register.el
+++ b/lisp/register.el
@@ -728,8 +728,10 @@ buffer-to-register
                          (current-buffer)
                        (read-buffer "Buffer: "))
                      (register-read-with-preview "Buffer to register: ")))
-  (add-hook 'kill-buffer-hook 'register-buffer-to-file-query nil t)
-  (set-register register (cons 'buffer buffer)))
+  (let ((buffer (or (get-buffer buffer) buffer)))
+    (with-current-buffer buffer
+      (add-hook 'kill-buffer-hook 'register-buffer-to-file-query nil t))
+    (set-register register (cons 'buffer buffer))))
 
 (cl-defgeneric register-val-jump-to (_val _arg)
   "Execute the \"jump\" operation of VAL.
-- 
2.46.0


[-- Attachment #3: Type: text/plain, Size: 1342 bytes --]


Thanks for the catch!
-----
Barra

Eshel Yaron <me@eshelyaron.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> branch: master
>> commit 048e72a403e22dc394b265c6ff290b8d40e806a5
>> Author: Barra Ó Catháin <barra@ocathain.ie>
>> Commit: Eli Zaretskii <eliz@gnu.org>
>>
>>     Add file/buffer-to-register (Bug#73308)
>
> [...]
>
>> +(defun buffer-to-register (buffer register)
>> +  "Insert BUFFER into REGISTER.
>> +To visit the buffer, use \\[jump-to-register].
>> +
>> +Interactively, prompt for REGISTER using `register-read-with-preview'.
>> +With a prefix-argument, prompt for BUFFER-NAME using `read-buffer',
>> +With no prefix-argument, use the current buffer for BUFFER."
>> +  (interactive (list (if (eq current-prefix-arg nil)
>> +                         (current-buffer)
>> +                       (read-buffer "Buffer: "))
>> +                     (register-read-with-preview "Buffer to register: ")))
>> +  (add-hook 'kill-buffer-hook 'register-buffer-to-file-query nil t)
>> +  (set-register register (cons 'buffer buffer)))
>
> Hmm, this looks like a subtle bug: the BUFFER argument can be any
> buffer, but the add-hook call is always local to the current buffer.
> Shouldn't that add-hook happen inside a with-current-buffer or something
> like that?
>
>
> Best,
>
> Eshel

Re: master 048e72a403e 1/2: Add file/buffer-to-register (Bug#73308)

[Eshel Yaron]

Hi,

Barra Ó Catháin <barra@ocathain.ie> writes:

> D'oh! You're absolutely correct. I've attached a patch containing a fix
> for exactly this; adds a check to see if buffer is currently alive,
> and uses with-current-buffer to ensure that it's adding to the correct
> one.

Thanks!  I've applied your patch (with a minor tweak to the commit
message) in commit c0f10fc3d48.  I then followed up with a couple of
small refinements in commit 6a272dedad3It.  I mostly improved the
docstring and the minibuffer prompts, but note that I removed the
(let ((buffer (or (get-buffer buffer) buffer))) ...) binding: it only
has any effect if BUFFER is a string, which it isn't, it's a buffer.
Let me know if I misread something.

> Let me know if I should send this somewhere else; I'm still
> learning mailing list development etiquette.

This is fine for such a follow up, I think.


Best,

Eshel

Re: master 048e72a403e 1/2: Add file/buffer-to-register (Bug#73308)

[Barra Ó Catháin via Emacs development discussions.]

Looks nice to me; the binding I was trying to do was mostly because I
didn't actually require a match in read-buffer, and read-buffer returns
a string. It looks silly because it was silly; as you have done here,
should have just gotten a buffer in the first place! (the only
difference in functionality would be the ability to specify a buffer
that isn't currently open. Being that this is mostly going to be used
interactively... just open a buffer :P)

I'm personally happy enough with how it behaves now. Thanks for your
help!

Thanks,
-----
Barra

Eshel Yaron <me@eshelyaron.com> writes:

> Hi,
>
> Barra Ó Catháin <barra@ocathain.ie> writes:
>
>> D'oh! You're absolutely correct. I've attached a patch containing a fix
>> for exactly this; adds a check to see if buffer is currently alive,
>> and uses with-current-buffer to ensure that it's adding to the correct
>> one.
>
> Thanks!  I've applied your patch (with a minor tweak to the commit
> message) in commit c0f10fc3d48.  I then followed up with a couple of
> small refinements in commit 6a272dedad3It.  I mostly improved the
> docstring and the minibuffer prompts, but note that I removed the
> (let ((buffer (or (get-buffer buffer) buffer))) ...) binding: it only
> has any effect if BUFFER is a string, which it isn't, it's a buffer.
> Let me know if I misread something.
>
>> Let me know if I should send this somewhere else; I'm still
>> learning mailing list development etiquette.
>
> This is fine for such a follow up, I think.
>
>
> Best,
>
> Eshel