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

[Blue Swirl]

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.

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. 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 haven't applied this patch because we're in freeze for the 1.4
> release.
>
> Regards,
>
> Anthony Liguori
>
>>   The patch as it stands now simply moves existing code to another file
>> and thus is pretty safe.  Adding a preparatory patch to reformat the
>> code is easy to mess up and raises the chances of introducing a regression.
>>
>> Why not just submit patches to clean up coding style for the entire code
>> base independent of any refactoring?
>>
>> When I originally wrote checkpatch.pl it was with the intention of
>> avoiding arguments over coding style.  I see that I missed a corner case
>> by having it not notice code moves as a special case to ignore.
>>
>> -Joel