Re: [Qemu-block] [PATCH v4 2/5] qcow2: Assign the L2 cache relatively to image size

[Eric Blake]

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).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org