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

[Christophe de Dinechin]

> On 8 Feb 2018, at 11:01, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> 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
> 
> OT: Not only trailing, also tabs for instance.
> 
>> - We nack patches that fix trailing whitespace on their own
> 
> Not saying that, I'm saying is not black and white.

Granted, you are not saying it, Marc-André is… But if one nacks patches with only whitespace and the other nacks patches where whitespace are incidental, we still have a problem.

> 
>> - We nack patches that include incidental whitespace fixes
>> 
> 
> Not saying that either, I'm saying that if the patch says "fix this"
> and is also fixing spaces in unrelated hunks should be split.

Which is exactly what I am saying, isn’t it?

> 
>> Sorry, I can’t agree with that, it’s inconsistent.
>> 
>> 
>> Christophe
>> 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel