Re: [PATCHv1] [long] improvements to limit/ulimit API and doc

[Oliver Kiddle <opk@xxxxxxx>]

Stephane Chazelas wrote:
> - I've only tested it on Ubuntu, FreeBSD and Cygwin. I suspect
>   my test cases will fail on 32bit systems. They're skipped on

I tried building and running tests on Solaris 11 and a 32bit build on
Solaris 10. I also checked OpenBSD, NetBSD and Dragonfly. There were
quite a few other issues but nothing introduced by your patch and
your tests passed. Let me know if there's anything additionally that
it'd be useful for me to test manually.

I've got a few comments, mostly on the documentation.

> -If no var(resource) is given, print all limits.
> +If tt(-s) is given without other arguments, the resource limits of the
> +current shell is set to the previously set resource limits of the

s/is/are/

> +sitem(tt(rt_priority))(Maximum realtime priority.)
> +sitem(tt(rt_time))(Maximum realtime CPU time slice.)

From the tcsh manual, these appear to be called maxrtprio and maxrttime
there. But we already had these names, right? Similarly tcsh has
posixlocks.

> -var(limit) is a number, with an optional scaling factor, as follows:
> +If var(limit) is a decimal integer number without suffix, then for
> +historical reason and compatibility with csh where that command comes from,

s/reason/reasons/
Or perhaps just drop the first three words of that line.

> +the unit will depend on the type of limit: microsecond for tt(rt_time),
> +second for all other time limits, kibibytes (1024 bytes) for all limits that

"microseconds" and "seconds" - units are mostly always plural.

> +The same suffixes are recognised as for tt(limit), but bear in mind that
> +default units when no suffix is specified varry between the two.

s/varry/vary/

> +performed with kibibytes, mebibytes, or blocks (of 512 bytes; catch: not
> +em(pebibytes)) instead. (On some systems additional specifiers are available

Not sure about the use of "catch:" there. I'd likely apply emphasis to
not instead

> +The `limit` builtin accepts more units for resource limits, now shared
> +with the `ulimit` builtin allowing to specify arbitrary limit values.

"allowing to" isn't correct unless you add an object for the verb "allow"
such as "you". It might be better to reword it in the passive tense ("to
be specified").

> +`limit -s somelimit` now reports the shell process' own value of the limit.
> +The `limit` and `unlimit` builtins are now available again in csh emulation.
> +More generally, the resouce limit handling interfaces and documentation have

"resource".
"More generally" doesn't really add anything useful to the sentence.

> +++ b/Src/Builtins/rlimits.c
> @@ -55,7 +55,8 @@ typedef struct resinfo_T {
>   * 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.

>  0:check if limit option letters are unique
> 
> +  if sh -c 'ulimit 2048' > /dev/null 2>&1; then

This doesn't work on Solaris or any of the BSDs except FreeBSD - you
have to specify -f to limit file sizes. So it would be skipped despite
the fact that the test could otherwise work.
Isn't -f fairly standard?

From my testing of the changes, it definitely looks like a nice improvement.

Oliver