Hacker News Reader

Comment by chenhan | Hacker News Reader

This is a good article that covers how to communicate in a pull request. I think there are two other essentials in making good pull requests.

1. Good commit messages. Follow this guide: https://chris.beams.io/posts/git-commit/

2. Following the pull request workflow correctly. Follow this guide: https://github.com/susam/gitpr

Writing good and consistent commit messages make the commit log easy to read and search.

Pull request workflow is equally important. It is kind of a rules of engagement that co-developers follow to keep working on their stuff concurrently. It keeps unnecessary merges to minimum and keeps the commit history clean.

By following these two things, you are not only going to be nice to your co-developers but you are going to be nice to your managers and release departments too who could look at your commit log and figure out what bugs were fixed and what features were released.

View on HN

I will elaborate why I think the two points in my comment above are important.

Git Rebase, Git bisect and other operations display the commit summary line when they get stuck with merge conflicts or find an issue, so I find good commit summary lines very helpful during those operations. Without good commit messages, resolving issues during those operations can get confusing. This is one of the reasons why writing good commit messages are important.

I see many developers working on their PR branch and constantly merging new commits from master into the PR branch as the master keeps moving ahead. It creates a big mess of merges in both directions-- (A) from master to PR branch during development and (B) later again from PR branch to master when the PR gets merged. The first set of merges from master to PR branch are totally unnecessary. It adds nothing meaningful to the commit history. They are just extra commits to scroll through while looking at git log. Every time the master branch moves ahead, just rebase your PR branch on master. The commit history remains clean and minimal. A good PR workflow teaches you that.

Having said that, merge commits from PR branch to master are totally fine. They do add something meaningful. They show the point at which a PR was merged into the main project.

If you share a development branch with someone, then you should prefer merging over rebasing because rebasing changes rewrites the commit history, causing the locally checked out branches among collaborators to disagree. You never know when someone will need to take your branch to develop on, so generally it’s a good idea to merge over rebase. When master is merged into your PR branch, the fork point is forwarded to the last master commit, so you shouldn’t have any trouble merging back into master.

I think the OPs concern was more with the commit history in the PR being polluted with merge commits.

I do agree that merge commits can obfuscate history somewhat. However, I agree with dimes that its better to not rewrite history just to keep the commit history clean... the cleanliness is not worth the price for inability to collaborate effectively.

Also, any professional code review tool will not let merge commits affect your review. Highly recommend reviewable.io for this.

> If you share a development branch with someone, then you should prefer merging over rebasing

Your parent comment is suggesting rebase only for pulling latest changes in master into your pull request.

For merging someone's pull request to the team's master, sure use a merge commit.

But if you are working on a pull request and while you are working on it, the team's master gets updated and now you want to base your work on the recent master, by all means, use git rebase. That is what it is meant for, to rebase your work on another work. It's in the name itself.

Right tool for the right job.

Using a rebase to update from master will cause conflicts for anyone else working on the PR branch. To understand why, imagine a developer branches from master at commit A, and then creates two commits on the branch B and C. In the meantime, master gets updated with commits D and E. Rebasing in this situation, results in a branch that looks like A, D, E, F, G, where F and G contain the same content as B and C, but are now tracked under different hashes. If a collaborator has this branch, git will report the local branch has two different commits (B and C) and that the remote branch has two different commits (F and G). Doing a typical “git pull” in this situation will lead to merge conflicts in everything touched by commits B and C. You can work around this specific problem by using git pull —rebase. However, git works best when the remote history is immutable. If you have a specific commit in your PR that you want someone to look at, you send them a link to it using the commit hash. Once you rebase, that commit hash will no longer point to anything. Rebasing is useful for completely changing the branch your PR branch is cut from. But once you rebase from one base branch to another, you should then continue to merge from the new base branch.

> Using a rebase to update from master will cause conflicts for anyone else working on the PR branch.

The best way to handle that is to run git stash save, git fetch origin, then run git rebase @{u} to rebase your local branch on top of the new upstream branch. Then run git stash pop to apply any uncommitted changes.

> Doing a typical “git pull” in this situation will lead to merge conflicts

This is why I never use git pull, and always run git fetch instead. This allows me decide whether I want to merge the upstream changes, rebase on top of them, or just run git reset --hard to just use the upstream branch as is.

> Once you rebase, that commit hash will no longer point to anything.

Unless git gc deleted the dangling commits (along associated trees and boobs), the hash value will still show the commit. In fact, it's possible to show a diff from that commit to the corresponding commit in the rebased branch.

I partially agree. While I use git rebase on my branches, if somebody refuses to do it because of whatever reason and has on their branch a bunch of merge commits (usually `develop` into their branch) due to a stale PR, or a bunch of typo fixes or, worst of all, a bunch of emoji commits - so be it. BUT when they merge it back into develop/master I expect all that silliness to be squashed to one or more commits with clear commit messages.

It boils down to: what you are doing on your own branches is your thing, but when interacting with shared ones do so with some professionalism and decency.

I think rebase is fine, you just need to synchronise with the people working with you on the branch. There shouldn't be more than 1 or 2, otherwise you're probably doing something wrong.

Also, I only rebase when it makes sense in that case, you need something from master or you're about to merge back into master, so disruption should be minimal.

Reply on news.ycombinator.com