Re: [PATCH] ulimit option completions using ulimit -a output

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Stephane Chazelas wrote on Thu, Nov 26, 2020 at 20:14:31 +0000:
> 2020-11-26 00:43:33 +0100, Oliver Kiddle:
> [...]
> > >   * 1. Add zsh_LIMIT_PRESENT(RLIMIT_XXX) in configure.ac.
> > >   * 2. Add an entry for RLIMIT_XXX to known_resources[].
> > >   *    Make sure the option letter (resinto_T.opt) is unique.
> > > - * 3. Build zsh and run the test B12rlimit.ztst.
> > > + * 3. Add entry in documentation for the limit and ulimit builtins
> > > + * 4. Build zsh and run the test B12rlimit.ztst.
> > 
> > 5. Update Completion/Zsh/Command/_ulimit
> > 
> > Units were often fairly well documented there but are missing in a
> > couple of cases so you may be able to add a couple.
> [...]
> 
> The problem is that the list of ulimit options is system
> dependent, so I propose the improvement below which uses the
> output of ulimit -a to build the list of completions.

Sorry for the very large delay on getting to review this.  Thanks
Lawrence for fishing it out of the archives.  Review:

> From: Stephane Chazelas <stephane@chazelas.org>
> Date: Thu, 26 Nov 2020 19:22:03 +0000
> Subject: [PATCH] ulimit option completions using ulimit -a output
> 
> The list of options supported by ulimit is system dependent. Rather than
> hardcoding the full list of options supported on any system in the
> completer, we get the list of options from the output of "ulimit -a".

+1

> That means less descriptive descriptions but more relevant completions
> to the user (and not having to update the completer every time a new
> limit is added).

Should we copy the more-descriptive descriptions to the output of
`ulimit -a`?  (I.e., known_resources[] in Src/Modules/rlimits.c)

> +++ b/Completion/Zsh/Command/_ulimit
> @@ -2,18 +2,16 @@
>  
>  [[ $PREFIX = u* ]] && compadd unlimited && return 0
>  
> +local -a opts
> +setopt localoptions extendedglob
> +
> +opts=(
> +  ${${${(fo)"$(ulimit -a)"}:#-N*}%%  *}
> +)
>  _arguments -s \
>    '-H[set hard limits]' \
>    '-S[set soft and hard limits (with -H)]' \
> -  '(-H -S -c -d -f -l -m -n -s -t *)-a[list all current resource limits]' \
> -  '-c[core dump size limit]:max core dump size (512-byte blocks)' \
> -  '-d[maximum size of data segment]:maximum size of data segment (K-bytes)' \
> -  '-f[size of largest file allowed]:size of largest file allowed (512-byte blocks)' \
> -  '-l[maximum size of locked in memory]:maximum size of locked in memory (K-bytes)' \
> -  '-m[maximum size of physical memory]:maximum size of physical memory (K-bytes)' \
> -  '-n[maximum no. of open file descriptors]:maximum no. of open file descriptors' \
> -  '-s[stack size limit]:stack size limit (K-bytes)' \
> -  '-t[maximum cpu time per process]:maximum cpu time per process (seconds)' \
> -  '-u[processes available to the user]:processes' \
> -  '-v[maximum size of virtual memory]:maximum size of virtual memory (K-bytes)' \
> -  '*:size of largest file allowed'
> +  '-N+[specify limit by number]:limit number::limit value' \

[Corrected per 47661]

Why is the second argument optional?  s/::/: :/?

Completing «-N 15 <TAB>» doesn't offer "rt cpu time (microseconds)",
even though the output of `ulimit -a` does show that description.

Completing after «-N <TAB>» could offer "15".

> +  "(-N ${(M@j: :)opts#-?} *)-a[list all current resource limits]" \
> +  ${opts/(#b)(-?): (*)/$match[1][$match[2]]:$match[2]} \

Escape colons in ${match[1]} and ${match[2]}.

It would be nice to retain the current values in in $opts and then
include them in the `message' part of the spec.

> +  '*:file size (blocks)'

Sorry, what's this?  The manual does mention additional positional
arguments, but doesn't describe them, does it?

Sorry again for our very late review.

Review points that aren't regressions aren't blockers.

Daniel