Subject: bug#77718: 31.0.50; completion styles substring and flex are broken

From: Spencer Baugh <[email-masked]>
Date: Thu, 17 Apr 2025 12:39:48 -0400

Stefan Monnier <[email-masked]> writes:
>> I will try to think of another approach which is more similar to how
>> completion-table-with-quoting does things, but it will be more
>> complicated.
>
> I thought all that's missing is to double the $$ in the text we append
> during completion.

Yes, actually you're right, that turns out to be relatively simple.  And
now the quoting behavior doesn't change at all.  Updated patch attached.

The $ doubling also needs to happen in all-completions, not just
try-completion.  But, with sufficiently robust logic for figuring out
the completion boundaries, that's straightforward: just double the $ in
the newly added text.

>From 6a8685edc22cba617ddced1f2b2e1b6c4e1c9de2 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <[email-masked]>
Date: Mon, 14 Apr 2025 16:01:38 -0400
Subject: [PATCH] Improve env var handling in read-file-name

Fix various bugs, including bug#77718, by rewriting the way file
name completion handles environment variable expansion.  Instead
of using completion-table-with-quoting to manipulate the string
being completed on, simply make the completion table itself
understand substitute-in-file-name.

Tests are updated: partial-completion now preserves unexpanded
environment variables.  However, partial-completion no longer
works across environment variables containing delimiters; that's
an acceptable sacrifice.

* lisp/minibuffer.el (completion--sifn-boundaries): Add.
(completion--file-name-table): Rewrite to use
substitute-in-file-name explicitly. (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting):
Update.
---
 lisp/minibuffer.el            | 59 ++++++++++++++++++++++++++++++++---
 test/lisp/minibuffer-tests.el |  6 ++--
 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c0ccfc00ce5..8edeba1fba9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3596,13 +3596,62 @@ completion--sifn-requote
           (setq qpos (1- qpos)))
         (cons qpos #'minibuffer-maybe-quote-filename)))))
 
-(defalias 'completion--file-name-table
-  (completion-table-with-quoting #'completion-file-name-table
-                                 #'substitute-in-file-name
-                                 #'completion--sifn-requote)
+(defun completion--sifn-boundaries (string table pred suffix)
+  "Return completion boundaries on file name STRING.
+
+Runs `substitute-in-file-name' on STRING first, but returns completion
+boundaries for the original string."
+  (let* (;; Calculate the completion boundaries without expanding env vars.
+         (unquoted
+          (let ((process-environment nil)) (substitute-in-file-name string)))
+         (bounds (completion-boundaries unquoted table pred suffix))
+         ;; This is the part of UNQUOTED inside the completion boundaries.
+         (unquoted-suffix (substring unquoted (car bounds)))
+         ;; Each $ in UNQUOTED-SUFFIX is either $ or $$ in STRING.
+         (regex
+          (concat (replace-regexp-in-string "\\$" "\\$\\$?" unquoted-suffix t t) "$")))
+    (cl-assert (string-match regex string) t)
+    (cons (match-beginning 0) (cdr bounds))))
+
+(defun completion--file-name-table (orig pred action)
   "Internal subroutine for `read-file-name'.  Do not call this.
 This is a completion table for file names, like `completion-file-name-table'
-except that it passes the file name through `substitute-in-file-name'.")
+except that it passes the file name through `substitute-in-file-name'."
+  (let ((table #'completion-file-name-table))
+    (if (eq (car-safe action) 'boundaries)
+        (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
+      (let* ((sifned (substitute-in-file-name orig))
+             (result
+              (let ((completion-regexp-list
+                     ;; Regexps are matched against the real file names after
+                     ;; expansion, so regexps containing $ won't work.  Drop
+                     ;; them; we'll return more completions, but callers need to
+                     ;; handle that anyway.
+                     (cl-remove-if (lambda (regexp) (string-search "$" regexp))
+                                   completion-regexp-list)))
+                (complete-with-action action table sifned pred))))
+        (cond
+         ((null action)                 ; try-completion
+          (if (stringp result)
+              ;; Extract the newly added text, quote any dollar signs, and
+              ;; append it to ORIG.
+              (let ((new-text (substring result (length sifned))))
+                (concat orig (minibuffer--double-dollars new-text)))
+            result))
+         ((eq action t)                 ; all-completions
+          (mapcar
+           (let ((orig-prefix
+                  (substring orig (car (completion--sifn-boundaries orig table pred ""))))
+                 (sifned-prefix-length
+                  (- (length sifned)
+                     (car (completion-boundaries sifned table pred "")))))
+             ;; Extract the newly added text, quote any dollar signs, and append
+             ;; it to the part of ORIG inside the completion boundaries.
+             (lambda (compl)
+               (let ((new-text (substring compl sifned-prefix-length)))
+                 (concat orig-prefix (minibuffer--double-dollars new-text)))))
+           result))
+         (t result))))))
 
 (defalias 'read-file-name-internal
   (completion-table-in-turn #'completion--embedded-envvar-table
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 59ac5ab9578..9e28077026c 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -91,7 +91,7 @@ completion-table-subvert-test
 
 (ert-deftest completion-table-test-quoting ()
   (let ((process-environment
-         `("CTTQ1=ed" "CTTQ2=et/" ,@process-environment))
+         `("CTTQ1=ed" ,@process-environment))
         (default-directory (ert-resource-directory)))
     (pcase-dolist (`(,input ,output)
                    '(
@@ -101,10 +101,8 @@ completion-table-test-quoting
                      ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
                      ;; Test that env-vars are preserved.
                      ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
-                     ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
                      ;; Test that env-vars don't prevent partial-completion.
-                     ;; FIXME: Ideally we'd like to keep the ${CTTQ}!
-                     ("lis/c${CTTQ1}/se-u" "lisp/cedet/semantic-utest")
+                     ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
                      ))
       (should (equal (completion-try-completion input
                                                 #'completion--file-name-table
-- 
2.39.3