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

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

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 266 bytes --]

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.)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Set-minibuffer-completion-variables-locally-in-more-.patch --]
[-- Type: text/x-patch, Size: 6775 bytes --]

From 049d57e6d10edca1d6a0af119f557e364d8ea93f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-Don-t-bind-minibuffer-completion-table-to-nil-in-rea.patch --]
[-- Type: text/x-patch, Size: 1657 bytes --]

From 466169b9f679a78aec00f9735335d90718c0d898 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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

[Lars Ingebrigtsen]

miha@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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Lars Ingebrigtsen]

Stefan Monnier <monnier@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

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

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 1130 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Set-minibuffer-completion-table-buffer-locally-in-re.patch --]
[-- Type: text/x-patch, Size: 6158 bytes --]

From 70fb493398d4961be0fa997684261554822e66b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@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))))
+
 \f
 ;;; 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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

[Eli Zaretskii]

> Cc: 48925@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@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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 48925@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@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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: <miha@kamnitnik.top>,  monnier@iro.umontreal.ca,  48925@debbugs.gnu.org
> Date: Thu, 11 Nov 2021 13:11:10 +0100
> 
> Eli Zaretskii <eliz@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.

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

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: <miha@kamnitnik.top>,  monnier@iro.umontreal.ca,  48925@debbugs.gnu.org
>> Date: Thu, 11 Nov 2021 13:11:10 +0100
>> 
>> Eli Zaretskii <eliz@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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Gregory Heytings]

>> 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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: miha@kamnitnik.top,  monnier@iro.umontreal.ca,  48925@debbugs.gnu.org
> Date: Fri, 12 Nov 2021 03:47:54 +0100
> 
> Eli Zaretskii <eliz@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?

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: monnier@iro.umontreal.ca,  miha@kamnitnik.top,  48925@debbugs.gnu.org
> Date: Sun, 14 Nov 2021 01:49:37 +0100
> 
> Eli Zaretskii <eliz@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

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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