GNU bug report logs -
#52999
29.0.50; [PATCH] `eshell-eval-using-options' should follow POSIX/GNU argument conventions
Previous Next
Reported by: Jim Porter <jporterbugs <at> gmail.com>
Date: Tue, 4 Jan 2022 01:37:02 UTC
Severity: normal
Tags: patch
Found in version 29.0.50
Done: Eli Zaretskii <eliz <at> gnu.org>
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 52999 in the body.
You can then email your comments to 52999 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Tue, 04 Jan 2022 01:37: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
.
(Tue, 04 Jan 2022 01:37:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Currently, `eshell-eval-using-options' doesn't follow POSIX/GNU argument
conventions[1], resulting in some confusing behavior. To see this in
action, the easiest way is probably to make a small patch to
`eshell-do-ls' in lisp/eshell/em-ls.el: just comment out the line that says,
:external "ls"
and then eval the function again. (This is necessary so that eshell
doesn't fall back to the system's `ls' command when it gets confused.)
Then open `eshell' and run:
ls '-I*.el'
Instead of what you'd expect (a directory listing that ignores Emacs
Lisp files), instead you get a directory listing of *all* the files in
the long listing format. That's because `eshell-eval-using-options'
assumes that all the characters after the "-" are names of short
options, rather than a single short option followed by its value. You
can see a similar problem with:
ls '--ignore=*.el'
In this case, `eshell-eval-using-options' looks for an option named
"ignore=*.el" instead of an option named "ignore" followed by its value.
I've attached a patch with tests to fix this and use the POSIX/GNU
argument conventions, supporting both the above cases. However, I should
mention that this is a slightly incompatible change. A small number of
existing eshell commands work like `ls -I', and their behavior is now a
bit different. Previously, you could do the following,
ls -Ia '*.el'
to list all the files in a directory, excluding Emacs Lisp files. Now,
you have to spell that as:
ls -aI '*.el'
# or...
ls -aI'*.el'
I think that's ok though, since I can't imagine anyone *wanting* the old
behavior. It surprised me quite a bit when I stumbled across it, and
worse, it only crops up sometimes, since eshell transparently falls back
to the real commands when it gets confused.
For completeness, the following commands+options are affected:
sudo -u
du -d
ls -I
[1] https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html
[0001-Follow-POSIX-GNU-argument-conventions-for-eshell-eva.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Tue, 04 Jan 2022 05:34:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 52999 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 1/3/2022 5:36 PM, Jim Porter wrote:
> I've attached a patch with tests to fix this and use the POSIX/GNU
> argument conventions, supporting both the above cases.
One small addition here. I just noticed that `eshell-eval-using-options'
already supports "--" to terminate all the options, so I added a test
case for that too.
[0001-Follow-POSIX-GNU-argument-conventions-for-eshell-eva.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Tue, 04 Jan 2022 13:02:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 52999 <at> debbugs.gnu.org (full text, mbox):
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Mon, 3 Jan 2022 21:33:28 -0800
>
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1060,6 +1060,9 @@ dimensions.
> Specifying a cons as the from argument allows to start measuring text
> from a specified amount of pixels above or below a position.
>
> +---
> +** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
This is too terse: we cannot assume that everyone knows what does
"POSIX/GNU argument syntax conventions" stand for. Especially since
you didn't even say "command-line arguments", just "arguments".
Please make the entry more informative.
And why don't people who propose and install changes in Eshell also
update the Eshell manual? That the manual in its current shape leaves
a lot to be desired is not a justification to leave it that way. We
will never improve that manual unless we start adding useful stuff to
it, one small piece at a time.
> (ert-deftest test-eshell-eval-using-options ()
> "Tests for `eshell-eval-using-options'."
> + ;; Test short options.
> (eshell-eval-using-options
> - "sudo" '("-u" "root" "whoami")
> - '((?u "user" t user "execute a command as another USER")
> - :parse-leading-options-only)
> - (should (equal user "root")))
> + "ls" '("-a" "/dev/null")
> + '((?a "all" nil show-all
> + "do not ignore entries starting with ."))
> + (should (eq show-all t))
> + (should (equal args '("/dev/null"))))
Can these tests be made less platform-specific? For example, not all
the supported platforms have /dev/null, and we have a portable
abstraction for it.
> + "sudo" '("-u" "root" "whoami")
> + '((?u "user" t user "execute a command as another USER")
> + :parse-leading-options-only)
> + (should (equal user "root"))
> + (should (equal args '("whoami"))))
> + (eshell-eval-using-options
> + "sudo" '("--user" "root" "whoami")
> + '((?u "user" t user "execute a command as another USER")
> + :parse-leading-options-only)
> + (should (equal user "root"))
> + (should (equal args '("whoami"))))
> + (eshell-eval-using-options
> + "sudo" '("emerge" "-uDN" "world")
> + '((?u "user" t user "execute a command as another USER"))
> + (should (equal user "DN"))
> + (should (equal args '("emerge" "world"))))
> + (eshell-eval-using-options
> + "sudo" '("emerge" "-uDN" "world")
> + '((?u "user" t user "execute a command as another USER")
> + :parse-leading-options-only)
> + (should (eq user nil))
> + (should (equal args '("emerge" "-uDN" "world")))))
And here, sudo and whoami don't necessarily exist, so something should
be done about that, I think.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Tue, 04 Jan 2022 21:10:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 52999 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Thanks, I've attached an updated patch.
On 1/4/2022 5:01 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Mon, 3 Jan 2022 21:33:28 -0800
>>
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -1060,6 +1060,9 @@ dimensions.
>> Specifying a cons as the from argument allows to start measuring text
>> from a specified amount of pixels above or below a position.
>>
>> +---
>> +** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
>
> This is too terse: we cannot assume that everyone knows what does
> "POSIX/GNU argument syntax conventions" stand for. Especially since
> you didn't even say "command-line arguments", just "arguments".
> Please make the entry more informative.
Ok, I added a brief description explaining what specifically has been
changed.
> And why don't people who propose and install changes in Eshell also
> update the Eshell manual? That the manual in its current shape leaves
> a lot to be desired is not a justification to leave it that way. We
> will never improve that manual unless we start adding useful stuff to
> it, one small piece at a time.
I just wasn't sure if `eshell-eval-using-options' should be in the
manual or not. Thinking it over a bit more, it would have helped me if
it had been in the manual (I encountered this bug while trying to write
my own Eshell built-in command), so I added some info about it to the
manual, mostly adapted from the docstring for
`eshell-eval-using-options'. Hopefully I followed the right conventions
here; I'm only vaguely familiar with the Texinfo format.
>> (ert-deftest test-eshell-eval-using-options ()
>> "Tests for `eshell-eval-using-options'."
>> + ;; Test short options.
>> (eshell-eval-using-options
>> - "sudo" '("-u" "root" "whoami")
>> - '((?u "user" t user "execute a command as another USER")
>> - :parse-leading-options-only)
>> - (should (equal user "root")))
>> + "ls" '("-a" "/dev/null")
>> + '((?a "all" nil show-all
>> + "do not ignore entries starting with ."))
>> + (should (eq show-all t))
>> + (should (equal args '("/dev/null"))))
>
> Can these tests be made less platform-specific? For example, not all
> the supported platforms have /dev/null, and we have a portable
> abstraction for it.
They should actually work cross-platform, since the tests don't invoke
the commands at all; they just make sure that
`eshell-eval-using-options' can parse the switches correctly. To make
this a bit clearer though, I replaced "/dev/null" with "/some/path".
Hopefully when people see that, they'll understand that this is a "fake"
path not corresponding to anything on the actual filesystem.
>> + "sudo" '("-u" "root" "whoami")
>> + '((?u "user" t user "execute a command as another USER")
>> + :parse-leading-options-only)
>> + (should (equal user "root"))
>> + (should (equal args '("whoami"))))
[snip]
>
> And here, sudo and whoami don't necessarily exist, so something should
> be done about that, I think.
The same applies here; the commands aren't actually invoked, so they
could just as easily be named "foo" and "bar". I think the reason for
them looking like real commands is just so that a reader can glance at
them and understand more-readily what the expected result is. Readers
are likely to be familiar with "sudo" and "whoami", but wouldn't have
any preconceptions about the semantics of (fake) commands named "foo"
and "bar". If you still think it's a problem, I can change it though.
(Also, technically, both of those commands should always exist in
Eshell, since they're defined as built-in commands. "sudo" runs the
TRAMP sudo method, and "whoami" is a TRAMP-aware Lisp implementation.)
[0001-Follow-POSIX-GNU-argument-conventions-for-eshell-eva.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Wed, 05 Jan 2022 14:50:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 52999 <at> debbugs.gnu.org (full text, mbox):
> Cc: 52999 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Tue, 4 Jan 2022 13:09:29 -0800
>
> I just wasn't sure if `eshell-eval-using-options' should be in the
> manual or not. Thinking it over a bit more, it would have helped me if
> it had been in the manual (I encountered this bug while trying to write
> my own Eshell built-in command), so I added some info about it to the
> manual, mostly adapted from the docstring for
> `eshell-eval-using-options'. Hopefully I followed the right conventions
> here; I'm only vaguely familiar with the Texinfo format.
Yes, the documentation part is fine, modulo some minor comments below.
> > Can these tests be made less platform-specific? For example, not all
> > the supported platforms have /dev/null, and we have a portable
> > abstraction for it.
>
> They should actually work cross-platform, since the tests don't invoke
> the commands at all; they just make sure that
> `eshell-eval-using-options' can parse the switches correctly. To make
> this a bit clearer though, I replaced "/dev/null" with "/some/path".
> Hopefully when people see that, they'll understand that this is a "fake"
> path not corresponding to anything on the actual filesystem.
Apologies for misreading this part of the code.
> +@item symbol
> +This element is the name of the Lisp symbol that will be bound to
> +@var{value}.
Is it a symbol or its name (a string)? You say "name", but the
example:
> If @var{symbol} is @code{nil}, specifying this switch
uses a symbol, not its name.
> +@item :preserve-args
> +If present, do not pass @var{macro-args} through @code{flatten-tree}
> +and @code{eshell-stringify-list}.
I think this should explain the effect of that, or the difference
between using and not using this keyword.
> +---
> +** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
> +This now accepts command-line options with values passed as a single
^^^^
"Eshell" instead of "This" will make it more clear what you mean.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Thu, 06 Jan 2022 00:49:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 52999 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 1/5/2022 6:50 AM, Eli Zaretskii wrote:
>> Cc: 52999 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Tue, 4 Jan 2022 13:09:29 -0800
>>
>> +@item symbol
>> +This element is the name of the Lisp symbol that will be bound to
>> +@var{value}.
>
> Is it a symbol or its name (a string)? You say "name", but the
> example:
>
>> If @var{symbol} is @code{nil}, specifying this switch
>
> uses a symbol, not its name.
Good catch. I've fixed this to say that it's the Lisp symbol.
>> +@item :preserve-args
>> +If present, do not pass @var{macro-args} through @code{flatten-tree}
>> +and @code{eshell-stringify-list}.
>
> I think this should explain the effect of that, or the difference
> between using and not using this keyword.
I had to do a bit of digging to figure out what this keyword is supposed
to do in practice. It seems that it's used when a built-in Eshell
command wants to be able to accept arbitrary Lisp objects as arguments,
instead of working with just a flat list of strings. I've added more
detail to this paragraph.
>> +---
>> +** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
>> +This now accepts command-line options with values passed as a single
> ^^^^
> "Eshell" instead of "This" will make it more clear what you mean.
Ok, I updated this to refer to "Built-in commands in Eshell".
Thanks for looking over the patch.
[0001-Follow-POSIX-GNU-argument-conventions-for-eshell-eva.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Thu, 06 Jan 2022 12:32:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 52999 <at> debbugs.gnu.org (full text, mbox):
> Cc: 52999 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Wed, 5 Jan 2022 16:48:39 -0800
>
> Thanks for looking over the patch.
Thanks, the new patch LGTM.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Sat, 08 Jan 2022 19:59:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 52999 <at> debbugs.gnu.org (full text, mbox):
On 1/6/2022 4:31 AM, Eli Zaretskii wrote:
>> Cc: 52999 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Wed, 5 Jan 2022 16:48:39 -0800
>>
>> Thanks for looking over the patch.
>
> Thanks, the new patch LGTM.
Ok, I think this should be ready to merge then? (I don't have commit
access, so I can't merge it myself.)
I have a couple other Eshell improvements that I'm working on, but I'll
file separate bugs for those.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#52999
; Package
emacs
.
(Sat, 08 Jan 2022 20:11:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 52999 <at> debbugs.gnu.org (full text, mbox):
> Cc: 52999 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sat, 8 Jan 2022 11:58:29 -0800
>
> On 1/6/2022 4:31 AM, Eli Zaretskii wrote:
> >> Cc: 52999 <at> debbugs.gnu.org
> >> From: Jim Porter <jporterbugs <at> gmail.com>
> >> Date: Wed, 5 Jan 2022 16:48:39 -0800
> >>
> >> Thanks for looking over the patch.
> >
> > Thanks, the new patch LGTM.
>
> Ok, I think this should be ready to merge then?
If no further comments are posted, yes.
Reply sent
to
Eli Zaretskii <eliz <at> gnu.org>
:
You have taken responsibility.
(Wed, 12 Jan 2022 15:01:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Jim Porter <jporterbugs <at> gmail.com>
:
bug acknowledged by developer.
(Wed, 12 Jan 2022 15:01:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 52999-done <at> debbugs.gnu.org (full text, mbox):
> Cc: 52999 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Wed, 5 Jan 2022 16:48:39 -0800
>
> Ok, I updated this to refer to "Built-in commands in Eshell".
>
> Thanks for looking over the patch.
Thanks, I installed this, and I'm closing the bug.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Thu, 10 Feb 2022 12:24:11 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 47 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.