GNU bug report logs - #60845
30.0.50; [PATCH] Add tests for Eshell interactive completion (and fix a bug in it)

Previous Next

Package: emacs;

Reported by: Jim Porter <jporterbugs <at> gmail.com>

Date: Mon, 16 Jan 2023 01:51:02 UTC

Severity: normal

Tags: patch

Found in version 30.0.50

Done: Jim Porter <jporterbugs <at> gmail.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60845 in the body.
You can then email your comments to 60845 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Mon, 16 Jan 2023 01:51:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Jim Porter <jporterbugs <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Mon, 16 Jan 2023 01:51:02 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Add tests for Eshell interactive completion (and fix
 a bug in it)
Date: Sun, 15 Jan 2023 17:50:10 -0800
[Message part 1 (text/plain, inline)]
This is a followup from bug#60464 and friends. Over there, we discussed 
problems with Pcomplete when used from Eshell, namely that Eshell 
sometimes gives Pcomplete non-string arguments. I mentioned that we 
should probably have automated tests for the Eshell side so that we can 
make further improvements to Pcomplete without causing regressions, so 
here are some tests.

I also fixed an edge case in 'eshell-complete-parse-arguments' where it 
wasn't correctly handling the new variable-splicing syntax in Eshell. 
That's patch 0002.

Of course, these tests are just a start, and there are probably lots of 
others that we could add.
[0001-Add-regression-tests-for-Eshell-completions.patch (text/plain, attachment)]
[0002-Properly-parse-Eshell-variable-splices-for-interacti.patch (text/plain, attachment)]

Information forwarded to gregory <at> heytings.org, bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Sun, 22 Jan 2023 21:35:02 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 60845 <at> debbugs.gnu.org
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Sun, 22 Jan 2023 13:34:44 -0800
X-Debbugs-CC: gregory <at> heytings.org

On 1/15/2023 5:50 PM, Jim Porter wrote:
> This is a followup from bug#60464 and friends. Over there, we discussed 
> problems with Pcomplete when used from Eshell, namely that Eshell 
> sometimes gives Pcomplete non-string arguments. I mentioned that we 
> should probably have automated tests for the Eshell side so that we can 
> make further improvements to Pcomplete without causing regressions, so 
> here are some tests.

After studying how Eshell interfaces with Pcomplete, I think I've 
convinced myself that Eshell should be the one responsible for 
converting all its arguments to strings. It already does this for some 
cases (nil becomes "", numbers are stringified, and ".../" forms become 
"../../"), so the fact that it doesn't do this universally is probably a 
bug.

The third patch in this series fixes this for the Eshell side, so we 
could probably remove the workaround in pcomplete.el for Emacs 30.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Sun, 22 Jan 2023 21:36:01 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 60845 <at> debbugs.gnu.org
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Sun, 22 Jan 2023 13:35:33 -0800
[Message part 1 (text/plain, inline)]
On 1/22/2023 1:34 PM, Jim Porter wrote:
> The third patch in this series fixes this for the Eshell side, so we 
> could probably remove the workaround in pcomplete.el for Emacs 30.

... It would help if I actually attached the patches.
[0001-Add-regression-tests-for-Eshell-completions.patch (text/plain, attachment)]
[0002-Properly-parse-Eshell-variable-splices-for-interacti.patch (text/plain, attachment)]
[0003-During-completion-convert-all-Eshell-arguments-to-st.patch (text/plain, attachment)]

Information forwarded to monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Mon, 30 Jan 2023 07:15:02 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 60845 <at> debbugs.gnu.org
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Sun, 29 Jan 2023 23:14:49 -0800
X-Debbugs-Cc: monnier <at> iro.umontreal.ca

On 1/22/2023 1:35 PM, Jim Porter wrote:
> On 1/22/2023 1:34 PM, Jim Porter wrote:
>> The third patch in this series fixes this for the Eshell side, so we 
>> could probably remove the workaround in pcomplete.el for Emacs 30.

Stefan, do you have any thoughts on these changes, especially the third 
patch? I did a bit of tinkering with pcomplete functions, and I think it 
should be ok for the Eshell side of things. Eshell-specific pcomplete 
functions can get the real value if they need by examining the 
'pcomplete-arg-value' string property.

For pcomplete functions that aren't Eshell-specific, they'll always get 
the string form of arguments by default, so things should Just Work there.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Mon, 30 Jan 2023 14:55:02 GMT) Full text and rfc822 format available.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60845 <at> debbugs.gnu.org
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Mon, 30 Jan 2023 09:54:09 -0500
>>> The third patch in this series fixes this for the Eshell side, so we
>>> could probably remove the workaround in pcomplete.el for Emacs 30.
>
> Stefan, do you have any thoughts on these changes, especially the third
> patch? I did a bit of tinkering with pcomplete functions, and I think it
> should be ok for the Eshell side of things. Eshell-specific pcomplete
> functions can get the real value if they need by examining the
> 'pcomplete-arg-value' string property.
>
> For pcomplete functions that aren't Eshell-specific, they'll always get the
> string form of arguments by default, so things should Just Work there.

It sounds good to me, but I'm definitely not well versed in this aspect
of the interaction between Eshell and Pcomplete (more specifically,
this is a part of their interaction which I find quite tricky), so it's
good that you add corresponding regression tests.


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Tue, 31 Jan 2023 02:01:02 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 60845 <at> debbugs.gnu.org
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Mon, 30 Jan 2023 18:00:33 -0800
On 1/30/2023 6:54 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> It sounds good to me, but I'm definitely not well versed in this aspect
> of the interaction between Eshell and Pcomplete (more specifically,
> this is a part of their interaction which I find quite tricky), so it's
> good that you add corresponding regression tests.

Thanks for taking a look. I've merged my patches as e7d0aa248e. We can 
leave this open though to discuss what to do about the Pcomplete side of 
things. I think we can remove the workaround for Emacs 29, but maybe we 
want some additional changes.

(It would also be nice to add some more Pcomplete support on the Eshell 
side. For example, when completing arguments to 'echo', Pcomplete shows 
options for the real /bin/echo, instead of showing options for the 
built-in function 'eshell/echo'.)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Tue, 05 Sep 2023 23:38:02 GMT) Full text and rfc822 format available.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60845 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Tue, 5 Sep 2023 16:36:59 -0700
Jim Porter <jporterbugs <at> gmail.com> writes:

> On 1/30/2023 6:54 AM, Stefan Monnier via Bug reports for GNU Emacs, the Swiss
> army knife of text editors wrote:
>> It sounds good to me, but I'm definitely not well versed in this aspect
>> of the interaction between Eshell and Pcomplete (more specifically,
>> this is a part of their interaction which I find quite tricky), so it's
>> good that you add corresponding regression tests.
>
> Thanks for taking a look. I've merged my patches as e7d0aa248e. We can leave
> this open though to discuss what to do about the Pcomplete side of things. I
> think we can remove the workaround for Emacs 29, but maybe we want some
> additional changes.

That was 9 months ago.  Is it still relevant to keep this bug open?

> (It would also be nice to add some more Pcomplete support on the Eshell
> side. For example, when completing arguments to 'echo', Pcomplete shows options
> for the real /bin/echo, instead of showing options for the built-in function
> 'eshell/echo'.)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Wed, 06 Sep 2023 00:48:01 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 60845 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Tue, 5 Sep 2023 17:47:06 -0700
On 9/5/2023 4:36 PM, Stefan Kangas wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> On 1/30/2023 6:54 AM, Stefan Monnier via Bug reports for GNU Emacs, the Swiss
>> army knife of text editors wrote:
>>> It sounds good to me, but I'm definitely not well versed in this aspect
>>> of the interaction between Eshell and Pcomplete (more specifically,
>>> this is a part of their interaction which I find quite tricky), so it's
>>> good that you add corresponding regression tests.
>>
>> Thanks for taking a look. I've merged my patches as e7d0aa248e. We can leave
>> this open though to discuss what to do about the Pcomplete side of things. I
>> think we can remove the workaround for Emacs 29, but maybe we want some
>> additional changes.
> 
> That was 9 months ago.  Is it still relevant to keep this bug open?

Yes, I believe so. I was planning to wait until Emacs 29.1 was released 
before pinging people on this, but then forgot all about it. We should 
probably use this time to fix the FIXME in 'pcomplete-arg', since (I 
think) the current behavior in Eshell no longer requires the FIXME bit:

            ;; FIXME: 'last' is handled specially in Emacs 29, because
            ;; 'pcomplete-parse-arguments' accepts a list of strings
            ;; (which are completion candidates) as return value for
            ;; (pcomplete-arg 'last).  See below: "it means it's a
            ;; list of completions computed during parsing,
            ;; e.g. Eshell uses that to turn globs into lists of
            ;; completions".  This special case will be dealt with
            ;; differently in Emacs 30: the pcomplete-arg-value
            ;; property will be used by 'pcomplete-parse-arguments'.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Wed, 06 Sep 2023 01:38:01 GMT) Full text and rfc822 format available.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 60845 <at> debbugs.gnu.org, Daniel Mendler <mail <at> daniel-mendler.de>,
 Gregory Heytings <gregory <at> heytings.org>, arstoffel <at> gmail.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Tue, 5 Sep 2023 18:37:47 -0700
[Message part 1 (text/plain, inline)]
On 9/5/2023 5:47 PM, Jim Porter wrote:
> On 9/5/2023 4:36 PM, Stefan Kangas wrote:
>> Jim Porter <jporterbugs <at> gmail.com> writes:
>>
>>> On 1/30/2023 6:54 AM, Stefan Monnier via Bug reports for GNU Emacs, 
>>> the Swiss
>>> army knife of text editors wrote:
>>>> It sounds good to me, but I'm definitely not well versed in this aspect
>>>> of the interaction between Eshell and Pcomplete (more specifically,
>>>> this is a part of their interaction which I find quite tricky), so it's
>>>> good that you add corresponding regression tests.
>>>
>>> Thanks for taking a look. I've merged my patches as e7d0aa248e. We 
>>> can leave
>>> this open though to discuss what to do about the Pcomplete side of 
>>> things. I
>>> think we can remove the workaround for Emacs 29, but maybe we want some
>>> additional changes.
>>
>> That was 9 months ago.  Is it still relevant to keep this bug open?
> 
> Yes, I believe so. I was planning to wait until Emacs 29.1 was released 
> before pinging people on this, but then forgot all about it. We should 
> probably use this time to fix the FIXME in 'pcomplete-arg', since (I 
> think) the current behavior in Eshell no longer requires the FIXME bit:
> 
>              ;; FIXME: 'last' is handled specially in Emacs 29, because
>              ;; 'pcomplete-parse-arguments' accepts a list of strings
>              ;; (which are completion candidates) as return value for
>              ;; (pcomplete-arg 'last).  See below: "it means it's a
>              ;; list of completions computed during parsing,
>              ;; e.g. Eshell uses that to turn globs into lists of
>              ;; completions".  This special case will be dealt with
>              ;; differently in Emacs 30: the pcomplete-arg-value
>              ;; property will be used by 'pcomplete-parse-arguments'.

Attached is a patch to revert the Emacs 29 workarounds. I *believe* I've 
fixed this on the Eshell side by always providing Pcomplete with the 
arguments in their string form. Could everyone try the patch out to make 
sure things still work?

In particular, see the cases in the following bugs: bug#60464, 
bug#60021, and bug#59956.
[0001-Revert-commits-dafa6d6badd6-and-72c45fa9109a.patch (text/plain, attachment)]

Reply sent to Jim Porter <jporterbugs <at> gmail.com>:
You have taken responsibility. (Tue, 10 Oct 2023 20:09:02 GMT) Full text and rfc822 format available.

Notification sent to Jim Porter <jporterbugs <at> gmail.com>:
bug acknowledged by developer. (Tue, 10 Oct 2023 20:09:02 GMT) Full text and rfc822 format available.

Message #34 received at 60845-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 60845-done <at> debbugs.gnu.org, Daniel Mendler <mail <at> daniel-mendler.de>,
 Gregory Heytings <gregory <at> heytings.org>, arstoffel <at> gmail.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Tue, 10 Oct 2023 13:07:50 -0700
On 9/5/2023 6:37 PM, Jim Porter wrote:
> Attached is a patch to revert the Emacs 29 workarounds. I *believe* I've 
> fixed this on the Eshell side by always providing Pcomplete with the 
> arguments in their string form. Could everyone try the patch out to make 
> sure things still work?
> 
> In particular, see the cases in the following bugs: bug#60464, 
> bug#60021, and bug#59956.

Since there have been no further comments in the last month, I've now 
merged my patch to the master branch as 239db5d5162. If anyone sees any 
issues resulting from this, feel free to back it out, file a bug, and/or 
let me know directly.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60845; Package emacs. (Tue, 10 Oct 2023 21:27:01 GMT) Full text and rfc822 format available.

Message #37 received at 60845-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60845-done <at> debbugs.gnu.org, Daniel Mendler <mail <at> daniel-mendler.de>,
 Gregory Heytings <gregory <at> heytings.org>, arstoffel <at> gmail.com,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive
 completion (and fix a bug in it)
Date: Tue, 10 Oct 2023 17:23:42 -0400
>> Attached is a patch to revert the Emacs 29 workarounds. I *believe* I've
>> fixed this on the Eshell side by always providing Pcomplete with the
>> arguments in their string form. Could everyone try the patch out to make
>> sure things still work?
>> In particular, see the cases in the following bugs: bug#60464, bug#60021,
>> and bug#59956.
>
> Since there have been no further comments in the last month, I've now merged
> my patch to the master branch as 239db5d5162. If anyone sees any issues
> resulting from this, feel free to back it out, file a bug, and/or let me
> know directly.

Thanks Jim,


        Stefan





bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 08 Nov 2023 12:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 141 days ago.

Previous Next


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