Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebase and ideally Squash instead of Merge for Pull Requests #2

Open
coderbyheart opened this issue Aug 30, 2018 · 7 comments
Open

Comments

@coderbyheart
Copy link
Owner

coderbyheart commented Aug 30, 2018

You should rebase a branch against the main branch before "merging" it, so that there is no merge commit.

While some consider rebasing to be an act of destroying the true history of a software project, I think this is doing actually the opposite.

What matters in terms of history is that we preserve the time when a set of changes became effective for the rest of the team. As long as a change is in an isolated branch on which only one developer works on, it cannot produce bugs for others (developers or users) thus it did not have the chance yet to make history so to speak.

By rebasing a PR against the main branch just before merging it will tell the real story: the changes will appear in this branch in the order they became active and this accurately reflects the time when others were made aware of these changes and also the time from when to entire codebase needed to deal with the changes introduced by them.

Squash merging

Once a PR is done and has been approved, I like to squash all commits into one commit with a good and concise explanation. That one commit should ideally look like the PR itself: a PR on GitHub has a title and a decription, which explains the motivation for the change and lists what needs to be done (simple future). Turning this into a commit means telling the story of what the commit now does (in present tense).

I consider squashing a PR into one commit to main an even better way to contribute changes to the main branch. I often have man small commits in a PR, which add tests, implement features, refactor. Having all these in the main branch does not really help other developers to quickly get the high-level summary of a change.

Cleaning up your commits for a PR

This does not mean that all PRs should only have one commit (although they rarely should have many) it makes reviewing a PR easier if all the commits in a PR are condensed down to a set of distinct commit messages, even if they get squashed when being merged anyway. See this as an opportunity to describe to the reviewer what you did when implementing the change. It also enables a reviewer to view changes in a bigger PR with a context specific to a commit.

Re-tell the story of your PR

Creating a clean set of distinct commits in a PR requires that atomic commits were made in the first place. It's a good habit while developing to commit early and commit often: create many small commits, which you then later can combine into bigger ones. A great helper to achieve this is using git add -p, which will go through all current unstaged changes and let you decide which changed segment in a file to stage. git add -p allows you to create multiple commits from a situation where you have applied different changes while developing (a new feature here, but also a bug fix, and maybe some formatting, or changes to the CI configuration).

Tip: when incorporating PR feedback into these commits, git-absorb can help with that.

What if you have unrelated changes in the same commit

You might encounter a situation where you accidentally combined unrelated changes in one commit. There is a way to split up these commits, but there is no reverse version of squash, it has to be done manually. See the Splitting a commit section in the official Git documentation. Here again, when in rebase, git add -p is your friend.

Resources

@coderbyheart coderbyheart changed the title Rebase instead of Merge for PRs Rebase and ideally Squash instead of Merge for PRs Dec 2, 2018
@coderbyheart coderbyheart changed the title Rebase and ideally Squash instead of Merge for PRs Rebase and ideally Squash instead of Merge for Pull Requests Dec 2, 2018
@ewoks
Copy link

ewoks commented Dec 2, 2018

Having all these in the main branch

what is main branch here? something like develop branch where feature branches are merged into? It looks to me that something is a little bit off in this explanation.

All these small commits that are part of PR aren't cherry picked from feature branch to appear again on the develop as separate commits, no? So other developers see just merged PR as one commit consisting of all of these atomic changes.

I really don't understand what is advantage of squashing everything together in this case, but I see big disadvantage that all the work of conscious make of atomic commits just got flushed away

@Wrent
Copy link

Wrent commented Dec 2, 2018

Nice description! Btw. there is double "when" in one of the sentences.

@gregturn
Copy link

gregturn commented Dec 2, 2018

I really don't understand what is advantage of squashing everything together in this case, but I see big disadvantage that all the work of conscious make of atomic commits just got flushed away

But it didn’t get flushed away. All the work still lives. During the squashing part, you polish and edit. All the “hack” commits can be simply dropped while the non-hacks are revised and properly ordered.

What does it matter what order the feature branch was done? It’s like reading a novel and feeling you are at a loss because you can’t see the earlier drafts. All those drafts end up getting “squashed” into the final product.

@ewoks
Copy link

ewoks commented Dec 2, 2018

Maybe I just didn't understand it right. Do you or can you point me to example repository? Even simple drawing of before vs after would be helpful. Thanks

@smolak
Copy link

smolak commented Dec 3, 2018

@ewoks I think you will find it in the article I wrote a while ago: https://medium.com/@jacek.smolak/how-do-i-commit-59bf6de2b264

In the latter part there are commits mentioned that are fixes and how I go about them.

@ewoks
Copy link

ewoks commented Dec 11, 2018

@smolak thanks for the link and article.

For me and most of the teams I lately worked with, GIT Branching Model looks and feels (in practise) much simpler than rewriting history and making documentation of git repository.
TL;DR every feature has its own branch, you as the feature owner commit in order you want and send pull request, team reviews and at the end feature branch will be merged to develop branch. Develop is always buildable and runnable and it should have deliverable product.

However, I understand that lack of branching strategy would require cleaning things up at least a bit before considering it complete. Huge time investment IMHO but everybody decides with the team.

Still it feels that I am missing some important point, cause what is described in your article looks to me like a huge overkill in comparing with how me and team is doing it at the moment, that's why I hoped for repository example.

@coderbyheart
Copy link
Owner Author

coderbyheart commented Dec 12, 2018

I think the article provided by @smolak gives a good explanation of how it would look like in a real repo.

But everybody learns differently so adding a repo with an example (feature branch with multiple commits, main branch with squashed commits) is a good idea.

It's also worth mentioning that I lean heavily towards short lived branches #16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants