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

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

Stephane Chazelas wrote on Wed, Nov 25, 2020 at 06:44:43 +0000:
> 2020-11-25 00:35:12 +0000, Daniel Shahaf:
> [...]
> > > A few issues addressed:
> > > 
> > 
> > This patch is over 1k lines long and makes multiple independent changes.
> > 
> > Could you please split this into one patch per logical change?  That
> > would make it easier to review, both now, and in the future should
> > a regression be bisected to it.
> [...]
> 
> Hi Daniel,
> 
> thanks for having looking into it.
> 
> They're not independent, except maybe for the "limit"/"unlimit"
> now back in csh which is a 5 code line change.
> 

*nod*

> Chunks of the code and doc have been rewritten to address
> those. Separating out the changes would mean rewriting the code
> several times which would be more effort for me and reviewers,
> mostly wasted if we're only keeping the last iteration.

How so?  In v2 2/2's log message I see, for starters, several bullet
points about documentation-only bugs, and in the .c diff the
showlimitvalue() stands out as _prima facie_ independent of the rest.

I don't see how _splitting_ those involves a _rewrite_.  Splitting is
fairly easy,¹ and I don't think it'd be a "waste" of time.  On the
contrary, I think it'd be a O(1) effort for O(N) gain (split it once and
then, even years from now, anyone who reviews the history will have a
easier time).  Moreover, smaller diff can help ensure that each change
is tested and help catch unintentional behaviour changes (always a risk
in refactorings).

I do agree that a single logical change that happens to fix a dozen
interdependent bugs needn't be split, but I don't see how that's the
case here.  Enlighten me ☺

> But maybe before looking in detail at the code, we can discuss
> whether the change in API makes sense.

Seems to make sense, going by the log message.  Really, the only part
that jumped out at me was changing the behaviour of "KB" on input from
error to "1000 bytes".

Cheers,

Daniel

¹ Stephane: I elaborated offlist recently on splitting.