GNU bug report logs - #48925
[PATCH] Set `minibuffer-completion-*` variables buffer-locally in a few more places

Previous Next

To reply to this bug, email your comments to 48925 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: miha <at> kamnitnik.top
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Set `minibuffer-completion-*` variables buffer-locally in a
 few more places
Date: Tue, 08 Jun 2021 20:30:29 +0200

[Message part 1 (text/plain, inline)]

This follows up on changes proposed in bug#45474. The second patch is a
bit more controversial, but is probably required if we want more
reliable usage of completion commands in non-innermost minibuffers (that
is, with minibuffer-follows-selected-frame set to nil.)

[0001-Set-minibuffer-completion-variables-locally-in-more-.patch (text/x-patch, inline)]

From 049d57e6d10edca1d6a0af119f557e364d8ea93f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha <at> kamnitnik.top>
Date: Tue, 8 Jun 2021 20:17:59 +0200
Subject: [PATCH 1/2] Set `minibuffer-completion-*` variables locally in more
 places

Follow-up to commit
2021-05-01 "* lisp/minibuffer.el (completing-read-default): Fix bug#45474"

* lisp/calc/calc-store.el (calc-read-var-name):
* lisp/emacs-lisp/crm.el (completing-read-multiple):
* lisp/progmodes/cc-styles.el (c-read-offset):
* lisp/window.el (read-buffer-to-switch):
Set `minibuffer-completion-*` variables buffer-locally instead of
using a global let-binding.
---
 lisp/calc/calc-store.el     | 15 +++++++-----
 lisp/emacs-lisp/crm.el      | 47 ++++++++++++++++++-------------------
 lisp/progmodes/cc-styles.el | 12 ++++++----
 lisp/window.el              |  2 +-
 4 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/lisp/calc/calc-store.el b/lisp/calc/calc-store.el
index ee29c440fe..d96b40156d 100644
--- a/lisp/calc/calc-store.el
+++ b/lisp/calc/calc-store.el
@@ -188,12 +188,15 @@ calc-read-var-name
   (let* ((calc-store-opers store-opers)
          (var (concat
               "var-"
-              (let ((minibuffer-completion-table
-                     (mapcar (lambda (x) (substring x 4))
-                             (all-completions "var-" obarray)))
-                    (minibuffer-completion-predicate
-                     (lambda (x) (boundp (intern (concat "var-" x)))))
-                    (minibuffer-completion-confirm t))
+              (minibuffer-with-setup-hook
+                  (lambda ()
+                    (setq-local minibuffer-completion-table
+                                (mapcar (lambda (x) (substring x 4))
+                                        (all-completions "var-" obarray)))
+                    (setq-local minibuffer-completion-predicate
+                                (lambda (x)
+                                  (boundp (intern (concat "var-" x)))))
+                    (setq-local minibuffer-completion-confirm t))
                 (read-from-minibuffer
                  prompt nil calc-var-name-map nil
                  'calc-read-var-name-history)))))
diff --git a/lisp/emacs-lisp/crm.el b/lisp/emacs-lisp/crm.el
index e106815817..67464bc6db 100644
--- a/lisp/emacs-lisp/crm.el
+++ b/lisp/emacs-lisp/crm.el
@@ -245,30 +245,29 @@ completing-read-multiple
 
 This function returns a list of the strings that were read,
 with empty strings removed."
-  (unwind-protect
-      (progn
-	(add-hook 'choose-completion-string-functions
-		  'crm--choose-completion-string)
-	(let* ((minibuffer-completion-table #'crm--collection-fn)
-	       (minibuffer-completion-predicate predicate)
-	       ;; see completing_read in src/minibuf.c
-	       (minibuffer-completion-confirm
-		(unless (eq require-match t) require-match))
-	       (crm-completion-table table)
-	       (map (if require-match
-			crm-local-must-match-map
-		      crm-local-completion-map))
-	       ;; If the user enters empty input, `read-from-minibuffer'
-	       ;; returns the empty string, not DEF.
-	       (input (read-from-minibuffer
-		       prompt initial-input map
-		       nil hist def inherit-input-method)))
-	  (when (and def (string-equal input ""))
-	    (setq input (if (consp def) (car def) def)))
-          ;; Remove empty strings in the list of read strings.
-	  (split-string input crm-separator t)))
-    (remove-hook 'choose-completion-string-functions
-		 'crm--choose-completion-string)))
+  (let* ((map (if require-match
+                  crm-local-must-match-map
+                crm-local-completion-map))
+         input)
+    (minibuffer-with-setup-hook
+        (lambda ()
+          (add-hook 'choose-completion-string-functions
+                    'crm--choose-completion-string nil 'local)
+          (setq-local minibuffer-completion-table #'crm--collection-fn)
+          (setq-local minibuffer-completion-predicate predicate)
+          ;; see completing_read in src/minibuf.c
+          (setq-local minibuffer-completion-confirm
+                      (unless (eq require-match t) require-match))
+          (setq-local crm-completion-table table))
+      (setq input (read-from-minibuffer
+                   prompt initial-input map
+                   nil hist def inherit-input-method)))
+    ;; If the user enters empty input, `read-from-minibuffer'
+    ;; returns the empty string, not DEF.
+    (when (and def (string-equal input ""))
+      (setq input (if (consp def) (car def) def)))
+    ;; Remove empty strings in the list of read strings.
+    (split-string input crm-separator t)))
 
 ;; testing and debugging
 ;; (defun crm-init-test-environ ()
diff --git a/lisp/progmodes/cc-styles.el b/lisp/progmodes/cc-styles.el
index 8514434e9a..873682043c 100644
--- a/lisp/progmodes/cc-styles.el
+++ b/lisp/progmodes/cc-styles.el
@@ -444,17 +444,19 @@ c-read-offset
 			  defstr))
 	 (prompt (concat symname " offset " defstr))
 	 (keymap (make-sparse-keymap))
-	 (minibuffer-completion-table obarray)
-	 (minibuffer-completion-predicate 'fboundp)
 	 offset input)
     ;; In principle completing-read is used here, but SPC is unbound
     ;; to make it less annoying to enter lists.
     (set-keymap-parent keymap minibuffer-local-completion-map)
     (define-key keymap " " 'self-insert-command)
     (while (not offset)
-      (setq input (read-from-minibuffer prompt nil keymap t
-					'c-read-offset-history
-					(format "%s" oldoff)))
+      (minibuffer-with-setup-hook
+          (lambda ()
+            (setq-local minibuffer-completion-table obarray)
+            (setq-local minibuffer-completion-predicate 'fboundp))
+        (setq input (read-from-minibuffer prompt nil keymap t
+                                          'c-read-offset-history
+                                          (format "%s" oldoff))))
       (if (c-valid-offset input)
 	  (setq offset input)
 	;; error, but don't signal one, keep trying
diff --git a/lisp/window.el b/lisp/window.el
index fd1c617d6b..029202e350 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8376,7 +8376,7 @@ read-buffer-to-switch
   (let ((rbts-completion-table (internal-complete-buffer-except)))
     (minibuffer-with-setup-hook
         (lambda ()
-          (setq minibuffer-completion-table rbts-completion-table)
+          (setq-local minibuffer-completion-table rbts-completion-table)
           ;; Since rbts-completion-table is built dynamically, we
           ;; can't just add it to the default value of
           ;; icomplete-with-completion-tables, so we add it
-- 
2.31.1

[0002-Don-t-bind-minibuffer-completion-table-to-nil-in-rea.patch (text/x-patch, inline)]

From 466169b9f679a78aec00f9735335d90718c0d898 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha <at> kamnitnik.top>
Date: Tue, 8 Jun 2021 20:19:44 +0200
Subject: [PATCH 2/2] Don't bind minibuffer-completion-table to nil in
 read-string

This reverts
2012-06-19 "* src/minibuf.c (Fread_string): Bind minibuffer-completion-table."

* src/minibuf.c (Fread_string): Don't bind minibuffer-completion-table
to nil.
---
 src/minibuf.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/minibuf.c b/src/minibuf.c
index 00069eabbe..adee471887 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1376,20 +1376,13 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
   (Lisp_Object prompt, Lisp_Object initial_input, Lisp_Object history, Lisp_Object default_value, Lisp_Object inherit_input_method)
 {
   Lisp_Object val;
-  ptrdiff_t count = SPECPDL_INDEX ();
-
-  /* Just in case we're in a recursive minibuffer, make it clear that the
-     previous minibuffer's completion table does not apply to the new
-     minibuffer.
-     FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
-  specbind (Qminibuffer_completion_table, Qnil);
 
   val = Fread_from_minibuffer (prompt, initial_input, Qnil,
 			       Qnil, history, default_value,
 			       inherit_input_method);
   if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
     val = CONSP (default_value) ? XCAR (default_value) : default_value;
-  return unbind_to (count, val);
+  return val;
 }
 
 DEFUN ("read-command", Fread_command, Sread_command, 1, 2, 0,
-- 
2.31.1

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: miha <at> kamnitnik.top
Cc: 48925 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Tue, 20 Jul 2021 14:30:11 +0200

miha <at> kamnitnik.top writes:

> This follows up on changes proposed in bug#45474. The second patch is a
> bit more controversial, but is probably required if we want more
> reliable usage of completion commands in non-innermost minibuffers (that
> is, with minibuffer-follows-selected-frame set to nil.)

Stefan, do have any comments about these two patches?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #11 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: miha <at> kamnitnik.top
Cc: 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Tue, 20 Jul 2021 10:29:40 -0400

> This follows up on changes proposed in bug#45474.

Thanks, the first patch looks good to me (assuming it works ;-).

> The second patch is a bit more controversial, but is probably required
> if we want more reliable usage of completion commands in non-innermost
> minibuffers (that is, with minibuffer-follows-selected-frame set
> to nil.)

The patch is fundamentally right, but as you say it's a bit more
controversial because it risks exposing bugs.  Hmm...

To be on the safer side, I guess we could replace the

    specbind (Qminibuffer_completion_table, Qnil);

with a use of `minibuffer-with-setup-hook` that sets the var to nil in
the new minibuffer.  But doing it in C is awkward so it would best be
done by moving the function to subr.el.


        Stefan

Message #14 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 48925 <at> debbugs.gnu.org, miha <at> kamnitnik.top
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 06:19:17 +0100

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> This follows up on changes proposed in bug#45474.
>
> Thanks, the first patch looks good to me (assuming it works ;-).

So I've now applied it to Emacs 29.  It didn't lead to any obvious
regressions (or test suite failures) that I can see, which is a good
sign.

>> The second patch is a bit more controversial, but is probably required
>> if we want more reliable usage of completion commands in non-innermost
>> minibuffers (that is, with minibuffer-follows-selected-frame set
>> to nil.)
>
> The patch is fundamentally right, but as you say it's a bit more
> controversial because it risks exposing bugs.  Hmm...
>
> To be on the safer side, I guess we could replace the
>
>     specbind (Qminibuffer_completion_table, Qnil);
>
> with a use of `minibuffer-with-setup-hook` that sets the var to nil in
> the new minibuffer.  But doing it in C is awkward so it would best be
> done by moving the function to subr.el.

Sounds like a good idea to me.  Miha, could you do that?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #19 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: <miha <at> kamnitnik.top>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Stefan Monnier
 <monnier <at> iro.umontreal.ca>
Cc: 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 11:42:34 +0100

[Message part 1 (text/plain, inline)]

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> This follows up on changes proposed in bug#45474.
>>
>> Thanks, the first patch looks good to me (assuming it works ;-).
>
> So I've now applied it to Emacs 29.  It didn't lead to any obvious
> regressions (or test suite failures) that I can see, which is a good
> sign.
>
>>> The second patch is a bit more controversial, but is probably required
>>> if we want more reliable usage of completion commands in non-innermost
>>> minibuffers (that is, with minibuffer-follows-selected-frame set
>>> to nil.)
>>
>> The patch is fundamentally right, but as you say it's a bit more
>> controversial because it risks exposing bugs.  Hmm...
>>
>> To be on the safer side, I guess we could replace the
>>
>>     specbind (Qminibuffer_completion_table, Qnil);
>>
>> with a use of `minibuffer-with-setup-hook` that sets the var to nil in
>> the new minibuffer.  But doing it in C is awkward so it would best be
>> done by moving the function to subr.el.
>
> Sounds like a good idea to me.  Miha, could you do that?

Okay, patch attached.

[0001-Set-minibuffer-completion-table-buffer-locally-in-re.patch (text/x-patch, inline)]

From 70fb493398d4961be0fa997684261554822e66b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha <at> kamnitnik.top>
Date: Thu, 11 Nov 2021 11:38:03 +0100
Subject: [PATCH] Set minibuffer-completion-table buffer-locally in read-string

* src/callint.c (Fcall_interactively):
* src/minibuf.c (Fread_string): Move function subr.el and use
minibuffer-with-setup-hook to set minibuffer-completion-table
buffer-locally.
---
 lisp/subr.el  | 26 ++++++++++++++++++++++++++
 src/callint.c | 10 ++++------
 src/minibuf.c | 35 -----------------------------------
 3 files changed, 30 insertions(+), 41 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 5a5842d428..75f00f33d4 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3507,6 +3507,32 @@ y-or-n-p
         (message "%s%c" prompt (if ret ?y ?n)))
       ret)))
 
+(defun read-string ( prompt &optional initial-input history
+                     default-value inherit-input-method)
+  "Read a string from the minibuffer, prompting with string PROMPT.
+If non-nil, second arg INITIAL-INPUT is a string to insert before reading.
+  This argument has been superseded by DEFAULT-VALUE and should normally be nil
+  in new code.  It behaves as INITIAL-CONTENTS in `read-from-minibuffer' (which
+  see).
+The third arg HISTORY, if non-nil, specifies a history list
+  and optionally the initial position in the list.
+See `read-from-minibuffer' for details of HISTORY argument.
+Fourth arg DEFAULT-VALUE is the default value or the list of default values.
+ If non-nil, it is used for history commands, and as the value (or the first
+ element of the list of default values) to return if the user enters the
+ empty string.
+Fifth arg INHERIT-INPUT-METHOD, if non-nil, means the minibuffer inherits
+ the current input method and the setting of `enable-multibyte-characters'."
+  (minibuffer-with-setup-hook
+      (lambda ()
+        (setq-local minibuffer-completion-table nil))
+    (let ((ret (read-from-minibuffer prompt initial-input nil nil
+                                     history default-value
+                                     inherit-input-method)))
+      (if (and default-value (equal "" ret))
+          (if (consp default-value) (car default-value) default-value)
+        ret))))
+
 
 ;;; Atomic change groups.
 
diff --git a/src/callint.c b/src/callint.c
index 44dae361c1..4e80d510ce 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -631,8 +631,8 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
 
 	case 'M':		/* String read via minibuffer with
 				   inheriting the current input method.  */
-	  args[i] = Fread_string (callint_message,
-				  Qnil, Qnil, Qnil, Qt);
+	  args[i] = call5 (intern ("read-string"),
+			   callint_message, Qnil, Qnil, Qnil, Qt);
 	  break;
 
 	case 'N':     /* Prefix arg as number, else number from minibuffer.  */
@@ -672,13 +672,11 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
 
 	case 's':		/* String read via minibuffer without
 				   inheriting the current input method.  */
-	  args[i] = Fread_string (callint_message,
-				  Qnil, Qnil, Qnil, Qnil);
+	  args[i] = call1 (intern ("read-string"), callint_message);
 	  break;
 
 	case 'S':		/* Any symbol.  */
-	  visargs[i] = Fread_string (callint_message,
-				     Qnil, Qnil, Qnil, Qnil);
+	  visargs[i] = call1 (intern ("read-string"), callint_message);
 	  args[i] = Fintern (visargs[i], Qnil);
 	  break;
 
diff --git a/src/minibuf.c b/src/minibuf.c
index 6c0cd358c5..f0f08d97c0 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1366,40 +1366,6 @@ DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
 
 /* Functions that use the minibuffer to read various things.  */
 
-DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
-       doc: /* Read a string from the minibuffer, prompting with string PROMPT.
-If non-nil, second arg INITIAL-INPUT is a string to insert before reading.
-  This argument has been superseded by DEFAULT-VALUE and should normally be nil
-  in new code.  It behaves as INITIAL-CONTENTS in `read-from-minibuffer' (which
-  see).
-The third arg HISTORY, if non-nil, specifies a history list
-  and optionally the initial position in the list.
-See `read-from-minibuffer' for details of HISTORY argument.
-Fourth arg DEFAULT-VALUE is the default value or the list of default values.
- If non-nil, it is used for history commands, and as the value (or the first
- element of the list of default values) to return if the user enters the
- empty string.
-Fifth arg INHERIT-INPUT-METHOD, if non-nil, means the minibuffer inherits
- the current input method and the setting of `enable-multibyte-characters'.  */)
-  (Lisp_Object prompt, Lisp_Object initial_input, Lisp_Object history, Lisp_Object default_value, Lisp_Object inherit_input_method)
-{
-  Lisp_Object val;
-  ptrdiff_t count = SPECPDL_INDEX ();
-
-  /* Just in case we're in a recursive minibuffer, make it clear that the
-     previous minibuffer's completion table does not apply to the new
-     minibuffer.
-     FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
-  specbind (Qminibuffer_completion_table, Qnil);
-
-  val = Fread_from_minibuffer (prompt, initial_input, Qnil,
-			       Qnil, history, default_value,
-			       inherit_input_method);
-  if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
-    val = CONSP (default_value) ? XCAR (default_value) : default_value;
-  return unbind_to (count, val);
-}
-
 DEFUN ("read-command", Fread_command, Sread_command, 1, 2, 0,
        doc: /* Read the name of a command and return as a symbol.
 Prompt with PROMPT.  By default, return DEFAULT-VALUE or its first element
@@ -2513,7 +2479,6 @@ syms_of_minibuf (void)
   defsubr (&Sactive_minibuffer_window);
   defsubr (&Sset_minibuffer_window);
   defsubr (&Sread_from_minibuffer);
-  defsubr (&Sread_string);
   defsubr (&Sread_command);
   defsubr (&Sread_variable);
   defsubr (&Sinternal_complete_buffer);
-- 
2.33.1

[signature.asc (application/pgp-signature, inline)]

Message #22 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: <miha <at> kamnitnik.top>
Cc: larsi <at> gnus.org, monnier <at> iro.umontreal.ca, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 13:27:01 +0200

> Cc: 48925 <at> debbugs.gnu.org
> Date: Thu, 11 Nov 2021 11:42:34 +0100
> From: miha--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> >> To be on the safer side, I guess we could replace the
> >>
> >>     specbind (Qminibuffer_completion_table, Qnil);
> >>
> >> with a use of `minibuffer-with-setup-hook` that sets the var to nil in
> >> the new minibuffer.  But doing it in C is awkward so it would best be
> >> done by moving the function to subr.el.
> >
> > Sounds like a good idea to me.  Miha, could you do that?
> 
> Okay, patch attached.

Moving read-string to subr.el means the function will be unavailable
during loadup until subr.elc is loaded.

What is awkward to do in C?  Maybe I could help with that, so that we
wouldn't need to move this to Lisp.

Thanks.

Message #25 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 13:11:10 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> Moving read-string to subr.el means the function will be unavailable
> during loadup until subr.elc is loaded.

That's a worry, so I tried Miha's patch now and did both a "make" and a
"make bootstrap", and both completed without any problems.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #28 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: <miha <at> kamnitnik.top>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, monnier <at> iro.umontreal.ca, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 14:04:17 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 48925 <at> debbugs.gnu.org
>> Date: Thu, 11 Nov 2021 11:42:34 +0100
>> From: miha--- via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>> >> To be on the safer side, I guess we could replace the
>> >>
>> >>     specbind (Qminibuffer_completion_table, Qnil);
>> >>
>> >> with a use of `minibuffer-with-setup-hook` that sets the var to nil in
>> >> the new minibuffer.  But doing it in C is awkward so it would best be
>> >> done by moving the function to subr.el.
>> >
>> > Sounds like a good idea to me.  Miha, could you do that?
>> 
>> Okay, patch attached.
>
> Moving read-string to subr.el means the function will be unavailable
> during loadup until subr.elc is loaded.

Sorry, I forgot to include a disclaimer that I don't really know that
much about loadup and bootstrapping, I kind of just blindly moved the
function to lisp saw that "make" worked. I did check loadup.el and saw
that `read-string' or `call-interactively' aren't used directly before
subr.el, but I'm not sure that this is sufficient.

It may be used indirectly through `command-execute', which is defined in
simple.el, so I think it should be okay.

> What is awkward to do in C?  Maybe I could help with that, so that we
> wouldn't need to move this to Lisp.

We want to do what `minibuffer-with-setup-hook' does: add a function to
a hook that will remove itself from this hook. If I understand
correctly, we'd have to do this without `add-hook' and `remove-hook'
since they are defined in subr.el.

> Thanks.

[signature.asc (application/pgp-signature, inline)]

Message #31 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 16:17:25 +0200

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: <miha <at> kamnitnik.top>,  monnier <at> iro.umontreal.ca,  48925 <at> debbugs.gnu.org
> Date: Thu, 11 Nov 2021 13:11:10 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Moving read-string to subr.el means the function will be unavailable
> > during loadup until subr.elc is loaded.
> 
> That's a worry, so I tried Miha's patch now and did both a "make" and a
> "make bootstrap", and both completed without any problems.

I didn't say it will be a problem now.  But it's a time bomb waiting
to go off.  So I'd like to see if we could still do this in C.

Message #34 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: <miha <at> kamnitnik.top>
To: Eli Zaretskii <eliz <at> gnu.org>, Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: monnier <at> iro.umontreal.ca, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 17:50:33 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi <at> gnus.org>
>> Cc: <miha <at> kamnitnik.top>,  monnier <at> iro.umontreal.ca,  48925 <at> debbugs.gnu.org
>> Date: Thu, 11 Nov 2021 13:11:10 +0100
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > Moving read-string to subr.el means the function will be unavailable
>> > during loadup until subr.elc is loaded.
>> 
>> That's a worry, so I tried Miha's patch now and did both a "make" and a
>> "make bootstrap", and both completed without any problems.
>
> I didn't say it will be a problem now.  But it's a time bomb waiting
> to go off.  So I'd like to see if we could still do this in C.

In that case, my personal opinion is that it's okay to leave it as is
and close this bug.

The specbinding in `read-string' isn't a very big problem. The only
problematic case I can think of is quite specific: the user runs a
function that let-binds `minibuffer-completion-table' around a call to
`read-from-minibuffer' (this is the old convention, the new convection
is to set the completion table buffer locally), and then recursively
uses `read-string' during this minibuffer session on a separate frame
with `minibuffer-follows-selected-frame' customized to nil. Completion
commands will now not work in the outer minibuffer.

IMO, it's not really worth trying too hard to figure out a way to fix
this very specific issue in C. One simple solution would be to introduce
a new optional argument to `read-from-minibuffer', a function that would
be run in the minibuffer as an alternative to
minibuffer-with-setup-hook. I believe Stefan M. proposed something like
this, but this should probably be discussed more thoroughly.

[signature.asc (application/pgp-signature, inline)]

Message #37 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 48925 <at> debbugs.gnu.org, miha <at> kamnitnik.top
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Thu, 11 Nov 2021 18:58:51 -0500

> Moving read-string to subr.el means the function will be unavailable
> during loadup until subr.elc is loaded.

I believe this should not be a problem: `read-string` is only used for
interaction with the user so it's never used until much later than the
load of `subr.el` (it's not used during bootstrap).

> What is awkward to do in C?

We don't have anby facility to create closures from C, so we'd basically
have to call an ELisp function to create the closure.


        Stefan

Message #40 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, larsi <at> gnus.org, miha <at> kamnitnik.top,
 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Fri, 12 Nov 2021 00:22:58 +0000

>> What is awkward to do in C?
>
> We don't have anby facility to create closures from C, so we'd basically 
> have to call an ELisp function to create the closure.
>

BTW, these patches would be much simpler and this discussion would not 
exist with the approach I defended in bug#45474.

Message #43 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Fri, 12 Nov 2021 03:47:54 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> I didn't say it will be a problem now.  But it's a time bomb waiting
> to go off.  So I'd like to see if we could still do this in C.

It's possible, of course, but it does seem unlikely that we'd start
using `read-string' during the early build.  (Especially since we're
apparently not doing that now.)  I'm having a hard time imagining when
there'd be a call to that function at that point.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #46 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Fri, 12 Nov 2021 10:57:10 +0200

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: miha <at> kamnitnik.top,  monnier <at> iro.umontreal.ca,  48925 <at> debbugs.gnu.org
> Date: Fri, 12 Nov 2021 03:47:54 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > I didn't say it will be a problem now.  But it's a time bomb waiting
> > to go off.  So I'd like to see if we could still do this in C.
> 
> It's possible, of course, but it does seem unlikely that we'd start
> using `read-string' during the early build.  (Especially since we're
> apparently not doing that now.)  I'm having a hard time imagining when
> there'd be a call to that function at that point.

The probability is low, indeed, but it isn't zero.

Given what Miha wrote, maybe we don't need to make any changes at all
here?

Message #49 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Sun, 14 Nov 2021 01:49:37 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> The probability is low, indeed, but it isn't zero.

Given such a low probability, it's worth a shot.

> Given what Miha wrote, maybe we don't need to make any changes at all
> here?

In which message?  I re-skimmed the thread, but didn't see Miha saying
that it's not needed?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #52 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Sun, 14 Nov 2021 08:57:23 +0200

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: monnier <at> iro.umontreal.ca,  miha <at> kamnitnik.top,  48925 <at> debbugs.gnu.org
> Date: Sun, 14 Nov 2021 01:49:37 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Given what Miha wrote, maybe we don't need to make any changes at all
> > here?
> 
> In which message?

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48925#34

Message #55 received at 48925 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, miha <at> kamnitnik.top, 48925 <at> debbugs.gnu.org
Subject: Re: bug#48925: [PATCH] Set `minibuffer-completion-*` variables
 buffer-locally in a few more places
Date: Mon, 15 Nov 2021 06:31:05 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> > Given what Miha wrote, maybe we don't need to make any changes at all
>> > here?
>> 
>> In which message?
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48925#34

My reading of that message is that Miha is saying that if we have to do
it in C, it'd be too much work just to fix this problem.

But I don't think we have to do it in C, because `read-string' can live
in subr.el just fine.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.