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

[Jim Porter]

Thanks, I've attached an updated patch.

On 1/4/2022 5:01 AM, Eli Zaretskii wrote:
> > From: Jim Porter <jporterbugs@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
Description: Text document