* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
@ 2023-01-01 10:47 Daniel Mendler
2023-01-01 11:16 ` Gregory Heytings
0 siblings, 1 reply; 39+ messages in thread
From: Daniel Mendler @ 2023-01-01 10:47 UTC (permalink / raw)
To: 60464
Follow up on bug#60021
1. M-x eshell
2. Enter `ls *.el` in a directory with Elisp files
3. Press TAB
Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p last)
(pcomplete-arg last)
(pcomplete-parse-arguments nil)
(pcomplete-completions)
(pcomplete-completions-at-point)
(#<subr completion--capf-wrapper> pcomplete-completions-at-point all)
(corfu--capf-wrapper-advice #<subr completion--capf-wrapper> pcomplete-completions-at-point all)
(apply corfu--capf-wrapper-advice #<subr completion--capf-wrapper> (pcomplete-completions-at-point all))
(completion--capf-wrapper pcomplete-completions-at-point all)
(completion-at-point)
(funcall-interactively completion-at-point)
(command-execute completion-at-point)
In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
version 1.16.0, Xaw scroll bars) of 2022-12-30 built on projects
Repository revision: d086cd6cf877c6ca7af6712f9b79b52dd0caa934
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 10:47 bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last Daniel Mendler
@ 2023-01-01 11:16 ` Gregory Heytings
2023-01-01 11:21 ` Daniel Mendler
0 siblings, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 11:16 UTC (permalink / raw)
To: Daniel Mendler; +Cc: 60464
>
> Follow up on bug#60021
>
> 1. M-x eshell
> 2. Enter `ls *.el` in a directory with Elisp files
> 3. Press TAB
>
Thanks for your bug report. It was not necessary to open another bug
report, I've seen your post in bug#60021 and am working on it. Note that
with 'ls *.el SPC TAB', which is something that is more likely to be used,
there is no error.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 11:16 ` Gregory Heytings
@ 2023-01-01 11:21 ` Daniel Mendler
2023-01-01 11:35 ` Gregory Heytings
0 siblings, 1 reply; 39+ messages in thread
From: Daniel Mendler @ 2023-01-01 11:21 UTC (permalink / raw)
To: Gregory Heytings; +Cc: 60464
On 1/1/23 12:16, Gregory Heytings wrote:
>
>>
>> Follow up on bug#60021
>>
>> 1. M-x eshell
>> 2. Enter `ls *.el` in a directory with Elisp files
>> 3. Press TAB
>>
>
> Thanks for your bug report. It was not necessary to open another bug
> report, I've seen your post in bug#60021 and am working on it. Note that
> with 'ls *.el SPC TAB', which is something that is more likely to be used,
> there is no error.
Okay, I wasn't sure if my other mail got lost. Anyway I wanted to attach
the additional stack trace. The problem occurs only if 'last is passed,
therefore the issue probably doesn't occur for `ls *.el SPC TAB`.
You may want to try my Corfu completion UI for testing, even if you
don't like popups for completion or auto completion. It is helpful for
debugging since the UI asks the completion table for completions very
often, in particular if one configures overly aggressive settings (not
recommended for real usage):
(setq corfu-auto-delay 0)
(setq corfu-auto-prefix 1)
(global-corfu-mode)
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 11:21 ` Daniel Mendler
@ 2023-01-01 11:35 ` Gregory Heytings
2023-01-01 11:53 ` Daniel Mendler
0 siblings, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 11:35 UTC (permalink / raw)
To: Daniel Mendler; +Cc: 60464
>
> Okay, I wasn't sure if my other mail got lost. Anyway I wanted to attach
> the additional stack trace. The problem occurs only if 'last is passed,
> therefore the issue probably doesn't occur for `ls *.el SPC TAB`.
>
It also occurs if 'first is passed, in fact. Try '$exec-path SPC TAB'.
>
> You may want to try my Corfu completion UI for testing, even if you
> don't like popups for completion or auto completion. It is helpful for
> debugging since the UI asks the completion table for completions very
> often, in particular if one configures overly aggressive settings (not
> recommended for real usage):
>
As I said in bug#59956 and bug#60021, I don't use Eshell, so I don't think
using another completion UI would have helped. That's why I asked for
feedback, Jim and you both tried the patch and did not report problems.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 11:35 ` Gregory Heytings
@ 2023-01-01 11:53 ` Daniel Mendler
2023-01-01 12:04 ` Gregory Heytings
0 siblings, 1 reply; 39+ messages in thread
From: Daniel Mendler @ 2023-01-01 11:53 UTC (permalink / raw)
To: Gregory Heytings; +Cc: 60464
On 1/1/23 12:35, Gregory Heytings wrote:
>> Okay, I wasn't sure if my other mail got lost. Anyway I wanted to attach
>> the additional stack trace. The problem occurs only if 'last is passed,
>> therefore the issue probably doesn't occur for `ls *.el SPC TAB`.
>>
>
> It also occurs if 'first is passed, in fact. Try '$exec-path SPC TAB'.
Of course.
>> You may want to try my Corfu completion UI for testing, even if you
>> don't like popups for completion or auto completion. It is helpful for
>> debugging since the UI asks the completion table for completions very
>> often, in particular if one configures overly aggressive settings (not
>> recommended for real usage):
>>
>
> As I said in bug#59956 and bug#60021, I don't use Eshell, so I don't think
> using another completion UI would have helped. That's why I asked for
> feedback, Jim and you both tried the patch and did not report problems.
Actually I tried the patch only for a short time and then forgot about
it when recompiling Emacs the next time. I looked into the issue again
when you closed the bug report.
If you don't use Eshell, maybe you use Shell? Pcomplete issues should
also occur there. But even if you try Eshell/Shell for only a short
time, using Corfu would definitely help in its aggressive settings.
There is no doubt about that since I only found these issues thanks to
Corfu. With default completion you simply don't press TAB after each
other key.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 11:53 ` Daniel Mendler
@ 2023-01-01 12:04 ` Gregory Heytings
2023-01-01 16:59 ` Gregory Heytings
0 siblings, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 12:04 UTC (permalink / raw)
To: Daniel Mendler; +Cc: 60464
>
> If you don't use Eshell, maybe you use Shell? Pcomplete issues should
> also occur there.
>
They don't, or at least the specific issue of this bug doesn't, AFAICS.
The pcomplete-parse-arguments-function in Shell is
shell--parse-pcomplete-arguments, which only splits the command line into
its whitespace-separated elements. IOW, that function only returns
strings. The problem with Eshell, in which the
pcomplete-parse-arguments-function is eshell-complete-parse-arguments, is
that it returns non-strings.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 12:04 ` Gregory Heytings
@ 2023-01-01 16:59 ` Gregory Heytings
2023-01-01 17:18 ` Daniel Mendler
` (3 more replies)
0 siblings, 4 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 16:59 UTC (permalink / raw)
To: Daniel Mendler, Jim Porter, Stefan Monnier, Eli Zaretskii; +Cc: 60464
[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]
After working a bit more on this bug, I concluded that what Stefan
initially suggested, to use the string representation of the value, is
safer than trying to extract the string corresponding to the argument that
the user typed in from the command line.
But that's not the end of the story. The problem is that, IIUC, in
Eshell, in a directory with .el files and without .EL files:
(1) ls *.el TAB should display all these files in *Completions*
(2) ls *.EL TAB should say "No match"
(3) ls *.el SPC TAB should display all files (not just the .el ones) in *Completions*
(4) ls *.EL SPC TAB should display all files (not just the .el ones) in *Completions*
IOW, sometimes pcomplete-arg should in fact return a list and not a single
string value, because that's what Eshell expects (case (1) above), and
sometimes it shouldn't. According to my tests, a non-string value can be
returned if and only if index is 'last'. Hopefully, this is also what
other users of pcomplete expect.
Can you please test the attached patch as extensively as possible, and
report if you see regressions?
Jim, can you also try the patch and report if you see regressions?
Stefan and Eli, does that patch look right to you?
Thanks.
[-- Attachment #2: Further-improvement-for-non-string-values-in-pcomple.patch --]
[-- Type: text/x-diff, Size: 2027 bytes --]
From ee2877018b1cdb93f11fffc89b9dd4ed461fcc96 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sun, 1 Jan 2023 16:47:36 +0000
Subject: [PATCH] Further improvement for non-string values in pcomplete
* lisp/pcomplete.el (pcomplete-arg): Use the string representation
of the argument value instead of the text representation of the
argument. Return the value, even when it is not a string, when
index is 'last'. Fixes bug#60464.
---
lisp/pcomplete.el | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 5bee515246..bce9aa5b4d 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -648,10 +648,10 @@ pcomplete-arg
accessing absolute argument positions.
When the argument has been transformed into something that is not
-a string by `pcomplete-parse-arguments-function', the text
-representation of the argument, namely what the user actually
-typed in, is returned, and the value of the argument is stored in
-the pcomplete-arg-value text property of that string."
+a string by `pcomplete-parse-arguments-function' and INDEX is not
+`last', the string representation of that value is returned, and
+the value is stored in the pcomplete-arg-value text property of
+that string."
(let ((arg
(nth (+ (pcase index
('first 0)
@@ -659,12 +659,10 @@ pcomplete-arg
(_ (- pcomplete-index (or index 0))))
(or offset 0))
pcomplete-args)))
- (if (stringp arg)
+ (if (or (stringp arg)
+ (eq index 'last))
arg
- (propertize
- (buffer-substring (pcomplete-begin index offset)
- (pcomplete-begin (1- (or index 0)) offset))
- 'pcomplete-arg-value arg))))
+ (propertize (format "%S" arg) 'pcomplete-arg-value arg))))
(defun pcomplete-begin (&optional index offset)
"Return the beginning position of the INDEXth argument.
--
2.39.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 16:59 ` Gregory Heytings
@ 2023-01-01 17:18 ` Daniel Mendler
2023-01-01 17:28 ` Gregory Heytings
2023-01-01 17:47 ` Eli Zaretskii
` (2 subsequent siblings)
3 siblings, 1 reply; 39+ messages in thread
From: Daniel Mendler @ 2023-01-01 17:18 UTC (permalink / raw)
To: Gregory Heytings, Jim Porter, Stefan Monnier, Eli Zaretskii; +Cc: 60464
On 1/1/23 17:59, Gregory Heytings wrote:
> After working a bit more on this bug, I concluded that what Stefan
> initially suggested, to use the string representation of the value, is
> safer than trying to extract the string corresponding to the argument that
> the user typed in from the command line.
This approach seems totally wrong to me. You now introduced a third
representation. pcomplete-arg returns the string representation (1) with
the value attached as second representation (2). For other scenarios it
returns the string argument itself from the command line (3).
Also the approach is pointless. Why would the caller of the function
want to get the string representation, given that the original value is
available as text property?
I suggest you go back to the way you implemented this before and return
the actual command line string with the value attached as text property.
I don't see why that should be less safe. It is just a matter of
determining the correct buffer boundaries.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:18 ` Daniel Mendler
@ 2023-01-01 17:28 ` Gregory Heytings
2023-01-01 17:34 ` Daniel Mendler
0 siblings, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 17:28 UTC (permalink / raw)
To: Daniel Mendler; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
>> After working a bit more on this bug, I concluded that what Stefan
>> initially suggested, to use the string representation of the value, is
>> safer than trying to extract the string corresponding to the argument
>> that the user typed in from the command line.
>
> This approach seems totally wrong to me.
>
Feel free to suggest something else.
>
> You now introduced a third representation.
>
No, there are two representations: pcomplete-arg returns a string
representation of the value, with that value attached to the string, when
its caller does not expect a non-string value.
>
> Also the approach is pointless. Why would the caller of the function
> want to get the string representation, given that the original value is
> available as text property?
>
Because the pcomplete functions, in particular pcomplete-here-using-help,
expect strings.
>
> I suggest you go back to the way you implemented this before and return
> the actual command line string with the value attached as text property.
> I don't see why that should be less safe. It is just a matter of
> determining the correct buffer boundaries.
>
Perhaps I should have explained what I mean by "less safe": it is
unexpectedly complex to compute the correct buffer boundaries in all
cases. If you don't believe me, try it yourself.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:28 ` Gregory Heytings
@ 2023-01-01 17:34 ` Daniel Mendler
2023-01-01 17:45 ` Gregory Heytings
0 siblings, 1 reply; 39+ messages in thread
From: Daniel Mendler @ 2023-01-01 17:34 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
On 1/1/23 18:28, Gregory Heytings wrote:
>
>>> After working a bit more on this bug, I concluded that what Stefan
>>> initially suggested, to use the string representation of the value, is
>>> safer than trying to extract the string corresponding to the argument
>>> that the user typed in from the command line.
>>
>> This approach seems totally wrong to me.
>>
> Feel free to suggest something else.
Yes, I did. The command line string should be returned.
>> Also the approach is pointless. Why would the caller of the function
>> want to get the string representation, given that the original value is
>> available as text property?
>>
> Because the pcomplete functions, in particular pcomplete-here-using-help,
> expect strings.
Of course. The only reason is to return a value of the correct type. But
you then as well return "foo".
>> I suggest you go back to the way you implemented this before and return
>> the actual command line string with the value attached as text property.
>> I don't see why that should be less safe. It is just a matter of
>> determining the correct buffer boundaries.
>
> Perhaps I should have explained what I mean by "less safe": it is
> unexpectedly complex to compute the correct buffer boundaries in all
> cases. If you don't believe me, try it yourself.
I believe you that doing this correctly is non trivial. But this doesn't
justify going with your hack.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:34 ` Daniel Mendler
@ 2023-01-01 17:45 ` Gregory Heytings
2023-01-01 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:26 ` Daniel Mendler
0 siblings, 2 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 17:45 UTC (permalink / raw)
To: Daniel Mendler; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
>> Feel free to suggest something else.
>
> Yes, I did. The command line string should be returned.
>
I mean, to propose some other code.
>
> I believe you that doing this correctly is non trivial. But this doesn't
> justify going with your hack.
>
It's extracting the correct part of the command line string that is hacky,
too hacky to my taste.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 16:59 ` Gregory Heytings
2023-01-01 17:18 ` Daniel Mendler
@ 2023-01-01 17:47 ` Eli Zaretskii
2023-01-01 18:29 ` Gregory Heytings
2023-01-01 17:57 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 5:51 ` Jim Porter
3 siblings, 1 reply; 39+ messages in thread
From: Eli Zaretskii @ 2023-01-01 17:47 UTC (permalink / raw)
To: Gregory Heytings; +Cc: mail, 60464, monnier, jporterbugs
> Date: Sun, 01 Jan 2023 16:59:37 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60464@debbugs.gnu.org
>
> Stefan and Eli, does that patch look right to you?
I don't consider myself an expert on pcomplete, sorry. I will defer
to Jim and Stefan. (Although I must say that I'm not sure I see a bug
here.)
However, I do have two questions:
. is this suggested for master or for the emacs-29 branch?
. if the latter, then which past version of Emacs handled this correctly?
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 16:59 ` Gregory Heytings
2023-01-01 17:18 ` Daniel Mendler
2023-01-01 17:47 ` Eli Zaretskii
@ 2023-01-01 17:57 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:19 ` Gregory Heytings
2023-01-04 5:51 ` Jim Porter
3 siblings, 1 reply; 39+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-01 17:57 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
> After working a bit more on this bug, I concluded that what Stefan initially
> suggested, to use the string representation of the value, is safer than
> trying to extract the string corresponding to the argument that the user
> typed in from the command line.
Could you expand on when/where it's "unsafe" or what it breaks?
Stefan
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:45 ` Gregory Heytings
@ 2023-01-01 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:26 ` Daniel Mendler
1 sibling, 0 replies; 39+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-01 18:02 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
>> I believe you that doing this correctly is non trivial. But this doesn't
>> justify going with your hack.
> It's extracting the correct part of the command line string that is hacky,
> too hacky to my taste.
I wouldn't be surprised if it's difficult/impossible, indeed.
But the question is: when/where does it matter whether we really return
the "correct" part?
In my experience, there are some fundamental mismatches in the Pcomplete
API which we can't fix without a serious redesign, so until we do that
we have to live with "best effort" :-(
Stefan
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:57 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-01 18:19 ` Gregory Heytings
2023-01-01 18:38 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 18:19 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
>> After working a bit more on this bug, I concluded that what Stefan
>> initially suggested, to use the string representation of the value, is
>> safer than trying to extract the string corresponding to the argument
>> that the user typed in from the command line.
>
> Could you expand on when/where it's "unsafe" or what it breaks?
>
I simply spent too much time trying to get the "extract the correct part
of the command line from the buffer" right, and wasn't able to convince
myself that the result was correct in all circumstances. Given that
returning (format "%S" arg) is what you initially suggested, and that it
cannot be wrong, I concluded that it was the best/safest thing to do.
The semantics of the "index" argument of the pcomplete-arg function are
tricky: it can be 0, "the current argument being examined", < 0, "closer
to the last argument", and > 0, "closer to the first argument". And then
you also have the special values 'first and 'last. And it can also be
nil, which is equivalent to 0.
There is a pcomplete-actual-arg function, which returns "the actual text
representation of the last argument" (in fact, "the actual text
representation of the INDEXth argument and the following ones"), but no
function which returns the actual text representation of a given argument.
Perhaps we could just use it and assume that all arguments are separated
by spaces, though, in which case the patch would become:
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 5bee515246..c829b6c3b7 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -648,10 +648,11 @@ pcomplete-arg
accessing absolute argument positions.
When the argument has been transformed into something that is not
-a string by `pcomplete-parse-arguments-function', the text
-representation of the argument, namely what the user actually
-typed in, is returned, and the value of the argument is stored in
-the pcomplete-arg-value text property of that string."
+a string by `pcomplete-parse-arguments-function' and INDEX is not
+`last', the text representation of the argument, namely what the
+user actually typed in, is returned, and the value of the
+argument is stored in the pcomplete-arg-value text property of
+that string."
(let ((arg
(nth (+ (pcase index
('first 0)
@@ -659,11 +660,11 @@ pcomplete-arg
(_ (- pcomplete-index (or index 0))))
(or offset 0))
pcomplete-args)))
- (if (stringp arg)
+ (if (or (stringp arg)
+ (eq index 'last))
arg
(propertize
- (buffer-substring (pcomplete-begin index offset)
- (pcomplete-begin (1- (or index 0)) offset))
+ (car (split-string (pcomplete-actual-arg index offset)))
'pcomplete-arg-value arg))))
(defun pcomplete-begin (&optional index offset)
^ permalink raw reply related [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:45 ` Gregory Heytings
2023-01-01 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-01 18:26 ` Daniel Mendler
1 sibling, 0 replies; 39+ messages in thread
From: Daniel Mendler @ 2023-01-01 18:26 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
Hi Gregory!
On 1/1/23 18:45, Gregory Heytings wrote:
>
>>> Feel free to suggest something else.
>>
>> Yes, I did. The command line string should be returned.
>>
>
> I mean, to propose some other code.
>
>>
>> I believe you that doing this correctly is non trivial. But this doesn't
>> justify going with your hack.
>>
>
> It's extracting the correct part of the command line string that is hacky,
> too hacky to my taste.
I only gave this a quick try and I don't make use of pcomplete-* helper
functions which may exist. This is just a best effort solution, which
assumes that we can split at spaces. If that's not the case we will get
wrong results, also in the presence of quotation.
But I believe that this is better than nothing and it should give a
sufficiently good result in most cases. We should acknowledge that
completion is not always perfect. It is okay to return a heuristical
result sometimes. If we observe bugs due to invalid return values we
could improve afterwards. I still consider this better than returning an
"arbitrary" string value as in your most recent proposal.
(defun pcomplete-arg (&optional index offset)
(let* ((idx (+ (pcase index
('first 0)
('last pcomplete-last)
(_ (- pcomplete-index (or index 0))))
(or offset 0)))
(arg (nth idx pcomplete-args)))
(if (stringp arg)
arg
(propertize
(buffer-substring-no-properties
(nth idx pcomplete-begins)
(save-excursion
(if (< idx pcomplete-last)
(goto-char (nth (1+ idx) pcomplete-begins))
(end-of-line))
(if (re-search-backward "\\S-" nil t)
(1+ (point))
(pos-eol))))
'pcomplete-arg-value arg))))
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 17:47 ` Eli Zaretskii
@ 2023-01-01 18:29 ` Gregory Heytings
2023-01-01 20:01 ` Eli Zaretskii
0 siblings, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 18:29 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: mail, 60464, monnier, jporterbugs
>> Stefan and Eli, does that patch look right to you?
>
> I don't consider myself an expert on pcomplete, sorry. I will defer to
> Jim and Stefan. (Although I must say that I'm not sure I see a bug
> here.)
>
> However, I do have two questions:
>
> . is this suggested for master or for the emacs-29 branch?
> . if the latter, then which past version of Emacs handled this correctly?
>
It's a sequel of dafa6d6bad, so it should probably go to emacs-29. I
can't really answer your second question with 100% accuracy, but I believe
Emacs 28 behaved correctly in the cases discussed in this bug and the
earlier ones (bug#59956 and bug#60021).
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 18:19 ` Gregory Heytings
@ 2023-01-01 18:38 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 19:13 ` Gregory Heytings
0 siblings, 2 replies; 39+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-01 18:38 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
> (let ((arg
> (nth (+ (pcase index
> ('first 0)
> @@ -659,11 +660,11 @@ pcomplete-arg
> (_ (- pcomplete-index (or index 0))))
> (or offset 0))
> pcomplete-args)))
> - (if (stringp arg)
> + (if (or (stringp arg)
> + (eq index 'last))
> arg
> (propertize
> - (buffer-substring (pcomplete-begin index offset)
> - (pcomplete-begin (1- (or index 0)) offset))
> + (car (split-string (pcomplete-actual-arg index offset)))
> 'pcomplete-arg-value arg))))
I'm not sure what specific problem this is trying to solve (is it the
choice of the "index" or is it the precise buffer positions of the
bounds)?
For the first, would the patch below help?
[ For the second, I suspect we can't provide 100% reliably correct
information anyway, so I think we'll need to find concrete usecases
where it matters before we can judge how much effort is warranted. ]
Stefan
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 2d3730e294a..815ad252fbd 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -1,6 +1,6 @@
;;; pcomplete.el --- programmable completion -*- lexical-binding: t -*-
-;; Copyright (C) 1999-2022 Free Software Foundation, Inc.
+;; Copyright (C) 1999-2023 Free Software Foundation, Inc.
;; Author: John Wiegley <johnw@gnu.org>
;; Keywords: processes abbrev
@@ -652,13 +652,12 @@ pcomplete-arg
representation of the argument, namely what the user actually
typed in, is returned, and the value of the argument is stored in
the pcomplete-arg-value text property of that string."
- (let ((arg
- (nth (+ (pcase index
+ (let* ((index (+ (pcase index
('first 0)
('last pcomplete-last)
(_ (- pcomplete-index (or index 0))))
- (or offset 0))
- pcomplete-args)))
+ (or offset 0)))
+ (arg (nth index pcomplete-args)))
(if (stringp arg)
arg
(propertize
^ permalink raw reply related [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 18:38 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-01 18:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 19:13 ` Gregory Heytings
1 sibling, 0 replies; 39+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-01 18:42 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
> For the first, would the patch below help?
Well, obviously not because I didn't bother to look at the rest of
the code.
Maybe this one would be closer, tho it probably needs some handling for
the boundary case where `index` is the last.
Stefan
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 2d3730e294a..36968d3b73c 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -1,6 +1,6 @@
;;; pcomplete.el --- programmable completion -*- lexical-binding: t -*-
-;; Copyright (C) 1999-2022 Free Software Foundation, Inc.
+;; Copyright (C) 1999-2023 Free Software Foundation, Inc.
;; Author: John Wiegley <johnw@gnu.org>
;; Keywords: processes abbrev
@@ -652,18 +652,17 @@ pcomplete-arg
representation of the argument, namely what the user actually
typed in, is returned, and the value of the argument is stored in
the pcomplete-arg-value text property of that string."
- (let ((arg
- (nth (+ (pcase index
+ (let* ((index (+ (pcase index
('first 0)
('last pcomplete-last)
(_ (- pcomplete-index (or index 0))))
- (or offset 0))
- pcomplete-args)))
+ (or offset 0)))
+ (arg (nth index pcomplete-args)))
(if (stringp arg)
arg
(propertize
- (buffer-substring (pcomplete-begin index offset)
- (pcomplete-begin (1- (or index 0)) offset))
+ (buffer-substring (nth index pcomplete-begins)
+ (nth (1+ index) pcomplete-begins))
'pcomplete-arg-value arg))))
(defun pcomplete-begin (&optional index offset)
^ permalink raw reply related [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 18:38 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-01 19:13 ` Gregory Heytings
1 sibling, 0 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-01 19:13 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
>> (_ (- pcomplete-index (or index 0))))
>> (or offset 0))
>> pcomplete-args)))
>> - (if (stringp arg)
>> + (if (or (stringp arg)
>> + (eq index 'last))
>> arg
>> (propertize
>> - (buffer-substring (pcomplete-begin index offset)
>> - (pcomplete-begin (1- (or index 0)) offset))
>> + (car (split-string (pcomplete-actual-arg index offset)))
>> 'pcomplete-arg-value arg))))
>
> I'm not sure what specific problem this is trying to solve (is it the
> choice of the "index" or is it the precise buffer positions of the
> bounds)?
>
I admit I don't understand your question. It tries to use
pcomplete-actual-arg, which uses buffer-substring, to get the text
representation of the argument. It's not guaranteed to work in all cases,
though, because it simply assumes that arguments are separated by spaces.
But it should be "good enough".
The (eq index 'last) means that when index is 'last we return the
argument, even when it is not a string (or more precisely: when it is a
list of strings, and the code assumes that a non-string arg is a list of
string), because that's in fact what the caller expects (or at least
that's what Eshell expects) in that case: that list is displayed in
*Completions*.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 18:29 ` Gregory Heytings
@ 2023-01-01 20:01 ` Eli Zaretskii
0 siblings, 0 replies; 39+ messages in thread
From: Eli Zaretskii @ 2023-01-01 20:01 UTC (permalink / raw)
To: Gregory Heytings; +Cc: mail, 60464, monnier, jporterbugs
> Date: Sun, 01 Jan 2023 18:29:53 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: mail@daniel-mendler.de, jporterbugs@gmail.com, monnier@iro.umontreal.ca,
> 60464@debbugs.gnu.org
>
>
> >> Stefan and Eli, does that patch look right to you?
> >
> > I don't consider myself an expert on pcomplete, sorry. I will defer to
> > Jim and Stefan. (Although I must say that I'm not sure I see a bug
> > here.)
> >
> > However, I do have two questions:
> >
> > . is this suggested for master or for the emacs-29 branch?
> > . if the latter, then which past version of Emacs handled this correctly?
> >
>
> It's a sequel of dafa6d6bad, so it should probably go to emacs-29. I
> can't really answer your second question with 100% accuracy, but I believe
> Emacs 28 behaved correctly in the cases discussed in this bug and the
> earlier ones (bug#59956 and bug#60021).
Thanks. In that case, I'll withhold my opinion until I see the final
variant of the proposed patch.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-01 16:59 ` Gregory Heytings
` (2 preceding siblings ...)
2023-01-01 17:57 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-04 5:51 ` Jim Porter
2023-01-04 13:48 ` Gregory Heytings
3 siblings, 1 reply; 39+ messages in thread
From: Jim Porter @ 2023-01-04 5:51 UTC (permalink / raw)
To: Gregory Heytings, Daniel Mendler, Stefan Monnier, Eli Zaretskii; +Cc: 60464
On 1/1/2023 8:59 AM, Gregory Heytings wrote:
> Jim, can you also try the patch and report if you see regressions?
Thanks. I tried this out for a bit, and I haven't been able to break it
yet (although I didn't notice the issue that spawned this bug# the last
time either, so it's possible I'm missing some problem this time around,
too).
I think it would be super-useful to collect some of these test cases for
Pcomplete-in-Eshell and write ERT tests for them. Then we can be more
confident that future changes to Pcomplete won't regress Eshell. I'll
look into doing that in the next week or two.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 5:51 ` Jim Porter
@ 2023-01-04 13:48 ` Gregory Heytings
2023-01-04 14:25 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 14:50 ` Eli Zaretskii
0 siblings, 2 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-04 13:48 UTC (permalink / raw)
To: Jim Porter; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Stefan Monnier
[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]
>> Jim, can you also try the patch and report if you see regressions?
>
> Thanks. I tried this out for a bit, and I haven't been able to break it
> yet (although I didn't notice the issue that spawned this bug# the last
> time either, so it's possible I'm missing some problem this time around,
> too).
>
Thanks for your confirmation.
Stefan, do you agree with the attached patch? Or would you like something
else?
Eli, are you okay with that patch?
The difference between the code before dafa6d6bad and the code with this
patch applied can be summarized as follows:
(defun pcomplete-arg (&optional index offset)
(let ((arg
(nth (+ (pcase index
('first 0)
('last pcomplete-last)
(_ (- pcomplete-index (or index 0))))
(or offset 0))
pcomplete-args)))
- arg
+ (if (or (stringp arg)
+ (eq index 'last))
+ arg
+ (propertize
+ (car (split-string (pcomplete-actual-arg index offset)))
+ 'pcomplete-arg-value arg))))
IOW, instead of unconditionally returning arg, we now sometimes (when arg
is not already a string and when index is not last) return the textual
representation of that argument (what the user typed in) with the value of
arg attached to it.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Further-improvement-for-non-string-values-in-pcomple.patch --]
[-- Type: text/x-diff; name=Further-improvement-for-non-string-values-in-pcomple.patch, Size: 3330 bytes --]
From e5e197a68a25315e93586db299757911862e74a8 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 4 Jan 2023 13:36:04 +0000
Subject: [PATCH] Further improvement for non-string values in pcomplete
* lisp/pcomplete.el (pcomplete-arg): Use the string representation
of the argument value instead of the text representation of the
argument. Return the value, even when it is not a string, when
index is 'last'. Fixes bug#60464.
(pcomplete-actual-arg): Move it before 'pcomplete-arg'.
---
lisp/pcomplete.el | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 5bee515246..73e14c562b 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -632,6 +632,13 @@ pcomplete-list
;;; Internal Functions:
;; argument handling
+(defsubst pcomplete-actual-arg (&optional index offset)
+ "Return the actual text representation of the last argument.
+This is different from `pcomplete-arg', which returns the textual value
+that the last argument evaluated to. This function returns what the
+user actually typed in."
+ (buffer-substring (pcomplete-begin index offset) (point)))
+
(defun pcomplete-arg (&optional index offset)
"Return the textual content of the INDEXth argument.
INDEX is based from the current processing position. If INDEX is
@@ -648,10 +655,11 @@ pcomplete-arg
accessing absolute argument positions.
When the argument has been transformed into something that is not
-a string by `pcomplete-parse-arguments-function', the text
-representation of the argument, namely what the user actually
-typed in, is returned, and the value of the argument is stored in
-the pcomplete-arg-value text property of that string."
+a string by `pcomplete-parse-arguments-function' and INDEX is not
+`last', the text representation of the argument, namely what the
+user actually typed in, is returned, and the value of the
+argument is stored in the pcomplete-arg-value text property of
+that string."
(let ((arg
(nth (+ (pcase index
('first 0)
@@ -659,11 +667,11 @@ pcomplete-arg
(_ (- pcomplete-index (or index 0))))
(or offset 0))
pcomplete-args)))
- (if (stringp arg)
+ (if (or (stringp arg)
+ (eq index 'last))
arg
(propertize
- (buffer-substring (pcomplete-begin index offset)
- (pcomplete-begin (1- (or index 0)) offset))
+ (car (split-string (pcomplete-actual-arg index offset)))
'pcomplete-arg-value arg))))
(defun pcomplete-begin (&optional index offset)
@@ -679,13 +687,6 @@ pcomplete-begin
(setq index (+ index offset)))
(nth index pcomplete-begins))
-(defsubst pcomplete-actual-arg (&optional index offset)
- "Return the actual text representation of the last argument.
-This is different from `pcomplete-arg', which returns the textual value
-that the last argument evaluated to. This function returns what the
-user actually typed in."
- (buffer-substring (pcomplete-begin index offset) (point)))
-
(defsubst pcomplete-next-arg ()
"Move the various pointers to the next argument."
(setq pcomplete-index (1+ pcomplete-index)
--
2.39.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 13:48 ` Gregory Heytings
@ 2023-01-04 14:25 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 14:35 ` Daniel Mendler
2023-01-04 14:36 ` Gregory Heytings
2023-01-04 14:50 ` Eli Zaretskii
1 sibling, 2 replies; 39+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-04 14:25 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Jim Porter, Eli Zaretskii, 60464, Daniel Mendler
> (defun pcomplete-arg (&optional index offset)
> (let ((arg
> (nth (+ (pcase index
> ('first 0)
> ('last pcomplete-last)
> (_ (- pcomplete-index (or index 0))))
> (or offset 0))
> pcomplete-args)))
> - arg
> + (if (or (stringp arg)
> + (eq index 'last))
> + arg
> + (propertize
> + (car (split-string (pcomplete-actual-arg index offset)))
> + 'pcomplete-arg-value arg))))
>
> IOW, instead of unconditionally returning arg, we now sometimes (when arg is
> not already a string and when index is not last) return the textual
> representation of that argument (what the user typed in) with the value of
> arg attached to it.
I don't understand the `last` condition.
Could you explain it (e.g. with a concrete example that breaks if you
don't include it)?
Stefan
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 14:25 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-04 14:35 ` Daniel Mendler
2023-01-04 14:46 ` Gregory Heytings
2023-01-04 14:36 ` Gregory Heytings
1 sibling, 1 reply; 39+ messages in thread
From: Daniel Mendler @ 2023-01-04 14:35 UTC (permalink / raw)
To: Stefan Monnier, Gregory Heytings; +Cc: Jim Porter, Eli Zaretskii, 60464
On 1/4/23 15:25, Stefan Monnier wrote:
>> (defun pcomplete-arg (&optional index offset)
>> (let ((arg
>> (nth (+ (pcase index
>> ('first 0)
>> ('last pcomplete-last)
>> (_ (- pcomplete-index (or index 0))))
>> (or offset 0))
>> pcomplete-args)))
>> - arg
>> + (if (or (stringp arg)
>> + (eq index 'last))
>> + arg
>> + (propertize
>> + (car (split-string (pcomplete-actual-arg index offset)))
>> + 'pcomplete-arg-value arg))))
>>
>> IOW, instead of unconditionally returning arg, we now sometimes (when arg is
>> not already a string and when index is not last) return the textual
>> representation of that argument (what the user typed in) with the value of
>> arg attached to it.
>
> I don't understand the `last` condition.
> Could you explain it (e.g. with a concrete example that breaks if you
> don't include it)?
This seems like its opening the door for yet another bug. Gregory, could
you please take a look at the version of pcomplete-arg that I've sent?
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 14:25 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 14:35 ` Daniel Mendler
@ 2023-01-04 14:36 ` Gregory Heytings
2023-01-04 15:19 ` Daniel Mendler
1 sibling, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-04 14:36 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Jim Porter, Eli Zaretskii, 60464, Daniel Mendler
>> IOW, instead of unconditionally returning arg, we now sometimes (when
>> arg is not already a string and when index is not last) return the
>> textual representation of that argument (what the user typed in) with
>> the value of arg attached to it.
>
> I don't understand the `last` condition.
>
> Could you explain it (e.g. with a concrete example that breaks if you
> don't include it)?
>
Yes: when index is 'last' returning a non-string (more precisely: a list
of strings) is allowed (and in fact expected, at least by Eshell). A
recipe, in the Emacs repository:
M-x eshell
cd lisp
ls *.el TAB
(Note that there is no SPC before TAB.)
Without the 'last' condition, you don't get what you're supposed to get at
that point (and what you got with Emacs 27 or 28), namely the list of .el
files in *Completions*. Instead you see "Complete, but not unique", and
you have to press TAB a second time to see the completions.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 14:35 ` Daniel Mendler
@ 2023-01-04 14:46 ` Gregory Heytings
2023-01-04 15:14 ` Daniel Mendler
2023-01-04 18:41 ` Jim Porter
0 siblings, 2 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-04 14:46 UTC (permalink / raw)
To: Daniel Mendler; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
>
> This seems like its opening the door for yet another bug.
>
That's always possible, of course. Jim tested it extensively. Can you do
the same?
>
> Gregory, could you please take a look at the version of pcomplete-arg
> that I've sent?
>
I did. It's not better (and in fact much more bug-prone) than reusing
pcomplete-actual-arg.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 13:48 ` Gregory Heytings
2023-01-04 14:25 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-04 14:50 ` Eli Zaretskii
1 sibling, 0 replies; 39+ messages in thread
From: Eli Zaretskii @ 2023-01-04 14:50 UTC (permalink / raw)
To: Gregory Heytings; +Cc: jporterbugs, 60464, monnier, mail
> Date: Wed, 04 Jan 2023 13:48:25 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Daniel Mendler <mail@daniel-mendler.de>,
> Stefan Monnier <monnier@iro.umontreal.ca>, Eli Zaretskii <eliz@gnu.org>,
> 60464@debbugs.gnu.org
>
> > Thanks. I tried this out for a bit, and I haven't been able to break it
> > yet (although I didn't notice the issue that spawned this bug# the last
> > time either, so it's possible I'm missing some problem this time around,
> > too).
> >
>
> Thanks for your confirmation.
>
> Stefan, do you agree with the attached patch? Or would you like something
> else?
>
> Eli, are you okay with that patch?
Yes, provided that Stefan is okay with it.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 14:46 ` Gregory Heytings
@ 2023-01-04 15:14 ` Daniel Mendler
2023-01-04 15:18 ` Eli Zaretskii
2023-01-04 15:39 ` Gregory Heytings
2023-01-04 18:41 ` Jim Porter
1 sibling, 2 replies; 39+ messages in thread
From: Daniel Mendler @ 2023-01-04 15:14 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
On 1/4/23 15:46, Gregory Heytings wrote:
>> Gregory, could you please take a look at the version of pcomplete-arg
>> that I've sent?
>
> I did. It's not better (and in fact much more bug-prone) than reusing
> pcomplete-actual-arg.
Maybe, but it works well for me. Your implementation with the 'last
condition doesn't make sense. It is not a good idea to treat 'last
specially such that we return a different type in this case. This is
more bug-prone for the caller.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:14 ` Daniel Mendler
@ 2023-01-04 15:18 ` Eli Zaretskii
2023-01-04 15:22 ` Daniel Mendler
2023-01-04 15:39 ` Gregory Heytings
1 sibling, 1 reply; 39+ messages in thread
From: Eli Zaretskii @ 2023-01-04 15:18 UTC (permalink / raw)
To: Daniel Mendler; +Cc: jporterbugs, gregory, 60464, monnier
> Date: Wed, 4 Jan 2023 16:14:14 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> Jim Porter <jporterbugs@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> 60464@debbugs.gnu.org
> From: Daniel Mendler <mail@daniel-mendler.de>
>
> Your implementation with the 'last condition doesn't make sense.
"Doesn't make sense" is a harsh judgment of someone's code, and I have
hard time believing Gregory's deserves that. Please try to be kinder
in how you express your disagreements with someone else's code.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 14:36 ` Gregory Heytings
@ 2023-01-04 15:19 ` Daniel Mendler
0 siblings, 0 replies; 39+ messages in thread
From: Daniel Mendler @ 2023-01-04 15:19 UTC (permalink / raw)
To: Gregory Heytings, Stefan Monnier; +Cc: Jim Porter, Eli Zaretskii, 60464
On 1/4/23 15:36, Gregory Heytings wrote:
>
>>> IOW, instead of unconditionally returning arg, we now sometimes (when
>>> arg is not already a string and when index is not last) return the
>>> textual representation of that argument (what the user typed in) with
>>> the value of arg attached to it.
>>
>> I don't understand the `last` condition.
>>
>> Could you explain it (e.g. with a concrete example that breaks if you
>> don't include it)?
>>
>
> Yes: when index is 'last' returning a non-string (more precisely: a list
> of strings) is allowed (and in fact expected, at least by Eshell). A
> recipe, in the Emacs repository:
>
> M-x eshell
> cd lisp
> ls *.el TAB
>
> (Note that there is no SPC before TAB.)
>
> Without the 'last' condition, you don't get what you're supposed to get at
> that point (and what you got with Emacs 27 or 28), namely the list of .el
> files in *Completions*. Instead you see "Complete, but not unique", and
> you have to press TAB a second time to see the completions.
Don't you think that this is by accident? Shouldn't Eshell fixed instead
to not expect a list in this case? I believe it would be better if
pcomplete-arg behaves uniformly for all arguments.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:18 ` Eli Zaretskii
@ 2023-01-04 15:22 ` Daniel Mendler
0 siblings, 0 replies; 39+ messages in thread
From: Daniel Mendler @ 2023-01-04 15:22 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: jporterbugs, gregory, 60464, monnier
On 1/4/23 16:18, Eli Zaretskii wrote:
>> Date: Wed, 4 Jan 2023 16:14:14 +0100
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>> Jim Porter <jporterbugs@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>> 60464@debbugs.gnu.org
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>> Your implementation with the 'last condition doesn't make sense.
>
> "Doesn't make sense" is a harsh judgment of someone's code, and I have
> hard time believing Gregory's deserves that. Please try to be kinder
> in how you express your disagreements with someone else's code.
I try to formulate it more fairly. In my opinion Gregory's patches so
far did not fix the issue in a way which is future-proof. The first
prematurely installed patch was obviously broken. The current proposal
introduces an unexpected special casing, where it would be better if the
function behaves uniformly for all arguments.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:14 ` Daniel Mendler
2023-01-04 15:18 ` Eli Zaretskii
@ 2023-01-04 15:39 ` Gregory Heytings
2023-01-04 15:48 ` Daniel Mendler
2023-01-04 15:56 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 2 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-04 15:39 UTC (permalink / raw)
To: Daniel Mendler; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
>
> Your implementation with the 'last condition doesn't make sense.
>
Can you please try to make it fail?
>
> It is not a good idea to treat 'last specially such that we return a
> different type in this case.
>
I'm not the one who wrote that code. According to my analysis of the
code, the only place where pcomplete-arg is called with a 'last' argument
is pcomplete-parse-arguments, where you will see the following:
(defun pcomplete-parse-arguments (&optional expand-p)
...
(let ((results (funcall pcomplete-parse-arguments-function)))
(when results
(setq ...
pcomplete-stub (pcomplete-arg 'last))
(let ...
(if (and (listp pcomplete-stub) ;??
(not pcomplete-expand-only-p))
;; If `pcomplete-stub' is a list, it means it's a list of
;; completions computed during parsing, e.g. Eshell uses
;; that to turn globs into lists of completions.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:39 ` Gregory Heytings
@ 2023-01-04 15:48 ` Daniel Mendler
2023-01-04 15:56 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 39+ messages in thread
From: Daniel Mendler @ 2023-01-04 15:48 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Jim Porter, Eli Zaretskii, 60464, Stefan Monnier
On 1/4/23 16:39, Gregory Heytings wrote:
>> Your implementation with the 'last condition doesn't make sense.
>
> Can you please try to make it fail?
That was not my point. The way the function behaves now is problematic
on theoretical grounds.
>> It is not a good idea to treat 'last specially such that we return a
>> different type in this case.
>>
>
> I'm not the one who wrote that code. According to my analysis of the
> code, the only place where pcomplete-arg is called with a 'last' argument
> is pcomplete-parse-arguments, where you will see the following:
I am not blaming you for it. If your analysis is right (and there are
not many or even only a single caller), what about deprecating support
for 'last and 'first then, if these lead to problems? Stefan said before
that Pcomplete needs some refactoring and fixing so one could do it in
small steps. We could then fix the few call sites which are there. But
such changes may be too late for Emacs 29.
Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:39 ` Gregory Heytings
2023-01-04 15:48 ` Daniel Mendler
@ 2023-01-04 15:56 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 16:08 ` Gregory Heytings
2023-01-04 18:49 ` Jim Porter
1 sibling, 2 replies; 39+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-04 15:56 UTC (permalink / raw)
To: Gregory Heytings; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
Gregory Heytings [2023-01-04 15:39:26] wrote:
>> Your implementation with the 'last condition doesn't make sense.
> Can you please try to make it fail?
>> It is not a good idea to treat 'last specially such that we return a
>> different type in this case.
> I'm not the one who wrote that code. According to my analysis of the code,
> the only place where pcomplete-arg is called with a 'last' argument is
> pcomplete-parse-arguments, where you will see the following:
>
> (defun pcomplete-parse-arguments (&optional expand-p)
> ...
> (let ((results (funcall pcomplete-parse-arguments-function)))
> (when results
> (setq ...
> pcomplete-stub (pcomplete-arg 'last))
> (let ...
> (if (and (listp pcomplete-stub) ;??
> (not pcomplete-expand-only-p))
> ;; If `pcomplete-stub' is a list, it means it's a list of
> ;; completions computed during parsing, e.g. Eshell uses
> ;; that to turn globs into lists of completions.
That's also my understanding. So I think The Right Fix (or at least The
Better Fix) is to pay no special attention to `last` in `pcomplete-arg`
and instead in the above code of `pcomplete-parse-arguments` to look for
the `pcomplete-arg-value` property.
Maybe for `emacs-29` we can use your patch (with a comment about why
`last` is handled specially pointing to its handling in
`pcomplete-parse-arguments`) and then in `master` we remove this special
handling of `last`?
Stefan
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:56 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-01-04 16:08 ` Gregory Heytings
2023-01-14 21:27 ` Gregory Heytings
2023-01-04 18:49 ` Jim Porter
1 sibling, 1 reply; 39+ messages in thread
From: Gregory Heytings @ 2023-01-04 16:08 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
>> I'm not the one who wrote that code. According to my analysis of the
>> code, the only place where pcomplete-arg is called with a 'last'
>> argument is pcomplete-parse-arguments, where you will see the
>> following:
>>
>> (defun pcomplete-parse-arguments (&optional expand-p)
>> ...
>> (let ((results (funcall pcomplete-parse-arguments-function)))
>> (when results
>> (setq ...
>> pcomplete-stub (pcomplete-arg 'last))
>> (let ...
>> (if (and (listp pcomplete-stub) ;??
>> (not pcomplete-expand-only-p))
>> ;; If `pcomplete-stub' is a list, it means it's a list of
>> ;; completions computed during parsing, e.g. Eshell uses
>> ;; that to turn globs into lists of completions.
>
> That's also my understanding. So I think The Right Fix (or at least The
> Better Fix) is to pay no special attention to `last` in `pcomplete-arg`
> and instead in the above code of `pcomplete-parse-arguments` to look for
> the `pcomplete-arg-value` property.
>
That would be even better, indeed. But it would be a larger change.
>
> Maybe for `emacs-29` we can use your patch (with a comment about why
> `last` is handled specially pointing to its handling in
> `pcomplete-parse-arguments`) and then in `master` we remove this special
> handling of `last`?
>
I'm fine with that.
Unless someone objects, I'll push that patch in a day or two, and I'll do
that change on master afterwards.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 14:46 ` Gregory Heytings
2023-01-04 15:14 ` Daniel Mendler
@ 2023-01-04 18:41 ` Jim Porter
1 sibling, 0 replies; 39+ messages in thread
From: Jim Porter @ 2023-01-04 18:41 UTC (permalink / raw)
To: Gregory Heytings, Daniel Mendler; +Cc: Eli Zaretskii, 60464, Stefan Monnier
On 1/4/2023 6:46 AM, Gregory Heytings wrote:
>
>>
>> This seems like its opening the door for yet another bug.
>>
>
> That's always possible, of course. Jim tested it extensively. Can you
> do the same?
Well, let's not say "extensively". :) I tested the small set of things I
could think of that would be likely to break here. However, since I know
very little about Pcomplete, my testing was mostly just "try a few
different Eshell constructs and make sure no errors get signaled".
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 15:56 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 16:08 ` Gregory Heytings
@ 2023-01-04 18:49 ` Jim Porter
1 sibling, 0 replies; 39+ messages in thread
From: Jim Porter @ 2023-01-04 18:49 UTC (permalink / raw)
To: Stefan Monnier, Gregory Heytings; +Cc: Daniel Mendler, Eli Zaretskii, 60464
On 1/4/2023 7:56 AM, Stefan Monnier via Bug reports for GNU Emacs, the
Swiss army knife of text editors wrote:
> Maybe for `emacs-29` we can use your patch (with a comment about why
> `last` is handled specially pointing to its handling in
> `pcomplete-parse-arguments`) and then in `master` we remove this special
> handling of `last`?
I'd definitely support a minimal fix for the 29 branch, and then we can
come up with something more thorough for master. It's possible (maybe
even likely) that we need to make some changes on the Eshell side too.
Once I get some ERT tests written up for Pcomplete-from-Eshell, it
should hopefully make it easier to try out new implementations and make
sure it doesn't regress anything.
^ permalink raw reply [flat|nested] 39+ messages in thread
* bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last
2023-01-04 16:08 ` Gregory Heytings
@ 2023-01-14 21:27 ` Gregory Heytings
0 siblings, 0 replies; 39+ messages in thread
From: Gregory Heytings @ 2023-01-14 21:27 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Daniel Mendler, Eli Zaretskii, 60464, Jim Porter
>
> Unless someone objects, I'll push that patch in a day or two
>
Now done (72c45fa910).
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2023-01-14 21:27 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-01 10:47 bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last Daniel Mendler
2023-01-01 11:16 ` Gregory Heytings
2023-01-01 11:21 ` Daniel Mendler
2023-01-01 11:35 ` Gregory Heytings
2023-01-01 11:53 ` Daniel Mendler
2023-01-01 12:04 ` Gregory Heytings
2023-01-01 16:59 ` Gregory Heytings
2023-01-01 17:18 ` Daniel Mendler
2023-01-01 17:28 ` Gregory Heytings
2023-01-01 17:34 ` Daniel Mendler
2023-01-01 17:45 ` Gregory Heytings
2023-01-01 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:26 ` Daniel Mendler
2023-01-01 17:47 ` Eli Zaretskii
2023-01-01 18:29 ` Gregory Heytings
2023-01-01 20:01 ` Eli Zaretskii
2023-01-01 17:57 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:19 ` Gregory Heytings
2023-01-01 18:38 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 18:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-01 19:13 ` Gregory Heytings
2023-01-04 5:51 ` Jim Porter
2023-01-04 13:48 ` Gregory Heytings
2023-01-04 14:25 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 14:35 ` Daniel Mendler
2023-01-04 14:46 ` Gregory Heytings
2023-01-04 15:14 ` Daniel Mendler
2023-01-04 15:18 ` Eli Zaretskii
2023-01-04 15:22 ` Daniel Mendler
2023-01-04 15:39 ` Gregory Heytings
2023-01-04 15:48 ` Daniel Mendler
2023-01-04 15:56 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-01-04 16:08 ` Gregory Heytings
2023-01-14 21:27 ` Gregory Heytings
2023-01-04 18:49 ` Jim Porter
2023-01-04 18:41 ` Jim Porter
2023-01-04 14:36 ` Gregory Heytings
2023-01-04 15:19 ` Daniel Mendler
2023-01-04 14:50 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).