From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Spencer Baugh via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#74420: 31.0.50; PCM completion for ~/src/emacs/trunk/*/minibuf breaks Date: Mon, 18 Nov 2024 17:17:06 -0500 Message-ID: References: Reply-To: Spencer Baugh Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="9194"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 74420@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Nov 18 23:18:28 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1tDA4h-0002DS-PB for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 18 Nov 2024 23:18:28 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1tDA4L-0006LD-WE; Mon, 18 Nov 2024 17:18:06 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tDA4J-0006Kf-8v for bug-gnu-emacs@gnu.org; Mon, 18 Nov 2024 17:18:03 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1tDA4J-0007HN-0o for bug-gnu-emacs@gnu.org; Mon, 18 Nov 2024 17:18:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:Date:References:In-Reply-To:From:To:Subject; bh=xolQFc+QiuW3SFmn1paIRWMe5T9D3Z80Jd/bAcToOiE=; b=vjX3VI6I/cPrOlXYnMT1Guog1jw/8TfNu+B/u4gRWpuU/sPdvuHxGacXSiZV2Fk1wSKNC+4FncYstTd7UZD1TbAkrnj06yuHef0i8CliiZD+5lCEV8RWPuqlsT6ufNwYM79VgoYicGsV82tAmkRCRbf5n5ubdVmo1H5PdI4P+y0FDbSF0fHHXUV9836qdX5S7F77p7ZsR4kr5S9+N7DgCaKrqyFpZ2cViNDxaerjH4S3llMMqCGaYxsFLaFLMdinDc+He9CGAnI49Qm4KakT+OBE2SGXNAJOdrkoINJnqDHIOZ3q9C+mGoYEb5jrKhYbWz4KjlYLgvULfn3v7qtuQg==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1tDA4I-0005s8-IN for bug-gnu-emacs@gnu.org; Mon, 18 Nov 2024 17:18:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Spencer Baugh Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 18 Nov 2024 22:18:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 74420 X-GNU-PR-Package: emacs Original-Received: via spool by 74420-submit@debbugs.gnu.org id=B74420.173196823522512 (code B ref 74420); Mon, 18 Nov 2024 22:18:02 +0000 Original-Received: (at 74420) by debbugs.gnu.org; 18 Nov 2024 22:17:15 +0000 Original-Received: from localhost ([127.0.0.1]:39503 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tDA3W-0005r1-Hw for submit@debbugs.gnu.org; Mon, 18 Nov 2024 17:17:15 -0500 Original-Received: from mxout5.mail.janestreet.com ([64.215.233.18]:41571) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tDA3T-0005qm-Ve for 74420@debbugs.gnu.org; Mon, 18 Nov 2024 17:17:13 -0500 In-Reply-To: (Stefan Monnier's message of "Mon, 18 Nov 2024 15:39:17 -0500") DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=janestreet.com; s=waixah; t=1731968226; bh=xolQFc+QiuW3SFmn1paIRWMe5T9D3Z80Jd/bAcToOiE=; h=From:To:Cc:Subject:In-Reply-To:References:Date; b=20L8z47+LaC1C3CXnY9aKQ/MCm7nfqPpL5KUGc2w7dK+3wM8MhwkWAHvTrWXdNVjm vvRUCATldZz7r8vfNdWjR1MF23GHgzzbG23rbUovXk6PsyuUzlcyp88NUpk4RAdFyx WRJFja8q7nJcrGzoYKaPdFvxtfYfVuCRL3mThgkKIQAzaGzXDutqOB14HAWOV6z9nI fGWdnyuCcZ1OWBuf2xk1DTWrPwAzv615xyS6P4kcs22qg1dLdIwCGZNkGrnszpaD0f thTuXyxdHOFjfoWdhGoCUxFS/fdxC+/JMcxVqabwdHYJl2NDSEjKSWUbz7tKVDRrOG SP13TvNhJRq0Q== X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:295614 Archived-At: --=-=-= Content-Type: text/plain Stefan Monnier writes: >> An explicitly typed * has different semantics from automatically >> inserted PCM wildcards, so it should be preserved on >> try-completion. We already do this in some cases, but now we do >> it more. Concretely, we do it by optimizing the PCM pattern >> more aggressively to avoid having multiple wildcards in a row: >> after those are removed, the existing code in >> completion-pcm--merge-completions is able to preserve * in more >> cases. > > Oh, indeed, thank you. > > The patch looks good to me, so if there are no objection, I'll install > it in a few days (feel free to ping me if I forget). Actually... I just realized this misses some cases, namely when we have "star point" or "point star". Attached at the end is a different patch which takes a different approach (and which includes new tests for the problematic cases). The changes to optimize still seem reasonable to me, since they might improve performance, so feel free to install it if you think it's good. Although maybe we want to revise it with the below discussion. > Banter follows. > >> * lisp/minibuffer.el (completion-pcm--optimize-pattern): Add >> more optimizations. (bug#74420) > > Aha, so that's why you didn't submit the patch together with the initial > bug report! > >> - (`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_) >> - (setq p (cdr p))) >> + ;; Remove duplicate `any' and `prefix' >> + (`(any any . ,rest) >> + (setq p (cons 'any rest))) >> + (`(prefix prefix . ,rest) >> + (setq p (cons 'prefix rest))) >> + ;; `any' matches anything `any-delim' does, and grows the same way. >> + (`(any-delim any . ,rest) >> + (setq p (cons 'any rest))) > > AFAICT you can still merge the `any-delim any` and `any any` cases. True, I just thought this was a bit easier to read, since the cases are justified as an optimization in slightly different ways. >> + ;; Remove other wildcards found around `star' or `point'. >> + ((or `(,(and keep (or 'star 'point)) ,(or 'any 'any-delim 'prefix) . ,rest) >> + `(,(or 'any 'any-delim 'prefix) ,(and keep (or 'star 'point)) . ,rest)) >> + (setq p (cons keep rest))) > > BTW, maybe we should go with something like > > (`(,(and (pred symbolp) s1) ,(and (pred symbolp) s2) . ,rest) > (cond ((completion-pcm---p s1 s2) (setq p (cons s1 rest))) > ((completion-pcm---p s2 s1) (setq p (cons s2 rest))) > (t (push (pop p) n)))) > > Where `completion-pcm---p` is some kind of partial ordering? Interesting thought. Maybe it would make sense to have something like - completion-pcm--wildcard-grows-on-left-p (non-nil for star, point, any, any-delim) - completion-pcm--wildcard-grows-on-right-p (non-nil for star, point, prefix) - completion-pcm--wildcard-in-text-p (non-nil for star and point) Then this case would be something like "if grows-left/right-p is the same, and at most one of the symbols is in-text-p, delete the non-in-text-p one". Also, then the first two helpers could be used in completion-pcm--merge-completions as well, which could make the wildcard behavior easier to understand. (I want to do a bit of refactoring in completion-pcm--merge-completions, especially after my attached change; I think it could all be a bit easier to understand.) --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Preserve-an-explicit-in-pcm-try-completion.patch >From 8c2ff1152800ede05cb9a9fd80deac45646ce52f Mon Sep 17 00:00:00 2001 From: Spencer Baugh Date: Mon, 18 Nov 2024 12:26:55 -0500 Subject: [PATCH] Preserve an explicit * in pcm-try-completion An explicitly typed * has different semantics from automatically inserted PCM wildcards, so it should be preserved on try-completion. We already do this in some cases, but now we do it more. This is especially significant for filename completion: removing an explicit * can take us from ~/src/emacs/trunk/*/minibuf to ~/src/emacs/trunk//minibuf The explicit double slash is interpreted by the file name completion table to mean "start completing from the root directory", so deleting the * here substantially changes semantics. * lisp/minibuffer.el (completion-pcm--merge-completions): Don't drop important wildcards. (bug#74420) * test/lisp/minibuffer-tests.el (completion-pcm-test-7): Add tests. --- lisp/minibuffer.el | 21 +++++++++++++++------ test/lisp/minibuffer-tests.el | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el index 5f3f5d3ead1..2d27fef44ab 100644 --- a/lisp/minibuffer.el +++ b/lisp/minibuffer.el @@ -4513,12 +4513,17 @@ completion-pcm--merge-completions ;; Then for each of those non-constant elements, extract the ;; commonality between them. (let ((res ()) - (fixed "")) + (fixed "") + ;; Accumulate each stretch of wildcards, and process them as a unit. + (wildcards ())) ;; Make the implicit trailing `any' explicit. (dolist (elem (append pattern '(any))) (if (stringp elem) - (setq fixed (concat fixed elem)) + (progn + (setq fixed (concat fixed elem)) + (setq wildcards nil)) (let ((comps ())) + (push elem wildcards) (dolist (cc (prog1 ccs (setq ccs nil))) (push (car cc) comps) (push (cdr cc) ccs)) @@ -4542,14 +4547,16 @@ completion-pcm--merge-completions (push prefix res) ;; `prefix' only wants to include the fixed part before the ;; wildcard, not the result of growing that fixed part. - (when (eq elem 'prefix) + (when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards) (setq prefix fixed)) (push prefix res) - (push elem res) + ;; Push all the wildcards in this stretch, to preserve `point' and + ;; `star' wildcards before ELEM. + (setq res (nconc wildcards res)) ;; Extract common suffix additionally to common prefix. ;; Don't do it for `any' since it could lead to a merged ;; completion that doesn't itself match the candidates. - (when (and (memq elem '(star point prefix)) + (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards) ;; If prefix is one of the completions, there's no ;; suffix left to find. (not (assoc-string prefix comps t))) @@ -4563,7 +4570,9 @@ completion-pcm--merge-completions comps)))))) (cl-assert (stringp suffix)) (unless (equal suffix "") - (push suffix res))))) + (push suffix res)))) + ;; We pushed these wildcards on RES, so we're done with them. + (setq wildcards nil)) (setq fixed ""))))) ;; We return it in reverse order. res))))) diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el index 38c2b8c4552..c166450d87a 100644 --- a/test/lisp/minibuffer-tests.el +++ b/test/lisp/minibuffer-tests.el @@ -258,6 +258,31 @@ completion-pcm-test-6 (car (completion-pcm-all-completions "li-pac*" '("do-not-list-packages") nil 7))))) +(ert-deftest completion-pcm-test-7 () + ;; Wildcards are preserved even when right before a delimiter. + (should (equal + (completion-pcm-try-completion + "x*/" + '("x1/y1" "x2/y2") + nil 3) + '("x*/y" . 4))) + ;; Or around point. + (should (equal + (completion-pcm--merge-try + '(point star "foo") '("xxfoo" "xyfoo") "" "") + '("x*foo" . 1))) + (should (equal + (completion-pcm--merge-try + '(star point "foo") '("xxfoo" "xyfoo") "" "") + '("x*foo" . 2))) + ;; This is important if the wildcard is at the start of a component. + (should (equal + (completion-pcm-try-completion + "*/minibuf" + '("lisp/minibuffer.el" "src/minibuf.c") + nil 9) + '("*/minibuf" . 9)))) + (ert-deftest completion-substring-test-1 () ;; One third of a match! (should (equal -- 2.39.3 --=-=-=--