Robottelo: Proposal to use GitHub "Squash and merge" when merging pull request from fork to SatelliteQE repos

[pgagne]

GitHub give the PR merger the option to either either merge all commits from a pull request into the Upstream Satellite6QE master branch or "Squash and merge" where it automatically selects the commit, squashes them, and merges them to master.

See this example: https://github.com/SatelliteQE/robottelo/commit/75be337b35252e1d5ad96ad6d80d7287d7fdbeab

In the above there was only 1 commit in the PR already, but if this PR has multiple commits it would say the number of commits in the description for the squash and merge option.

This issue is to discuss using this feature as part of the normal PR merging process. This would be easier and cleaner then the developer squashing the commit in his own fork.

[pgagne]

@omaciel thoughts on this?

[rochacbruno]

:+1: I vote to get rid of the "single commit rule" for PRs and always use that "Squash and Merge" button, so we can do as many commits we need in our PR and it will be grouped into single one when merged into master.

Pros:

• We do not need to use the bad -f (force) option on every ammended commit
• We avoid problems with rebase
• Makes contribution process easy
• Keeps history of PR changes
• Easier for reviewer to see if asked changes have been implemented
• Easier to revert feature commits if needed

Cons:

• If github removes the squash and merge in future we will need to move back
• If we move away from github we will need to move back

(BTW: remote possibilities)

[omaciel]

Hi guys,

So I had to read a bit about this to determine if there are any drawbacks from using this feature. I'm still trying to understand what the difference is, if any, of doing a fast-forward merge compared to a 'normal' merge, but I am not getting anywhere. FWIW, I agree with the list of pros that @rochacbruno wrote down, but I do think (and remember, I'm still researching this topic) that squashing things automatically through the Github button will not give us good, clear/concise commit messages.

For example, and not to pick on you @pgagne , but look at the commit message for #3770 and you only see one line:

• "Changed cli/test_capsule.py test_negative_create_with_url to use assertRaisesRegex and check error message", which corresponds to the commit message of the first commit.

Now, look at the individual commit messages for the subsequent commits:

• "fixed test_negative_create_with_url codesytle"
• "Changed indentation in test_negative_create_with_url"
• "Fixed indentation again on line 51"

Now, let's look at a different commit message form this PR:

Skip tests affected by Pulp issue 2242

According to Pulp issue 2242, package signature ID checking is not done
when packages are synced into a repository. Skip the relevant tests.

See: https://pulp.plan.io/issues/2242

So, even though amending an existing commit may seem like a lot of work, it does allow you to put a really nice, clear commit message. I don't think that one could write a really nice commit message for the first commit, and then not have to include anything from any subsequent commit message because they're irrelevant to the overall code change. I do feel that writing a good commit message is just as important as the code that you're trying to merge, and because of this I'd vote against relying on Github to squash commits. The workflow is not really that much work anyhow:

• git commit --amend (and if you're ok with the commit message, you can skip touching it by adding --no-edit to this command)
• git push origin -f

[rochacbruno]

"So, even though amending an existing commit may seem like a lot of work, it does allow you to put a really nice, clear commit message"

@omaciel when we use the squash & merge button, the PR message + each commit message turns to be the squashed message. So if we move to this the only rule would be having a nice, clear and meaninful message in PR (and it is up to the reviewer)

[omaciel]

@rochacbruno correct, if your first commit message is clear and contains the main message, then using squash & merge would display only that one.

[abalakh]

@omaciel i've got used to our 'single commit' approach but i'd vote for using squash & merge button for only one reason - it becomes a nightmare to review really large PRs with current approach. Assume PR is 1000 lines long and you left 20 comments. Then, when author addresses your comments there's no easy way to check what was changed, you have to look through all the 1000 lines and find your comments by unwrapping outdated comments threads. And it becomes even harder if some extra lines of code were introduced and line numbers of comments and current revision do not match.
Another bonus is PR message will become commit message (if I got Bruno correctly, haven't tested it by my own) - on the one side it will result in meaningful PR messages which is good for reviewer, on the other side - PR messages are always in sight and every reviewer will check it (commit messages are 'hidden' and i don't think everyone is checking them for each PR).
Another thing is we can easily update PR message before merging if there's something to add/fix without necessity of pinging the author, amending commit and force pushing updated commit.
Regarding the drawbacks - except of already mentioned - merging PR will no longer be treated as contribution in github statistics (as we won't add merge commit). But i don't think that's any important :smile:

[omaciel]

Some times it is ok to break things down into smaller commits and keep them 'unsquashed'. Some times the individual commits happened between days and the changes don't really amount to anything worth mentioning (for example, I make a major commit today and later on I fix a typo with another commit. For this case, amend would be best imho).

[sghai]

I agreed with @omaciel and @abalakh and here is the compiled version :)

Commit squashing and separate commit really depends on the code or length of PR.

• Commit squashing is needed when you have small chunk of code with multiple commits and each commit is just an update on an existing PR. So my vote is to squash them with a nice message. If you are comfortable with github's Squash & Merge then just use that.
• If you have a very lengthy PR then being a developer and reviewer, I vote to have multiple commits and each commits covers a specific area of feature e.g.
  
  
  
  • commit A: All CRUD functions
  
  
  • commit B: all tests under a specific test class and that covers a whole bunch of tests, otherwise all test classes should fall in one commit.
  
  
  • commit C: If you got a large number of comments, then you may handle that in another commit message just for the reviewer to review the changes, later if needed we can again squash & merge
  
  

In short, if we are using squash & merge just make sure message is nice and meaning full. If you have a very lengthy PR, then PR with separate commits makes more sense.

[renzon]

@omaciel: I was curiou about fast-forward and got it after seen the image explaining it on this article: https://ariya.io/2013/09/fast-forward-git-merge.

[elyezer]

Squash and merge is good when the PR have one commit with a really good message, when that happens and the button is pressed the commit message is retained and defined on the squashed commit.

But when have multiple commits who is merging needs to provide an useful message. But since who is merge is not who have authored the commit/PR then is harder for him to express what that change really means.

My concern is, use squash and merge but the author must have the responsibility of creating a PR with only one commit with a good commit message. That means that the author will need to amend and force push, that is easy, isn't it?

When multiple commits are needed the old merge method can be used and if the merge commit is not wanted then the way to go is using the git on CLI. And yes this is not what happens most of the time but is something that is needed and there are cases where multiple commits are needed.

Using squash and merge creates a plain git history which makes a lot easy to follow what have happened. Check Pulp Smash [1] and Betelgeuse [2] history, specially the commit when a new version is released, having a plain history helps on automating release process and use git short-log for the changelog information. You will see that most of the commit only have the author and some have the commiter as well, that happens because on Betelgeuse and Pulp Smash almost all commit are merged manually using the command line, that adds more work but give the mentioned benefits in return.

[1] https://github.com/PulpQE/pulp-smash/commits/master

[2] https://github.com/SatelliteQE/betelgeuse/commits/master

[sghai]

@SatelliteQE/quality-engineers : Do we want to keep it open ? or Shall I close this out ? Anyway we can refer or point it someone if needed.

[sthirugn]

what is the decision from the mojo poll? :)

[rplevka]

@sthirugn i believe the result was to support multi-commit pull requests in combination with a common sense.

[rochacbruno]

I'll write a CONTRIBUTING.md file ASAP (I have a draft) then I'll include those notes there, so we can close this.

[elyezer]

Use the new Rebase and Marge as default. That is the way to go.

It will deal with both, single and multiple commits PRs, will ensure a good history by rebasing before merging and will require the author to fix conflicts if needed.

[abalakh]

Closing this as we agreed to use "Squash and merge" as our default approach (except the cases when we really need to merge multiple commits separately)