My preference is to not squash commits when a PR is created, I do prefer squash merge of PR. Also, instead of a rule I would prefer this to be a general guidelines, as there may be cases where it may be valid to have few commits in which case I will prefer doing a rebase+merge (via github or otherwise). As an example, if a PR is worked on by several people (say it's a big change, or someone carries forward an old PR whose primary author is not responding), I can prefer to have commits melded by author (to give credit to authors involved) or component (say on test, scripts/infra, java packages,).
I think during course of PR review, not squashing commits helps reviewers to track what got changed from the initial PR. Merging of PR with an explicit merge commit is something I don't like as it messes up the git history. - Rohit <https://cloudstack.apache.org> ________________________________ From: Rafael Weingärtner <rafaelweingart...@gmail.com> Sent: Wednesday, May 2, 2018 4:53:41 PM To: dev Subject: Re: [DISCUSS] new way of github working I think we are on the same page here. There is only one thing that is different from what we are doing in PRs. > Before you create a PR, squash all applicable commits to make it more > readable for reviewers > I would suggest not squashing everything before creating the PR. I mean, do the following when coding: - disable your auto formatter (in Eclipse you can just disable the save actions) if you are going to change a class that might need formatting - do the changes - create a commit for the main changes of the PR - enable the auto formatter and formatting the change files - create a commit for formatting and cleanup changes - then create the PR Separating formatting/cleanups commits makes it easier for reviewers to evaluate changes. On Tue, May 1, 2018 at 3:53 PM, Tutkowski, Mike <mike.tutkow...@netapp.com> wrote: > Hi everyone, > > We had a good conversation going here. Maybe we can continue it, get some > level of reasonable consensus, and implement it (if, in fact, the consensus > is a change from what we currently have). > > My suggested approach is the following: > > Before you create a PR, squash all applicable commits to make it more > readable for reviewers. Once reviews start coming in and you start making > changes, push new commits on top of the prior ones (do not squash at this > point). This will make it easier for reviewers to confirm that you and they > are on the same page with regards to what was changed. When you need to > draw in changes from the base branch, rebase your commits on top of it. > When the PR is given a LGTM by 2+ reviewers and passed the necessary > regression tests, it should be squashed and then merged. I see the > evolution of commits during the life of the PR as a temporary sandbox of > history that is no longer required once the PR has been completed. > > I think that process should cover the vast majority of our PRs. > > There are usually some exceptions to the rule, however. When this happens, > discuss your situation with the reviewers and bring any concerns to the > mailing list before deviating from the standard process. > > Thoughts? > Mike > > On 1/15/18, 1:47 PM, "Rene Moser" <m...@renemoser.net> wrote: > > > > On 01/15/2018 09:06 PM, Rafael Weingärtner wrote: > > Daan, > > > > Now that master is open for merges again, can we get a feedback > here? It > > might be interesting to find a consensus and a standardize way of > working > > for everybody before we start merging things in master again … > > +1 to allow merge commits on master branch to keep history of a series > of patches when they help to understand the change. > > René > > > > > -- Rafael Weingärtner rohit.ya...@shapeblue.com www.shapeblue.com 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue
