cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wilder Rodrigues <WRodrig...@schubergphilis.com>
Subject Re: Preparing for 4.6
Date Mon, 18 May 2015 08:04:04 GMT
-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.Turner@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:rajani@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.tutkowski@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.hoogland@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 <shadowsor@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.hoogland@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 <
>> runseb@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.tutkowski@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

Mime
View raw message