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>
Message #20 received at 77718 <at> debbugs.gnu.org (full text, mbox):
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 <stephen.berman <at> gmx.net> Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are broken Date: Mon, 14 Apr 2025 16:30:09 -0400
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> iro.umontreal.ca> 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.)
[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]
From 48653fd0b22739f6d836bbe121989f5051502d81 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): 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.