GNU bug report logs - #52999
29.0.50; [PATCH] `eshell-eval-using-options' should follow POSIX/GNU argument conventions

Previous Next

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.

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: 29.0.50; [PATCH] `eshell-eval-using-options' should follow POSIX/GNU
 argument conventions
Date: Mon, 3 Jan 2022 17:36:06 -0800

[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)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v2] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Mon, 3 Jan 2022 21:33:28 -0800

[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)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50;
 [PATCH v2] `eshell-eval-using-options' should follow POSIX/GNU
 argument conventions
Date: Tue, 04 Jan 2022 15:01:50 +0200

> 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.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Tue, 4 Jan 2022 13:09:29 -0800

[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)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Wed, 05 Jan 2022 16:50:01 +0200

> 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.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Wed, 5 Jan 2022 16:48:39 -0800

[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)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Thu, 06 Jan 2022 14:31:07 +0200

> 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.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
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? (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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 52999 <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Sat, 08 Jan 2022 22:10:35 +0200

> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 52999-done <at> debbugs.gnu.org
Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should
 follow POSIX/GNU argument conventions
Date: Wed, 12 Jan 2022 17:00:29 +0200

> 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.

Previous Next

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