I don't like squashed commits either. Merging a branch on github lets the reviewer switch between seeing the overall diff, and seeing the individual commits' diffs. (And to answer the other point, also allows the author to make a pull request comment, different from any of the individual commits' comments).
When reviewing on github, I normally start with the overall diff, but I like to be able to switch into the individual ones too, to understand the motivation for particular parts separately. So I think you get the best of both worlds that way. -- Stephen Turner -----Original Message----- From: Rajani Karuturi [mailto:raj...@apache.org] Sent: 16 May 2015 03:38 To: dev@cloudstack.apache.org Subject: Re: Preparing for 4.6 -1 for squashed commits. I agree to what Daan said. Feature branch merge is more convenient than squashed single commit. If it was a small feature which involved single dev squashing is still ok. But, a big no when it comes to big feature/refactoring involving effort from multiple people and multiple reviews. On Sat, May 16, 2015 at 3:21 AM, Mike Tutkowski < mike.tutkow...@solidfire.com> wrote: Those comments may or may not be useful anymore. What they describe may have been superseded by a subsequent commit. On Fri, May 15, 2015 at 3:12 PM, Daan Hoogland <daan.hoogl...@gmail.com <javascript:;>> wrote: > those comments will then have to be squashed s well, to this i put a -1. If > those comments where usefull at review-time they will be usefull in > the future as well. > > Op vr 15 mei 2015 om 19:29 schreef Marcus <shadow...@gmail.com <javascript:;>>: > > > I'm not sure it is any different. Either you have a giant block of > > code that represents a bunch of little commits, or a giant block > > that is one commit. We don't want to merge little chunks to master > > that don't fully implement the feature. > > > > To the extent that features and goals can be split up, yes, we don't want > > those features squashed together, or even submitted together, > > squashed or > > not. An example of this is in combining formatting/syntax fixes with > > functional changes, I have tried to review such pull requests and it > > is very difficult to see what is going on in a 1k line request when > > 2/3 of > the > > changes are just reformatting noise. > > > > Ideally the code is reviewed at the commit level as each small > > commit > goes > > from the developer's workstation to the dev branch, but I don't see > > a way > > around reviewing the same amount of code in a pull request that > represents > > multiple small commits vs one squashed commit. You do get more visibility > > into the comments, though. > > > > I suppose a way to get both would be to branch master, do a pull > > request from your dev branch to that branch, at which point it is > > reviewed, then squash merge that back into master. > > On May 15, 2015 8:55 AM, "Daan Hoogland" <daan.hoogl...@gmail.com <javascript:;>> > wrote: > > > > > Sebastien, I don't think commits in a big feature should be squashed. > It > > > hinders review if to big chunks are submitted at once. > > > > > > Op vr 15 mei 2015 om 11:27 schreef Sebastien Goasguen < > run...@gmail.com <javascript:;> > > >: > > > > > > > Folks, > > > > > > > > As we prepare to try a new process for 4.6 release it would be > > > > nice > to > > > > start paying attention to master. > > > > > > > > - Good commit messages > > > > - Reference to a JIRA bug > > > > - Squashing commits ( cc/ wilder :)) > > > > > > > > While you can apply patches directly, good practice is to apply > > > > the > > patch > > > > to a branch and then merge that branch into master. > > > > > > > > Before we start enforcing some of these things more strongly, > > > > please > > keep > > > > it in mind. > > > > > > > > thanks, > > > > > > > > -sebastien > > > > > > -- *Mike Tutkowski* *Senior CloudStack Developer, SolidFire Inc.* e: mike.tutkow...@solidfire.com <javascript:;> o: 303.746.7302 Advancing the way the world uses the cloud <http://solidfire.com/solution/overview/?video=play>*™* -- ~Rajani Sent from my Windows Phone
