Reformatting code

[Andrew Bartlett]

On Sun, 2011-07-24 at 23:26 +0200, Jelmer Vernooij wrote:
> On Sun, 2011-07-24 at 17:14 -0400, simo wrote:
> > On Sun, 2011-07-24 at 22:38 +0200, Volker Lendecke wrote:
> > > On Sun, Jul 24, 2011 at 04:08:17PM +0200, Jelmer Vernooij wrote:
> > > > >>On Fri, Jul 22, 2011 at 10:04:06AM +0200, Stefan (metze) Metzmacher wrote:
> > > > >>>http://lists.samba.org/archive/samba-technical/2011-June/078234.html
> > > > >>Yes, I know about that one. Has there been a resolution? I
> > > > >>think even back then I replied with some doubtful mail. Has
> > > > >>there been a definitive decision that this change is to be
> > > > >>done?
> > > > >No, but most of us thought it is a good idea to switch to
> > > > >just one style of using bool values.
> > > > Being consistent in the way we notate bools is definitely a good thing.
> > > > 
> > > > That said, there is a difference between fixing up old-style bools in 
> > > > code you are changing anyway, and actively reformatting code without 
> > > > making any other changes.
> > > > 
> > > > The latter makes it harder to track down its origin later, and adds 
> > > > noise to our revision history: it makes things like "git annotate" or 
> > > > "git log" harder to use. It's not worth the slight cosmetic improvement IMO.
> > > 
> > > That's exactly my point. If I change code lines anyway for
> > > other reasons, then I also change False->false. But just for
> > > the purpose of that change I leave it. But the two of us
> > > seem alone with this opinion, everybody else sees it
> > > differently.
> > git blame can be given a commit-id to get the previous history, so if a
> > "reformat" commit gets in your way you can always see what happened
> > earlier than that.
> Yes, but it's one or more extra levels of indirection that require noting the
> problematic id and running "git annotate" again. I don't think the
> slight cosmetic improvement is worth that cost. 
> 
> > If cluttering git blame is the only reason I am also in the camp that
> > doesn't care as git can easily cope with that.
> > 
> > Also if these commits are properly labeled with the word "reformat" in
> > the title they are not really annoying to me, they too can be easily
> > skipped or simply ignored.
> If we think that reformat-only commits are a good idea, can we just do a
> one time thing where we fix the entire codebase to conform to the coding
> style? That way there is at most one commit for each file that fixes the
> formatting, rather than one that fixes the bool capitalization, one that
> fixes the int types, one that fixes the indentation, etc.

I think the separate commits for the type of change might help us avoid
errors (because change change is a little easier to review if it changes
just one thing), but I certainly agree we should do this once, do it
over the entire tree and get it over and down with.  I'm not in favour
of splitting it per-file (hundreds of commits with the same purpose and
commit message sounds like CVS, not a real version control system).  

I also see a line between pure reformatting (essentially running intent
over a C file) and fixing up our types.  

The type changes are more than reformatting - just like the TALLOC_
macros I removed earlier, these types are not supported in 2 of the 3
primary divisions of our codebase - the top level and source4/.  My
reason for wanting to see these changed is that I think we should have a
consistent type system, and so moving a file to the top level should be
just 'git mv', possibly with some extra headers, not 'git mv, fix up
types'

As long as the proposed changes are agreed here in concept, then I it
would be really good to get these type changes into the tree with
minimal delay.   

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org