Re: svn commit: r1884279 - in /subversion/trunk: build/ subversion/tests/cmdline/svntest/

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

futatuki@apache.org wrote on Thu, 10 Dec 2020 14:45 +00:00:
> Ensure close file descriptors by using context manager in test suite.

Suggest instead:

    test suite: Ensure file descriptors are closed in a timely manner, using context managers.

I.e.:

- Use "$area: $description" format
- Fix ungrammatical use of "close"
  (alternative: "Ensure timely closure of file descriptors")

And since I'm reviewing already, a few more nits:

> Generally, it is true that as file descriptors are closed when their
> underlying objects are deleted, we don't need close them explicitly.
> However if a Python language implementation that uses other strategy
> than reference count model, there is no warranty it will happen

"if a Python language implementation uses a strategy other than reference counting"

> immediately after when those objects lose the last reference.  So we
> use context manager to ensure close() is called immediately after
> those objects to be unnecessary.  This helps us to run 'make check'
> with PyPy even smaler limit of number of open files, although we

s/even/under an even/

s/smaler/lower/ (the adjective modifies "limit", not "number")

Consider dropping the symbolic name (RLIMIT_NOFILE) into that sentence somewhere.

> don't recommend use it because it is much slower than CPython for

s/use it/use of it|to use it/

s/it/${whatever the pronoun refers to}/

> this purpose.

No comments on the diff itself, but I only skimmed it.

Cheers,

Daniel

Re: svn commit: r1884279 - in /subversion/trunk: build/ subversion/tests/cmdline/svntest/

[Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>]

On 2020/12/12 12:33, Daniel Shahaf wrote:
> futatuki@apache.org wrote on Thu, 10 Dec 2020 14:45 +00:00:
<snip all>
Thank you for the review. I've replaced the commit message.

Cheers,
-- 
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>