Re: [Qemu-devel] [PATCH v2] Move File operations to qemu-file.c

[Blue Swirl]

On Thu, Feb 14, 2013 at 10:09 PM, Anthony Liguori <address@hidden> wrote:
> Blue Swirl <address@hidden> writes:
>
>> On Thu, Feb 14, 2013 at 2:34 AM, Anthony Liguori <address@hidden> wrote:
>>> Joel Schopp <address@hidden> writes:
>>>
>>>>>> +    if(popen_file == NULL) {
>>>>>
>>>>> Please make a preparatory patch which adds missing spaces between 'if'
>>>>> statements and '('.
>>>>
>>>> I'll do a preparatory style cleanup patch of existing code if it is
>>>> deemed necessary by the maintainers, but I don't think it's a good
>>>> idea.
>>>
>>> I basically hate checkpatch :-)  There's no need to do a style cleanup,
>>> it's just going to confuse gits move detection and screw up merging.  In
>>> this case, it's such a trivial thing too.
>>
>> Either we do code cleanups when possible or we forget about
>> CODING_STYLE and checkpatch.pl mess.
>
> We should never force people to clean up coding style in code they
> aren't touching.

In this case, Joel is definitely touching the code since it's not a
pure rename. We force a lot of things to submitters before a patch is
applied, this is on the trivial end of the scale.

>
>> The whole point of having a separate patch to do the cleanup is to
>> keep git move detection happy.
>>
>>>
>>> I disabled the automated checkpatch bot because it got too annoying.  It
>>> throws way too many false positives or annoying nits that shouldn't keep
>>> us from merging useful code.
>>
>> Those 'nits' improve the code base.
>
> A change that only does coding style makes it harder to trouble shoot
> problems because there's an extra step of walking past the formating
> change to find the real source of the problem.
>
> I spend a lot of time chasing problems and having lots of little
> "improvements" just makes that more difficult.

But if that would be the highest priority, the best way to help that
would be to never make any changes to improve, only fix bugs.

>
> I value debuggability a lot more than whether a line of code has 'if('
> or 'if ('.
>
>> It means that a patch to fix one
>> thing must also improve the CODING_STYLE while at it. The alternative
>> to enforcing this is to do codebase cleanups separately, for example
>> in form of global reformatting and flag days.
>>
>> We don't have many written rules and nobody seems to want to follow
>> them.
>
> I'm okay with enforcing Coding Style on *new* code but moving code from
> one file to another is *not* new code.

Maybe, but it's still an opportunity to improve style.

>
> Regards,
>
> Anthony Liguori