[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [DISCUSS] new way of github working

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


From: Rafael Weingärtner <rafaelweingartner@xxxxxxxxx>
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.Tutkowski@xxxxxxxxxx>

> 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" <mail@xxxxxxxxxxxxx> 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

53 Chandos Place, Covent Garden, London  WC2N 4HSUK