Package: emacs;
Reported by: Stephen Berman <stephen.berman <at> gmx.net>
Date: Thu, 10 Apr 2025 22:23:02 UTC
Severity: normal
Found in version 31.0.50
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
View this message in rfc822 format
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net Subject: bug#77718: 31.0.50; completion styles substring and flex are broken Date: Tue, 22 Apr 2025 14:52:16 -0400
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> 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))))
>
> What makes us think that the `cl-assert` will always be satisfied?
> Won't it fail in `/blabla/foo-$BAR.c` when `BAR=bash` ?
Because I bound process-environment to nil around the
substitute-in-file-name call, with the goal of preventing environment
variable expansion.
But actually there's a simpler and better way to do that: just duplicate
all the $s to $$, then substitute-in-file-name turns all the $$ back to
$ without expanding environment variables. So I did that in the
attached updated patch.
>> @@ -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")
>
> Why do we remove this test? Maybe it's OK that it doesn't return the
> same value (e.g. that it behaves less well), but if so, we should
> probably keep this commented out with an explanation of what's going on.
Fixed - actually, this test doesn't need to change at all, it still
works, just an oversight on my part.
[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]
From c2773fbc301ef0b6eb34b84da181e916d563c490 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.
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 | 63 ++++++++++++++++++++++++++++++++---
test/lisp/minibuffer-tests.el | 3 +-
2 files changed, 59 insertions(+), 7 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c0ccfc00ce5..efe2eba9539 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3596,13 +3596,66 @@ 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."
+ ;; We want to compute the start boundary on the result of
+ ;; `substitute-in-file-name' (since that's what we use for actual completion),
+ ;; and then transform that into an offset in STRING instead. We can't do this
+ ;; if we expand environment variables, so double the $s to prevent that.
+ (let* ((doubled-string (replace-regexp-in-string "\\$" "$$" string t t))
+ ;; sifn will change $$ back into $, so the result is a suffix of STRING
+ ;; (in fact, it's the last absolute file name in STRING).
+ (last-file-name (substitute-in-file-name doubled-string))
+ (bounds (completion-boundaries last-file-name table pred suffix)))
+ (cl-assert (string-suffix-p last-file-name string) t)
+ ;; BOUNDS contains the start boundary in LAST-FILE-NAME; adjust it to be an
+ ;; offset in STRING instead.
+ (cons (+ (- (length string) (length last-file-name)) (car bounds))
+ ;; No special processing happens on SUFFIX and the end boundary.
+ (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 57c7cc2ae1a..00a2055e602 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -103,8 +103,7 @@ completion-table-test-quoting
("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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.