On 8/9/18 8:37 PM, Eric Blake wrote:
On 08/09/2018 11:46 AM, Leonid Bloch wrote:
There are no functional changes, why do you need to change the
indentation here?
It's in the "immediate area (few lines) of the lines [I'm] changing".
But there's no need to change those lines unless there's an obvious
mistake. In this case it's just a matter of style so they just add noise
to the patch.
Again, it just looks nicer, more readable, compliant to the generally
accepted style, and right next to the functional changes. It's a style
improvement which is in the immediate vicinity of the functional
improvements. I made another one, you must have seen it already, in v5.
Look, it just looks better. It's possible to make another patch for
these cosmetic changes, but is it worth it when they are right next to
the functional changes? It's a bit of noise in the patch, versus noise
in the Git history.
Patch splitting is an art form. But it IS easier to review two patches
(one that fixes style but has no semantic change, and one that does
semantic change in as few lines as possible) than to review one (that
mixes both steps at once). The more things you do in a single patch,
the more likely you were to be better off by having split it into
independent patches.
A longer git history is not a problem. Our bottleneck is reviewer time,
and everything you can do to make life easier for reviewers is a net win
in overall time spent on the project. And splitting two distinct changes
IS worthwhile, especially when a reviewer has requested that split.
Along those lines, I'll also comment that I've seen Berto request that
you consider splitting even the functional part of this patch into two
pieces - one raising the default value, and the other fixing things to
use only what is needed rather than the full specified length when the
specified/default length is larger than necessary. It's not a hard
split, and while you've continued to argue against the split, I tend to
agree that doing the two parts separately makes the series a bit easier
to backport to other stable branches (for example, if a distro wants to
change to yet a different default value, but still use your patch that
changes to not overallocate when the specified/default is larger than
needed).
Hi Eric,
I agree with your arguments here, splitting the cosmetic fixes being the
reviewer's request, and splitting the size setting for the reason that
you have mentioned above.
Thanks! Sending v6.
Leonid.
