GNU bug report logs - #77718
31.0.50; completion styles substring and flex are broken

Previous Next

Full log

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77718 <at> debbugs.gnu.org, stephen.berman <at> gmx.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 15:24:45 -0400

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

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

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: 77718 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,  Stephen Berman
>>   <stephen.berman <at> gmx.net>
>> Date: Mon, 14 Apr 2025 16:30:09 -0400
>> 
>> +     (;; Ensure we discard text before // by always using sifn in that case.
>> +      (unless (string-search "//" orig)
>> +        (complete-with-action action table orig pred)))
>
> AFAIU, this ignores the case of UNC file names, which begin with "//",
> and other complications with file names on Windows.  Emacs on Windows
> reacts differently to "//" than it does on Posix systems, and it does
> so for very good reasons.

Ah, thanks, good point.  I updated it to remove this hardcoded handling
of //, and instead defer to substitute-in-file-name, which will behave
right on Windows.  Updated patch attached.

> Please don't break file-name completion on Windows.  It took us many
> iterations to get it right, and the result is fragile.  Be sure to
> test each change on Windows as well before installing.

Can do, I'm working on setting up a test Windows system right now.

Is there any way to approximately test the behavior of Windows file name
completion while still on GNU/Linux?  It might be useful to have tests
which can run on GNU/Linux but test the Windows behavior.

> For the same reasons, please test thoroughly with remote file names of
> different formats.

Will do - I'll make some additions to tramp-tests as well.

[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]

From d35a5068cb9d9a75a19914fb624da5b0bd461937 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
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)
(completion--complete-on-expansion): 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            | 57 ++++++++++++++++++++++++++++++++---
 test/lisp/minibuffer-tests.el | 16 +++++-----
 2 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ab00395a850..e53bd673df9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3565,13 +3565,60 @@ 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))
+    (or
+     ;; First try completing without expanding environment variables; we still
+     ;; call `substitute-in-file-name' to handle // and other constructs.
+     (let ((sifned (let ((process-environment nil)) (substitute-in-file-name orig))))
+       ;; Fall through to the second case if `substitute-in-file-name' expanded
+       ;; something anyway.
+       (when (string-suffix-p sifned orig)
+         (let ((discarded (string-remove-suffix sifned orig))
+               (result (complete-with-action action table sifned pred)))
+           (cond
+            ((eq (car-safe action) 'boundaries)
+             (cons 'boundaries (cons (+ (length discarded) (cadr result)) (cddr result))))
+            ((null action)
+             (if (stringp result) (concat discarded result) result))
+            (t result)))))
+     ;; Try again, this time expanding environment variables.
+     (cond
+      ((eq (car-safe action) 'boundaries)
+       ;; Compute the boundaries on ORIG; boundaries created by expanding
+       ;; environment variables can't be used.
+       (complete-with-action action table orig pred))
+      ((let* ((sifned (substitute-in-file-name orig))
+              (result (complete-with-action action table sifned pred)))
+         (cond
+          ((null action)
+           (if (stringp result)
+               (completion--replace-prefix result sifned orig)
+             result))
+          ((eq action t)
+           (when result
+             ;; Find the part of SIFNED which is inside the boundaries, and
+             ;; replace it with the part of ORIG which is inside the boundaries.
+             (let* ((expansion-bounds (completion-boundaries sifned table pred ""))
+                    (expansion-in-bounds (substring sifned (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

Previous Next

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