GNU bug report logs - #54470
29.0.50; [PATCH] Add documentation/tests for Eshell argument expansion

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 54470 in the body.
You can then email your comments to 54470 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] Add documentation/tests for Eshell argument expansion
Date: Sat, 19 Mar 2022 18:34:44 -0700

[Message part 1 (text/plain, inline)]

Eshell supports a few ways of expanding and manipulating arguments: 
globs (which most people are likely familiar with from other shells), as 
well as predicates (which let you filter lists of file names based on 
the files' properties) and modifiers (which perform common 
transformations, mostly on lists). However, these lack unit tests and 
are currently only documented in the manual as follows:

  Eshell’s globbing syntax is very similar to that of Zsh.  Users coming
  from Bash can still use Bash-style globbing, as there are no
  incompatibilities.  Most globbing is pattern-based expansion, but
  there is also predicate-based expansion. See Filename Generation in
  The Z Shell Manual, for full syntax.

As the manual says, the syntax is "similar"; it's not actually the same. 
It also doesn't mention argument modifiers, which are related to 
predicates, but let you do different things. I think it would be best to 
fully-document the syntax so that the Eshell-specific bits are clear. 
Attached are some patches to add documentation and tests for this.

For globbing, I only added tests/documentation, but for predicates and 
modifiers, I found a few bugs as well. I'll describe each of them:

1. The "is a socket" predicate conflicts with the "is setuid" predicate 
(both are `s'), meaning that it's impossible to filter on the setuid 
bit. I've updated the "is a socket" predicate to be `=', matching zsh.

2. The "join" (`j') modifier was documented as joining a list and 
separating the element by a space, but it actually joined the list with 
no separator. It now behaves according to the documentation.

3. The "eval again" (`E') modifier didn't work; it called 
`eshell-parse-argument' with one argument, but that function takes no 
args. I've fixed this, though I had to make an educated guess on what 
the behavior should be. I chose the behavior closest to what the 
previous implementation looked like it was trying to do: re-evaluate the 
value as though it were a single argument (another option would be to 
re-evaluate as though it were a full shell command, i.e. to invoke a 
program). I've documented the behavior in the manual, so hopefully that 
will explain how it works.

Finally, I added support for the "is effective gid" predicate (`G'). 
That was already there, but commented out. It just needed uncommenting 
and the uid parts renamed to gid. I'm not sure if there was some reason 
for it to be commented out, but it works fine in my tests.

Hopefully the documentation I've added is structured/worded reasonably 
well. I did my best to follow existing conventions despite the 
very-different syntax (especially for predicates/modifiers), and tried 
to give a reasonable level of detail for all the options. If there's 
anything that's confusing or needs expanded, just let me know and I'll 
try to improve it.

Finally, I'm not sure if any of the behavior changes should be 
documented in NEWS. `G' ("is effective gid") is a small new feature, and 
the change from `s' to `=' for "is a socket" and the change for `j' 
(join) are technically incompatible changes, although maybe they're 
obscure enough that they don't need NEWS entries. I'll do whatever 
others think is best here.

[0001-Add-unit-tests-and-documentation-for-Eshell-pattern-.patch (text/plain, attachment)]

[0002-Add-unit-tests-and-documentation-for-Eshell-predicat.patch (text/plain, attachment)]

[0003-Add-G-argument-predicate-in-Eshell.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50;
 [PATCH] Add documentation/tests for Eshell argument expansion
Date: Sun, 20 Mar 2022 09:05:03 +0200

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sat, 19 Mar 2022 18:34:44 -0700
> 
> Eshell supports a few ways of expanding and manipulating arguments: 
> globs (which most people are likely familiar with from other shells), as 
> well as predicates (which let you filter lists of file names based on 
> the files' properties) and modifiers (which perform common 
> transformations, mostly on lists). However, these lack unit tests and 
> are currently only documented in the manual as follows:
> 
>    Eshell’s globbing syntax is very similar to that of Zsh.  Users coming
>    from Bash can still use Bash-style globbing, as there are no
>    incompatibilities.  Most globbing is pattern-based expansion, but
>    there is also predicate-based expansion. See Filename Generation in
>    The Z Shell Manual, for full syntax.
> 
> As the manual says, the syntax is "similar"; it's not actually the same. 
> It also doesn't mention argument modifiers, which are related to 
> predicates, but let you do different things. I think it would be best to 
> fully-document the syntax so that the Eshell-specific bits are clear. 
> Attached are some patches to add documentation and tests for this.

Thank you for working on this.  See some minor comments below.

> +Eshell's globbing syntax is very similar to that of Zsh
> +(@pxref{Filename Generation, , , zsh, The Z Shell Manual}).  Users
> +coming from Bash can still use Bash-style globbing, as there are no
> +incompatibilities.  To customize the syntax and behavior of globbing
> +in Eshell see the Customize <at> footnote{@xref{Easy Customization, , ,
> +emacs, The GNU Emacs Manual}.} group ``eshell-glob''.

Let's not have hyper-links in footnotes, it makes little sense to me.
(Yes, I know this was in the original text.)

> +@table @code
> +
> +@item *
> +Matches any string (including the empty string).  For example,
> +@samp{*.el} matches any file with the @file{.el} extension.

You use @code in the @table, but @samp in the body, which will look
inconsistent in the printed version of the manual.  Please use one of
them (I think @samp is better).

> +@item **
> +Matches any number of subdirectories in a file name, including zero.

The "including zero" part is confusing: it isn't immediately clear
whether it refers to "file name" or "any number".  I'd use "Matches
zero or more subdirectories..." instead.

> +For example, @samp{**/foo.el} matches @file{foo.el},
> +@file{bar/foo.el}, @file{bar/baz/foo.el}, etc.

The fact that the "zero" case removes the slash as well (if it does)
should be mentioned explicitly in the text, I think.  It is importand
in cases like foo/**/bar (which perhaps should have an example to make
the feature more clear).

> +@item ***
> +As @code{**}, but follows symlinks as well.
   ^^
"Like" is better, I think.

> +@item [ @dots{} ]
> +Defines a @dfn{character set} (@pxref{Regexps, , , emacs, The GNU
> +Emacs Manual}).

Every @dfn should have a @cindex entry for the term.  In this case,
the term should be qualified, since "character set" is used in many
different contexts.  Something like

  @cindex character set, in Eshell glob patterns

>                     A character set matches any character between
> +@samp{[} and @samp{]}

This is ambiguous: "between [ and ]" could be interpreted as
characters that are between those in the alphabetical order.  I'd
follow the description in the Emacs manual, which says "characters
between the two brackets".

>             You can also include ranges of characters in the set by
> +separating the start and end with @samp{-}.  Thus, @samp{[a-z]}
> +matches any lower-case @acronym{ASCII} letter.

It might be a good idea to mention here that, unlike with Zsh,
character ranges are interpreted in the Unicode codepoint order, not
in the locale-dependent collation order.  This affects stuff like
[a-z].  Also, does case-fold-search have any effect on these matches?

> +Additionally, you can include @dfn{character classes} in a character

Another @dfn without an index entry..

> +@item [^ @dots{} ]
> +Defines a @dfn{complemented character set}.  This behaves just like a

And another.

> +(ert-deftest em-glob-test/match-recursive ()
> +  "Test that \"**\" recursively matches directories."
> +  (with-fake-files '("a.el" "b.el" "ccc.el" "d.txt" "dir/a.el" "dir/sub/a.el"
> +                     "dir/symlink/a.el" "symlink/a.el" "symlink/sub/a.el")
> +    (should (equal (eshell-extended-glob "**/a.el")
> +                   '("a.el" "dir/a.el" "dir/sub/a.el")))))
> +
> +(ert-deftest em-glob-test/match-recursive-follow-symlinks ()
> +  "Test that \"***\" recursively matches directories, following symlinks."
> +  (with-fake-files '("a.el" "b.el" "ccc.el" "d.txt" "dir/a.el" "dir/sub/a.el"
> +                     "dir/symlink/a.el" "symlink/a.el" "symlink/sub/a.el")
> +    (should (equal (eshell-extended-glob "***/a.el")
> +                   '("a.el" "dir/a.el" "dir/sub/a.el" "dir/symlink/a.el"
> +                     "symlink/a.el" "symlink/sub/a.el")))))

I think symlink tests should be skipped on MS-Windows, at least by
default (with perhaps some Make variable to activate them?).  Creating
symlinks on Windows requires elevation of privileges, and causes the
system to stop and pop up the elevation confirmation dialog; for some
users it will fail even when enabled.

> +@node Argument Predication
> +@section Argument Predication and Modification
> +Eshell supports @dfn{argument predication}, to filter elements of a
> +glob, and @dfn{argument modification}, to manipulate argument values.
> +These are similar to glob qualifiers in Zsh (@pxref{Glob Qualifiers, ,
> +, zsh, The Z Shell Manual}).

Another place where index entries are needed.

> +
> +Predicates and modifiers are introduced with @code{( @dots{} )} after
> +any list argument, where @code{@dots{}} is a list of predicates or
> +modifiers.

Instead of using @dots{], which lacks any semantics here, I would
suggest to use @code{(@var{filters})}.

> +To customize the syntax and behavior of predicates and modifiers in
> +Eshell see the Customize <at> footnote{@xref{Easy Customization, , , emacs,
> +The GNU Emacs Manual}.} group ``eshell-pred''.

Again, please move this cross-reference from the footnote to the body.

> +@subsection Argument Predicates

I'd prefer not to have @subsections without nodes.  If you think a
@node is not appropriate for some reason, please use @subheading
instead.

> +The @code{^} and @code{-} operators are not argument predicates
> +themselves, but they modify the behavior of all subsequent predicates.
> +@code{^} inverts the meaning of subsequent predicates, so
> +@samp{*(^RWX)} expands to all files whose permissions disallow the
> +world from accessing them in any way (i.e., reading, writing to, or
> +modifying them).  When examining a symbolic link, @code{-} applies the
> +subsequent predicates to the link's target instead of the link itself.

This is better moved to after the table of predicates.

> +@table @asis

All the @items use @code, so "@table @code" is better, and then you
can drop @code in the @items.

> +If @var{file} is specified instead, compare against the modification
> +time of @file{file}.  Thus, @samp{a-'hello.txt'} matches all files
> +accessed after @file{hello.txt} was last accessed.

The use of quotes 'like this', here and elsewhere in a similar
context, begs the question: how to specify names that have embedded
single-quote characters in them?

> +@item e
> +Treating the value as a file name, gets the file extension.

What is considered the extension in 'foo.bar.baz'?

> +@item q
> +Marks that the value should be interpreted by Eshell literally.

What does "literally" mean here?

> +@item S
> +@item S/@var{pattern}/
> +Splits the value by the regular expression @var{pattern}.  If
> +@var{pattern} is omitted, split on spaces.

"Split the value by regexp" doesn't explain itself.  How about "split
the value using the regular expression @var{pattern} as delimiters"?

> +@item j
> +@item j/@var{delim}/
> +Joins a list of values, separated by the string @var{delim}.  If
> +@var{delim} is omitted, use a single space as the delimiter.

And if DELIM is not omitted, what should it be? a regexp?

> +@item o
> +Sorts a list of strings in ascending lexicographic order.
> +
> +@item O
> +Sorts a list of strings in descending lexicographic order.

This should clarify what is considered the lexicographic order here.
Given the usual dependence on the locale, this is not self-evident.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Sun, 20 Mar 2022 13:57:26 -0700

[Message part 1 (text/plain, inline)]

On 3/20/2022 12:05 AM, Eli Zaretskii wrote:
> Thank you for working on this.  See some minor comments below.

Thanks for the thorough review.

>> +Eshell's globbing syntax is very similar to that of Zsh
>> +(@pxref{Filename Generation, , , zsh, The Z Shell Manual}).  Users
>> +coming from Bash can still use Bash-style globbing, as there are no
>> +incompatibilities.  To customize the syntax and behavior of globbing
>> +in Eshell see the Customize <at> footnote{@xref{Easy Customization, , ,
>> +emacs, The GNU Emacs Manual}.} group ``eshell-glob''.
> 
> Let's not have hyper-links in footnotes, it makes little sense to me.
> (Yes, I know this was in the original text.)

Ok, fixed.

>> +@table @code
>> +
>> +@item *
>> +Matches any string (including the empty string).  For example,
>> +@samp{*.el} matches any file with the @file{.el} extension.
> 
> You use @code in the @table, but @samp in the body, which will look
> inconsistent in the printed version of the manual.  Please use one of
> them (I think @samp is better).

Done. I only did this for the glob section though. Should I change the 
items in the predicates/modifiers to use @samp too? They're different 
enough that I'm not quite sure.

Or would @kbd be better to use here? These are things meant to be typed 
by the user into an interactive prompt, after all...

>> +@item **
>> +Matches any number of subdirectories in a file name, including zero.
> 
> The "including zero" part is confusing: it isn't immediately clear
> whether it refers to "file name" or "any number".  I'd use "Matches
> zero or more subdirectories..." instead.

Fixed.

>> +For example, @samp{**/foo.el} matches @file{foo.el},
>> +@file{bar/foo.el}, @file{bar/baz/foo.el}, etc.
> 
> The fact that the "zero" case removes the slash as well (if it does)
> should be mentioned explicitly in the text, I think.  It is importand
> in cases like foo/**/bar (which perhaps should have an example to make
> the feature more clear).

I've updated the item to be '**/', which is more accurate, since the 
trailing '/' is really a part of the token (the Zsh manual also 
documents it this way). I also added a short note that the '**/' must be 
the entirety of a file name segment (so something like 'foo**/bar.el' is 
nonsense).

>> +@item ***
>> +As @code{**}, but follows symlinks as well.
>     ^^
> "Like" is better, I think.

Fixed.

>> +@item [ @dots{} ]
>> +Defines a @dfn{character set} (@pxref{Regexps, , , emacs, The GNU
>> +Emacs Manual}).
> 
> Every @dfn should have a @cindex entry for the term.  In this case,
> the term should be qualified, since "character set" is used in many
> different contexts.  Something like
> 
>    @cindex character set, in Eshell glob patterns

Fixed.

>>                      A character set matches any character between
>> +@samp{[} and @samp{]}
> 
> This is ambiguous: "between [ and ]" could be interpreted as
> characters that are between those in the alphabetical order.  I'd
> follow the description in the Emacs manual, which says "characters
> between the two brackets".

Fixed.

>>              You can also include ranges of characters in the set by
>> +separating the start and end with @samp{-}.  Thus, @samp{[a-z]}
>> +matches any lower-case @acronym{ASCII} letter.
> 
> It might be a good idea to mention here that, unlike with Zsh,
> character ranges are interpreted in the Unicode codepoint order, not
> in the locale-dependent collation order.  This affects stuff like
> [a-z].  Also, does case-fold-search have any effect on these matches?

Done. case-fold-search doesn't have any effect, but 
eshell-glob-case-insensitive does. I've added a bit of documentation 
about that to the manual.

>> +Additionally, you can include @dfn{character classes} in a character
> 
> Another @dfn without an index entry..

Fixed.

>> +@item [^ @dots{} ]
>> +Defines a @dfn{complemented character set}.  This behaves just like a
> 
> And another.

Fixed here (and for @dfn{group} just below this).

>> +(ert-deftest em-glob-test/match-recursive ()
>> +  "Test that \"**\" recursively matches directories."
>> +  (with-fake-files '("a.el" "b.el" "ccc.el" "d.txt" "dir/a.el" "dir/sub/a.el"
>> +                     "dir/symlink/a.el" "symlink/a.el" "symlink/sub/a.el")
>> +    (should (equal (eshell-extended-glob "**/a.el")
>> +                   '("a.el" "dir/a.el" "dir/sub/a.el")))))
>> +
>> +(ert-deftest em-glob-test/match-recursive-follow-symlinks ()
>> +  "Test that \"***\" recursively matches directories, following symlinks."
>> +  (with-fake-files '("a.el" "b.el" "ccc.el" "d.txt" "dir/a.el" "dir/sub/a.el"
>> +                     "dir/symlink/a.el" "symlink/a.el" "symlink/sub/a.el")
>> +    (should (equal (eshell-extended-glob "***/a.el")
>> +                   '("a.el" "dir/a.el" "dir/sub/a.el" "dir/symlink/a.el"
>> +                     "symlink/a.el" "symlink/sub/a.el")))))
> 
> I think symlink tests should be skipped on MS-Windows, at least by
> default (with perhaps some Make variable to activate them?).  Creating
> symlinks on Windows requires elevation of privileges, and causes the
> system to stop and pop up the elevation confirmation dialog; for some
> users it will fail even when enabled.

These tests (and the ones in em-pred-tests.el) don't actually touch the 
filesystem. I just provided mock implementations of the relevant Elisp 
functions so that the tests can pretend that the files corresponding to 
these names really exist. That's a bit less realistic, but it should 
work on all systems without errors. It should work fine on MS-Windows, 
unless there are other bugs present.

That said, if you'd prefer a different implementation (e.g. one that 
examines real files) or just more documentation about what I'm doing 
here, let me know.

>> +@node Argument Predication
>> +@section Argument Predication and Modification
>> +Eshell supports @dfn{argument predication}, to filter elements of a
>> +glob, and @dfn{argument modification}, to manipulate argument values.
>> +These are similar to glob qualifiers in Zsh (@pxref{Glob Qualifiers, ,
>> +, zsh, The Z Shell Manual}).
> 
> Another place where index entries are needed.

Done.

>> +
>> +Predicates and modifiers are introduced with @code{( @dots{} )} after
>> +any list argument, where @code{@dots{}} is a list of predicates or
>> +modifiers.
> 
> Instead of using @dots{], which lacks any semantics here, I would
> suggest to use @code{(@var{filters})}.

Done.

>> +To customize the syntax and behavior of predicates and modifiers in
>> +Eshell see the Customize <at> footnote{@xref{Easy Customization, , , emacs,
>> +The GNU Emacs Manual}.} group ``eshell-pred''.
> 
> Again, please move this cross-reference from the footnote to the body.

Done.

>> +@subsection Argument Predicates
> 
> I'd prefer not to have @subsections without nodes.  If you think a
> @node is not appropriate for some reason, please use @subheading
> instead.

Done, added nodes for these.

>> +The @code{^} and @code{-} operators are not argument predicates
>> +themselves, but they modify the behavior of all subsequent predicates.
>> +@code{^} inverts the meaning of subsequent predicates, so
>> +@samp{*(^RWX)} expands to all files whose permissions disallow the
>> +world from accessing them in any way (i.e., reading, writing to, or
>> +modifying them).  When examining a symbolic link, @code{-} applies the
>> +subsequent predicates to the link's target instead of the link itself.
> 
> This is better moved to after the table of predicates.

Done.

>> +@table @asis
> 
> All the @items use @code, so "@table @code" is better, and then you
> can drop @code in the @items.

This is because there's a single item that doesn't just use @code:

  @item @code{.} (Period)

I just lifted that convention from the "Syntax of Regular Expressions" 
section in the Emacs manual. I think it helps disambiguate what that 
character is, since a lone "." on a line is a bit confusing.

Note: I changed this slightly in the latest patch to add '@r{}' around 
'(Period)', matching the regexp section.

>> +If @var{file} is specified instead, compare against the modification
>> +time of @file{file}.  Thus, @samp{a-'hello.txt'} matches all files
>> +accessed after @file{hello.txt} was last accessed.
> 
> The use of quotes 'like this', here and elsewhere in a similar
> context, begs the question: how to specify names that have embedded
> single-quote characters in them?

"Very carefully." :)

Seriously though, this is an area I don't fully understand yet, but in 
which I've found several bugs (or at least I think they're bugs). As 
such, I intentionally avoided documenting this since it's pretty 
confusing. To answer your specific question, you could type:

  *(a-"hello'there.txt")

However, the quoting rules are inconsistent. For example, this is how 
you normally insert a single quote inside a single-quoted string in Eshell:

  ~ $ echo 'hi''there'
  hi'there

That doesn't work inside predicates though, and it would treat the file 
name as "hi''there".

Relatedly, the allowed delimiters aren't consistent. a/m/c/u/g all allow 
*any* non-digit character as a delimiter, so 'a-XfooX' compares against 
the file 'foo'. They also use "balanced" bracket pairs, so 'a-<foo>' 
means the same thing.

:s/:gs/:i/:x allow any character as the delimiter (including digits), 
but don't use "balanced" bracket pairs, so you could type 
':s<pattern<repl<' or ':i<pattern<'.

:j/:S only allow ' and / as the delimiters.

I think this should be fixed, but it'll take a fair bit of work, and 
part of the reason for my filing this bug was to lay the foundation of 
unit tests so that I could adjust the escaping logic without regressing 
anything.

>> +@item e
>> +Treating the value as a file name, gets the file extension.
> 
> What is considered the extension in 'foo.bar.baz'?

It's 'baz'. I've expanded on this, explaining that it returns the final 
extension sans dot. I also added examples for this and all the other 
file name modifiers ('h'/head, 't'/tail, and 'r'/root).

>> +@item q
>> +Marks that the value should be interpreted by Eshell literally.
> 
> What does "literally" mean here?

Added an explanation for this (it means that any special characters like 
'$' lose their meaning and are treated literally). That said, I'm not 
100% sure when this would be useful, since I couldn't figure out a time 
where this would change how a command is executed. I'm probably just 
missing something though.

>> +@item S
>> +@item S/@var{pattern}/
>> +Splits the value by the regular expression @var{pattern}.  If
>> +@var{pattern} is omitted, split on spaces.
> 
> "Split the value by regexp" doesn't explain itself.  How about "split
> the value using the regular expression @var{pattern} as delimiters"?

Fixed.

>> +@item j
>> +@item j/@var{delim}/
>> +Joins a list of values, separated by the string @var{delim}.  If
>> +@var{delim} is omitted, use a single space as the delimiter.
> 
> And if DELIM is not omitted, what should it be? a regexp?

A string. I mentioned this in the first sentence, but I think it was a 
bit too brief. I've reworded this to be more explicit.

>> +@item o
>> +Sorts a list of strings in ascending lexicographic order.
>> +
>> +@item O
>> +Sorts a list of strings in descending lexicographic order.
> 
> This should clarify what is considered the lexicographic order here.
> Given the usual dependence on the locale, this is not self-evident.

Fixed, and added a cross-reference to the corresponding section of the 
Elisp manual. It would have been nice to link to the function itself: 
'string<', though I'm not sure how to do that (or if that's something I 
shouldn't do).

[0001-Add-unit-tests-and-documentation-for-Eshell-pattern-.patch (text/plain, attachment)]

[0002-Add-unit-tests-and-documentation-for-Eshell-predicat.patch (text/plain, attachment)]

[0003-Add-G-argument-predicate-in-Eshell.patch (text/plain, attachment)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Sun, 27 Mar 2022 19:29:43 -0700

On 3/20/2022 1:57 PM, Jim Porter wrote:
> On 3/20/2022 12:05 AM, Eli Zaretskii wrote:
>> The use of quotes 'like this', here and elsewhere in a similar
>> context, begs the question: how to specify names that have embedded
>> single-quote characters in them?
> 
> "Very carefully." :)
> 
> Seriously though, this is an area I don't fully understand yet, but in 
> which I've found several bugs (or at least I think they're bugs). As 
> such, I intentionally avoided documenting this since it's pretty 
> confusing.

I narrowed down one of the bugs I mentioned here (and fixed it): bug#54603.

There are still a few other issues with quoting/escaping in argument 
predicates/modifiers. I can work on fixing these in a separate bug, or 
update my patches in this bug if you prefer. I lean very slightly 
towards the former, since the quoting/escaping logic could use some 
changes to be more consistent. That seems different enough from this bug 
that I think it'd be simpler to separate them. That said, if you'd 
prefer I do this all in this bug, that's ok too.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Tue, 29 Mar 2022 21:47:31 -0700

[Message part 1 (text/plain, inline)]

On 3/20/2022 1:57 PM, Jim Porter wrote:
> On 3/20/2022 12:05 AM, Eli Zaretskii wrote:
>> Thank you for working on this.  See some minor comments below.

I found an additional bug in the global substitution modifier which 
could cause an infinite loop in some cases, e.g. with "(:gs/a/a/)". The 
regexp search wouldn't advance to the next character correctly and could 
get stuck. I've fixed this by using `replace-regexp-in-string' instead.

[0001-Add-unit-tests-and-documentation-for-Eshell-pattern-.patch (text/plain, attachment)]

[0002-Add-unit-tests-and-documentation-for-Eshell-predicat.patch (text/plain, attachment)]

[0003-Add-G-argument-predicate-in-Eshell.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Thu, 31 Mar 2022 10:19:05 +0300

> Cc: 54470 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sun, 20 Mar 2022 13:57:26 -0700
> 
> >> +@table @code
> >> +
> >> +@item *
> >> +Matches any string (including the empty string).  For example,
> >> +@samp{*.el} matches any file with the @file{.el} extension.
> > 
> > You use @code in the @table, but @samp in the body, which will look
> > inconsistent in the printed version of the manual.  Please use one of
> > them (I think @samp is better).
> 
> Done. I only did this for the glob section though. Should I change the 
> items in the predicates/modifiers to use @samp too? They're different 
> enough that I'm not quite sure.
> 
> Or would @kbd be better to use here? These are things meant to be typed 
> by the user into an interactive prompt, after all...

For something that user should type, @kbd is appropriate, yes.  But
since these all are portions of file names, perhaps @file is the best
markup.

Thanks.

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

From: Richard Stallman <rms <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50;
 [PATCH] Add documentation/tests for Eshell argument expansion
Date: Fri, 01 Apr 2022 00:11:29 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Or would @kbd be better to use here? These are things meant to be typed 
  > > by the user into an interactive prompt, after all...

  > For something that user should type, @kbd is appropriate, yes.  But
  > since these all are portions of file names, perhaps @file is the best
  > markup.

@kbd is right for things that are meant specifically and only as
keyboard input.

All sorts of syntactic entities can be entered as input in certain contexts,
but that doesn't mean they should always be written in @kbd.
For instance, you can enter a file name as keyboard input.
Any file name can be entered that way.
But file names are used in many other contexts too.
Thus, in general a file name should not be written in @kbd,
not even whe you're talking about giving a file name as keyboard input.

Perhaps when you're talking about the act of typing a command
containing a file name you might use @kbd for that.


-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: rms <at> gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Fri, 1 Apr 2022 22:10:07 -0700

[Message part 1 (text/plain, inline)]

On 3/31/2022 9:11 PM, Richard Stallman wrote:
> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
> 
>    > > Or would @kbd be better to use here? These are things meant to be typed
>    > > by the user into an interactive prompt, after all...
> 
>    > For something that user should type, @kbd is appropriate, yes.  But
>    > since these all are portions of file names, perhaps @file is the best
>    > markup.
> 
> @kbd is right for things that are meant specifically and only as
> keyboard input.

Thanks for the explanation.

Having thought this over further, I think Eli's suggestion to use @samp 
makes sense for both the globs and the predicates/modifiers, so I've 
updated my patches to do this.

[0001-Add-unit-tests-and-documentation-for-Eshell-pattern-.patch (text/plain, attachment)]

[0002-Add-unit-tests-and-documentation-for-Eshell-predicat.patch (text/plain, attachment)]

[0003-Add-G-argument-predicate-in-Eshell.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Fri, 15 Apr 2022 15:56:32 +0300

> Cc: 54470 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Fri, 1 Apr 2022 22:10:07 -0700
> 
> > @kbd is right for things that are meant specifically and only as
> > keyboard input.
> 
> Thanks for the explanation.
> 
> Having thought this over further, I think Eli's suggestion to use @samp 
> makes sense for both the globs and the predicates/modifiers, so I've 
> updated my patches to do this.

Thanks.  I was about to install these, but then I saw that some of
the tests you added fail on my system:

  Test em-pred-test/combine-predicate-and-modifier backtrace:
    signal(ert-test-failed (((should (equal (eshell-eval-predicate files
    ert-fail(((should (equal (eshell-eval-predicate files ".:e:u") '("el
    (if (unwind-protect (setq value-724 (apply fn-722 args-723)) (setq f
    (let (form-description-726) (if (unwind-protect (setq value-724 (app
    (let ((value-724 'ert-form-evaluation-aborted-725)) (let (form-descr
    (let* ((fn-722 #'equal) (args-723 (condition-case err (let ((signal-
    (let ((files '("/fake/type=-.el" "/fake/type=-.txt" "/fake/type=s.el
    (progn (fset #'file-executable-p vnew) (fset #'file-symlink-p vnew)
    (unwind-protect (progn (fset #'file-executable-p vnew) (fset #'file-
    (let* ((vnew #'(lambda (file &rest rest) (apply (if (and ... ...) #'
    (closure (t) nil (let* ((vnew #'(lambda (file &rest rest) (apply (if
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name em-pred-test/combine-predicate-and-mo
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/em-pred-tests
    command-line()
    normal-top-level()
  Test em-pred-test/combine-predicate-and-modifier condition:
      (ert-test-failed
       ((should
	 (equal
	  (eshell-eval-predicate files ".:e:u")
	  '...))
	:form
	(equal nil
	       ("el" "txt"))
	:value nil :explanation
	(different-types nil
			 ("el" "txt"))))
     FAILED   1/34  em-pred-test/combine-predicate-and-modifier (0.484375 sec) at lisp/eshell/em-pred-tests.el:513


  Test em-pred-test/predicate-file-types backtrace:
    signal(ert-test-failed (((should (equal (eshell-eval-predicate files
    ert-fail(((should (equal (eshell-eval-predicate files "%") '("/fake/
    (if (unwind-protect (setq value-20 (apply fn-18 args-19)) (setq form
    (let (form-description-22) (if (unwind-protect (setq value-20 (apply
    (let ((value-20 'ert-form-evaluation-aborted-21)) (let (form-descrip
    (let* ((fn-18 #'equal) (args-19 (condition-case err (let ((signal-ho
    (let ((files (mapcar #'(lambda (i) (format "/fake/type=%s" i)) '("b"
    (progn (fset #'file-executable-p vnew) (fset #'file-symlink-p vnew)
    (unwind-protect (progn (fset #'file-executable-p vnew) (fset #'file-
    (let* ((vnew #'(lambda (file &rest rest) (apply (if (and ... ...) #'
    (closure (t) nil (let* ((vnew #'(lambda (file &rest rest) (apply (if
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name em-pred-test/predicate-file-types :do
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/em-pred-tests
    command-line()
    normal-top-level()
  Test em-pred-test/predicate-file-types condition:
      (ert-test-failed
       ((should
	 (equal
	  (eshell-eval-predicate files "%")
	  '...))
	:form
	(equal nil
	       ("/fake/type=b" "/fake/type=c"))
	:value nil :explanation
	(different-types nil
			 ("/fake/type=b" "/fake/type=c"))))
     FAILED  29/34  em-pred-test/predicate-file-types (0.140625 sec) at lisp/eshell/em-pred-tests.el:152


  Test em-pred-test/predicate-links backtrace:
    signal(ert-test-failed (((should (equal (eshell-eval-predicate files
    ert-fail(((should (equal (eshell-eval-predicate files "l1") '("/fake
    (if (unwind-protect (setq value-214 (apply fn-212 args-213)) (setq f
    (let (form-description-216) (if (unwind-protect (setq value-214 (app
    (let ((value-214 'ert-form-evaluation-aborted-215)) (let (form-descr
    (let* ((fn-212 #'equal) (args-213 (condition-case err (let ((signal-
    (let ((files '("/fake/links=1" "/fake/links=2" "/fake/links=3"))) (l
    (progn (fset #'file-executable-p vnew) (fset #'file-symlink-p vnew)
    (unwind-protect (progn (fset #'file-executable-p vnew) (fset #'file-
    (let* ((vnew #'(lambda (file &rest rest) (apply (if (and ... ...) #'
    (closure (t) nil (let* ((vnew #'(lambda (file &rest rest) (apply (if
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name em-pred-test/predicate-links :documen
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/em-pred-tests
    command-line()
    normal-top-level()
  Test em-pred-test/predicate-links condition:
      (ert-test-failed
       ((should
	 (equal
	  (eshell-eval-predicate files "l1")
	  '...))
	:form
	(equal nil
	       ("/fake/links=1"))
	:value nil :explanation
	(different-types nil
			 ("/fake/links=1"))))
     FAILED  31/34  em-pred-test/predicate-links (0.000000 sec) at lisp/eshell/em-pred-tests.el:228

  Test em-pred-test/predicate-size backtrace:
    signal(ert-test-failed (((should (equal (eshell-eval-predicate files
    ert-fail(((should (equal (eshell-eval-predicate files "L2048") '("/f
    (if (unwind-protect (setq value-379 (apply fn-377 args-378)) (setq f
    (let (form-description-381) (if (unwind-protect (setq value-379 (app
    (let ((value-379 'ert-form-evaluation-aborted-380)) (let (form-descr
    (let* ((fn-377 #'equal) (args-378 (condition-case err (let ((signal-
    (let ((files '("/fake/size=0" "/fake/size=1024" "/fake/size=2048" "/
    (progn (fset #'file-executable-p vnew) (fset #'file-symlink-p vnew)
    (unwind-protect (progn (fset #'file-executable-p vnew) (fset #'file-
    (let* ((vnew #'(lambda (file &rest rest) (apply (if (and ... ...) #'
    (closure (t) nil (let* ((vnew #'(lambda (file &rest rest) (apply (if
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name em-pred-test/predicate-size :document
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/em-pred-tests
    command-line()
    normal-top-level()
  Test em-pred-test/predicate-size condition:
      (ert-test-failed
       ((should
	 (equal
	  (eshell-eval-predicate files "L2048")
	  '...))
	:form
	(equal nil
	       ("/fake/size=2048"))
	:value nil :explanation
	(different-types nil
			 ("/fake/size=2048"))))
     FAILED  33/34  em-pred-test/predicate-size (0.156250 sec) at lisp/eshell/em-pred-tests.el:321

Can you fix these failures and resubmit?  If they are due to some
MS-Windows specific issues, please tell how can I help you with
diagnosing the problem.

Thanks.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Fri, 15 Apr 2022 21:57:02 -0700

[Message part 1 (text/plain, inline)]

On 4/15/2022 5:56 AM, Eli Zaretskii wrote:
>> Cc: 54470 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Fri, 1 Apr 2022 22:10:07 -0700
>>
>>> @kbd is right for things that are meant specifically and only as
>>> keyboard input.
>>
>> Thanks for the explanation.
>>
>> Having thought this over further, I think Eli's suggestion to use @samp
>> makes sense for both the globs and the predicates/modifiers, so I've
>> updated my patches to do this.
> 
> Thanks.  I was about to install these, but then I saw that some of
> the tests you added fail on my system:
[snip]

Thanks for testing. I think this is because `eshell-file-attributes' 
calls `expand-file-name' on the FILE argument, which prepends a drive 
letter on MS Windows. That makes my code in `eshell-partial-let-func' 
(in em-pred-tests.el) fail to identify the fake files.

Can you try the attached patch to see if the tests pass? If it works, 
I'll fold it into the previous patches and resubmit them. (It works for 
me on an MS Windows system, but I don't have build tools on it, so I 
just used the binary release of 28.1 with some of the bits copied from 
my patches to test it out.)

There are a few other ways I could fix this, but this seemed like the 
best. Now, if `eshell-file-attributes' calls `file-attributes', it 
always forwards the FILE argument unchanged, so the wrapping is more 
"transparent" in that case. (Note: I'm not sure `eshell-file-attributes' 
is even necessary anymore; maybe Tramp handles that for us? I haven't 
tested this enough to be confident we can remove it though...)

[fix-ms-windows-tests.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Sat, 16 Apr 2022 13:30:24 +0300

> Cc: 54470 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Fri, 15 Apr 2022 21:57:02 -0700
> 
> On 4/15/2022 5:56 AM, Eli Zaretskii wrote:
> >> Cc: 54470 <at> debbugs.gnu.org
> >> From: Jim Porter <jporterbugs <at> gmail.com>
> >> Date: Fri, 1 Apr 2022 22:10:07 -0700
> >>
> >>> @kbd is right for things that are meant specifically and only as
> >>> keyboard input.
> >>
> >> Thanks for the explanation.
> >>
> >> Having thought this over further, I think Eli's suggestion to use @samp
> >> makes sense for both the globs and the predicates/modifiers, so I've
> >> updated my patches to do this.
> > 
> > Thanks.  I was about to install these, but then I saw that some of
> > the tests you added fail on my system:
> [snip]
> 
> Thanks for testing. I think this is because `eshell-file-attributes' 
> calls `expand-file-name' on the FILE argument, which prepends a drive 
> letter on MS Windows. That makes my code in `eshell-partial-let-func' 
> (in em-pred-tests.el) fail to identify the fake files.
> 
> Can you try the attached patch to see if the tests pass? If it works, 
> I'll fold it into the previous patches and resubmit them. (It works for 
> me on an MS Windows system, but I don't have build tools on it, so I 
> just used the binary release of 28.1 with some of the bits copied from 
> my patches to test it out.)

Yes, this fixes the failures, thanks.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54470 <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Sat, 16 Apr 2022 10:14:43 -0700

[Message part 1 (text/plain, inline)]

On 4/16/2022 3:30 AM, Eli Zaretskii wrote:
>> Cc: 54470 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Fri, 15 Apr 2022 21:57:02 -0700
>>
>> Can you try the attached patch to see if the tests pass? If it works,
>> I'll fold it into the previous patches and resubmit them. (It works for
>> me on an MS Windows system, but I don't have build tools on it, so I
>> just used the binary release of 28.1 with some of the bits copied from
>> my patches to test it out.)
> 
> Yes, this fixes the failures, thanks.

Cool, thanks. Here are final patches for merging then (only the second 
patch is changed from before, but I attached the full series for 
convenience).

[0001-Add-unit-tests-and-documentation-for-Eshell-pattern-.patch (text/plain, attachment)]

[0002-Add-unit-tests-and-documentation-for-Eshell-predicat.patch (text/plain, attachment)]

[0003-Add-G-argument-predicate-in-Eshell.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54470-done <at> debbugs.gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Sun, 17 Apr 2022 10:32:42 +0300

> Cc: 54470 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sat, 16 Apr 2022 10:14:43 -0700
> 
> >> Can you try the attached patch to see if the tests pass? If it works,
> >> I'll fold it into the previous patches and resubmit them. (It works for
> >> me on an MS Windows system, but I don't have build tools on it, so I
> >> just used the binary release of 28.1 with some of the bits copied from
> >> my patches to test it out.)
> > 
> > Yes, this fixes the failures, thanks.
> 
> Cool, thanks. Here are final patches for merging then (only the second 
> patch is changed from before, but I attached the full series for 
> convenience).

Thanks, installed on the master branch, and closing the bug.

Please in the future always mention the bug number in the commit log
messages (I added that for you in this case).

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 54470 <at> debbugs.gnu.org, eliz <at> gnu.org
Subject: Re: bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell
 argument expansion
Date: Sun, 17 Apr 2022 11:38:20 -0700

On 4/17/2022 12:32 AM, Eli Zaretskii wrote:
> Thanks, installed on the master branch, and closing the bug.
> 
> Please in the future always mention the bug number in the commit log
> messages (I added that for you in this case).

Thanks, I'll try to remember that for the future.

Previous Next

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