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

From: Spencer Baugh <[email-masked]>
Date: Mon, 14 Apr 2025 16:30:09 -0400

Stefan Monnier <[email-masked]> writes:

>> Stefan, a related question: do you think it would be reasonable to split
>> completion-table-with-quoting into two functions, separating the
>> substitute-in-file-name and comint-quote-filename cases?  It seems to me
>> that the comint-quote-filename case is way simpler than the
>> substitute-in-file-name case, and it would be a lot easier to understand
>> if the two cases were done separately, without the abstraction of
>> requote and unquote functions.  I can do that if that seems reasonable.
>> It would make fixing this issue a lot easier.
>
> It's worth a try.  The system we have is the best I could come up with
> back then but is hideous, so I'm interested in a simpler solution.

OK, here is a rework of just the substitute-in-file-name side.  This
fixes this bug (by making completion continue to work when point is at
/usr/|/), and fixes another FIXME in a test (partial completion now
preserves environment variables!).  And I don't think it loses anything
important, but I may be missing something.

(completion--sifn-requote can now be deleted and there are probably
other cleanups possible after this patch, but I didn't do them here to
keep the patch smaller.  Also, rfn-eshadow needs to be updated, but
that's just a visual issue.)

>From 48653fd0b22739f6d836bbe121989f5051502d81 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.

The completion table first tries to complete without
substituting environment variables; this allows completion to
work on file names which contain things which look like
environment variables.  If that fails, we call
substitute-in-file-name and then complete again.

Tests are updated: we no longer duplicate $s in file names,
since that's not necessary.  And 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--replace-prefix): Add.
(completion--file-name-table): Rewrite to use
substitute-in-file-name explicitly. (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting)
(minibuffer-next-completion): Update.
---
 lisp/minibuffer.el            | 43 +++++++++++++++++++++++++++++++----
 test/lisp/minibuffer-tests.el | 16 ++++++-------
 2 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c0ccfc00ce5..4ccf3a1d2f9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3596,13 +3596,46 @@ 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--replace-prefix (s old new)
+  "Replace prefix OLD of string S with prefix NEW."
+  (concat new (substring s (length old))))
+
+(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))
+    (cond
+     ((eq (car-safe action) 'boundaries)
+      ;; Boundaries are always computed on the original string.
+      (complete-with-action action table orig pred))
+     (;; Ensure we discard text before // by always using sifn in that case.
+      (unless (string-search "//" orig)
+        (complete-with-action action table orig pred)))
+     (t ;; On a nil result, complete again after sifn and adjust the result.
+      (let* ((expansion (substitute-in-file-name orig))
+             (result (complete-with-action action table expansion pred)))
+        (cond
+         ((null action)
+          (if (stringp result)
+              (completion--replace-prefix result expansion orig)
+            result))
+         ((eq action t)
+          ;; Find the part of EXPANSION which is inside the boundaries, and
+          ;; replace it with the part of ORIG which is inside the boundaires.
+          (let* ((expansion-bounds (completion-boundaries expansion table pred ""))
+                 (expansion-in-bounds (substring expansion (car expansion-bounds)))
+                 (orig-bounds (completion-boundaries orig table pred ""))
+                 (orig-in-bounds (substring orig (car orig-bounds))))
+            (if (and (string-empty-p expansion-in-bounds)
+                     (string-empty-p orig-in-bounds))
+                ;; Nothing to replace, so avoid doing unnecessary work.
+                result
+              (mapcar
+               (lambda (s)
+                 (completion--replace-prefix s expansion-in-bounds orig-in-bounds))
+               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..38c005bd1b5 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -91,20 +91,18 @@ 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)
                    '(
-                     ;; Test that $ in files is properly $$ quoted.
-                     ("data/m-cttq" "data/minibuffer-test-cttq$$tion")
-                     ;; Test that $$ in input is properly unquoted.
-                     ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
+                     ;; Test that files containing $ can be completed
+                     ("data/m-cttq" "data/minibuffer-test-cttq$tion")
+                     ;; Test that $ can be completed
+                     ("data/m-cttq$" "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
@@ -690,7 +688,7 @@ minibuffer-next-completion
     (completing-read-with-minibuffer-setup #'read-file-name-internal
       (insert "d/")
       (execute-kbd-macro (kbd "M-<down> M-<down> M-<down>"))
-      (should (equal "data/minibuffer-test-cttq$$tion" (minibuffer-contents))))))
+      (should (equal "data/minibuffer-test-cttq$tion" (minibuffer-contents))))))
 
 (provide 'minibuffer-tests)
 ;;; minibuffer-tests.el ends here
-- 
2.39.3