[Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

[Christophe de Dinechin]

> On 8 Feb 2018, at 10:52, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote:
>>> Hey,
>>> 
>>> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
>>>> On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
>>>>>> 
>>>>>> From: Christophe de Dinechin <dinechin at redhat.com>
>>>>>> 
>>>>>> The objective of these guidelines is that:
>>>>>> - We avoid introducing new warnings
>>>>>> - We know how to fix old ones
>>>>>> - We don't have to isolate whitespace changes when submitting
>>>>>> patches,
>>>>>>  i.e. someone who use tools that automatically strip whitespaces and
>>>>>>  therefore "repairs" earlier errors should not be punished for it.
>>>>> 
>>>>> Sorry, I don't agree with the automatic tool, patches should not
>>>>> contain extra changes unless they fix space changes while changing
>>>>> these lines for other reasons.
>>>>> I'll personally accept single patches fixing the spaces.
>>>> 
>>>> I'm with Frediano here. If you want to automatically fix whitespace
>>>> errors, you can do it in a separate commit without much effort?
>>>> 
>>>> I can also go right now and fix all trailing whitespaces with a bash
>>>> oneliner, submit the patch and we have a moot point here? :)
>>> 
>>> Well, it was nacked before :)
>>> 
>>> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html
>> 
>> What is the reasoning behind the nack?
>> 
>> I'd rather have a style-only commit than to fight the bad indentation
>> manually and possibly have unrelated whitespace changes in another
>> patch...
>> 
>> By the way, mine got acked :D
>> 
>> https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html
>> 
>> Lukas
>> 
> 
> Depends on many cases. You don't want spurious changes to make harder to
> look at the history for instance (that is a point for Nack).
> The patch is not fixing anything or adding new feature (another for Nack).
> On the other hand applied to code not changed for a long period (where is
> unlikely to have to dig the history) or code with small history (where
> is faster to skip in any case) changes.
> Being style it depends also on personal opinions.

You can’t have it both ways. Here, you are simultaneously saying:

- We don’t want trailing whitespace
- We nack patches that fix trailing whitespace on their own
- We nack patches that include incidental whitespace fixes

Sorry, I can’t agree with that, it’s inconsistent.


Christophe


> 
> Frediano