multi-touch-dev team mailing list archive

[Peter Hutterer <peter.hutterer@xxxxxxxxx>]

On Mon, Mar 26, 2012 at 03:26:32PM -0700, Chase Douglas wrote:
> On 03/26/2012 03:15 PM, Peter Hutterer wrote:
> > On Mon, Mar 26, 2012 at 02:09:13PM -0700, Chase Douglas wrote:
> >> Hi all,
> >>
> >> As the uTouch project is growing larger and larger, we are now starting
> >> to hit issues where code formatting may stray from the unity style
> >> guidelines. When we develop changes, we might see incorrect style in
> >> surrounding code. Should we have a policy that requires code format
> >> fixes to be contained within separate commits? This has come up as an
> >> issue in a recent merge proposal, and we haven't really discussed it
> >> previously.
> >>
> >> I have two concerns with requiring separate commits:
> >>
> >> * It would be nicer to always have it split out into separate commits,
> >> but the burden for maintaining those commits usually means people don't
> >> bother with them, even when it's within the same area of code. In other
> >> words, I would rather allow for a few format fixes mixed in code changes
> >> than potentially forgo format fixing contributions because of the extra
> >> effort involved for trivial gain.
> >>
> >> * Git interactive rebase makes it very easy to split comment and code
> >> changes using "git add -p" and splitting, amending, and rebasing
> >> previous commits. Unfortunately, Bzr was designed with a different idea
> >> of how software development should be done, and does not readily allow
> >> for editing, amending, and rebasing previous commits. The best way to
> >> make it work with bzr is using bzr-pipeline, but it is not as safe as
> >> git rebasing (i.e. it might eat your work if you use it wrong), and it
> >> doesn't handle rebasing changes out of one commit and into a previous
> >> commit. I don't think it is practical to require separate commits for
> >> style changes with the current bzr tools.
> >>
> >> If you have input to provide, please do so asap, hopefully within the
> >> next day.
> > 
> > run an automated tool across the whole code-base. that way you only get one
> > reproducible commit per package. After that, coding discipline and patch
> > review should keep you there.
> 
> This particular issue was due to one merge while we were unclear what
> the style was for a few places. For the most part we are in alignment,
> so I don't think we need to spend time running format tools right now.
> It's merely a question of how to handle fixing little one-offs that
> creep in from time to time.

In that case, I strongly recommend separate commits unless the whitespace
can be fixed by that commit directly (i.e. the actual fix is in the same
line). whitespace patches are a no-brainer to review. If you fix misc
whitespace issues within another commit and you then have to revert that
commit, future patches that build on those unrelated hunks may have merge
conflicts. The only exception here is if they are in the same natural hunk
(i.e. the whitespace change doesn't make the hunk [significantly?] larger)

Cheers,
  Peter