bug#54470: 29.0.50; [PATCH] Add documentation/tests for Eshell argument expansion

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@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@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@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.