-1 for squashed commits on refactor/new features work. ;) If we have, let’s say, 2 commits, I think that would be fine to squash. But if someone has the courage to refactor 7 thousand lines of code on +30 commits, by squashing would difficult the review
In addition, if those 2 commits are part of 2 different bugs, I would actually have them in different PRs. The best case scenario: 1 commit to fix the bug and 1 commit for the unit tests added. If that’s about refactor/new feature, I wouldn’t bother about squashing. My 2 cents. Cheers, Wilder > On 18 May 2015, at 09:48, Stephen Turner <stephen.tur...@citrix.com> wrote: > > 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
