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

[Frediano Ziglio]

> 
> > 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.

> - 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.

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

Frediano