[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v4 2/5] qcow2: Assign the L2 cache relatively to image size
Date: Thu, 9 Aug 2018 12:37:28 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]