How to write the perfect pull request (2015) | Hacker News Reader

How to write the perfect pull request (2015)

github.blog

174 pointsxiaodai39 comments

> Ask, don’t tell. (“What do you think about trying…?” rather than “Don’t do…”)

When I led a team of 6 people, I did the opposite. The guideline was to always use the imperative form if you knew there was a better way, especially if there's a precedent or it's a written rule in a guideline, but even for opinionated things.

It resulted in clear debate in PRs, ZERO personal conflicts, everything was very civil and productive.

I'm just mentioning this as a personal anecdote, not suggesting that there's a right or wrong way. The reason this worked in my team is because we were, and still are - a team. They're still doing it like that even though I'm no longer involved in day to day.

> The guideline was to always use the imperative form if you knew there was a better way, especially if there's a precedent or it's a written rule in a guideline, but even for opinionated things.

Same here. We used imperative form too and never had any problems. All developers were matured enough to not take comments about technical stuff personally. I think as long as we are being direct and polite in the reviews, there should be no issues.

I agree.

It shouldn't disguise as a question if you're not really inviting a debate, doesn't feel honest IMO. When the imperative form is used the reader will often assume you know something they don't, and only argue when they see a problem or want to validate the assumption.

I believe the more the comments (annotations?) are about the code itself ("This should do X", "This is not doing Y") rather than a back-and-forth conversation ("What do you think about X?", "Have you thought about X?"), the less personal it feels, and the more the ego gets out of the way.

Same thing for denying PRs with "request changes" - I've had a few people say they avoid doing it because it feels aggressive, but knowing there's a problem and still letting it into production (and then have the PR author fix a much more costly mistake) isn't honest or professional.

> the less personal it feels, and the more the ego gets out of the way.

That's why I usually use "We should do this" instead of "You should do this", and "Our code" not "Your code" because we are a team and we do this together.

I have no problem denying PRs or if somebody do this with mine. If we can't get to the same page, we just call the tech lead to break up the tie.

> The guideline was to always use the imperative form

Has this team been distributed in some way or did it consist of people that have regular in-person interaction?

The entire team is within a 2 meter radius :)

There are other teams within the department which are distributed in a total of 4 cities inside the same country (a sort of "hub and spoke") and with SOME of these teams, because they collaborate and communicate often, this candid communication works well.

Granted this is from my experience of working in in-house dev teams (rather than say open source contributions), but very often I find there is no single objective right way to do something, certain things are just subjective opinions, and I'm not going to pretend that after looking at a PR on GitHub.com, that I know more than the person working on it (of course, there are exceptions, but this is what I find to be true generally).

If I see something that looks wrong, my assumption is that I'm missing some kind of knowledge or context that lead to the way for the code to be written the way it is. So I ask questions or offer suggestions or my thoughts. I'm definitely not going to tell someone to do something.

But at the end of the day, very rarely do I see something that should be a critical blocker for a piece of work, so I'll share my thoughts, approve, and leave it to the perfectly capable original submitter to do with that what they want.

This can be culturally biased as well. I find European groups tend to be more assertive where as east Asian groups definitely don't use imperative forms as a matter of course. I think the takeaway is to always consider the audience for which you're writing.

> Use emoji to clarify tone. Compare “:sparkles: :sparkles: Looks good :+1: :sparkles: :sparkles:” to “Looks good.”

Or, you know, write "Looks great!"

Exclamation mark is the OG emoji

Or formalize phrases. The Google review guide [1] defines some phrases and as such there is no question about the tone of it. Prefix comments on non-essential parts of the commit with "Nit:", approve the changes with "LGTM" (looks good to me).

[1] https://google.github.io/eng-practices/review/

That's boring. As a form of protest, I'm going to do all future reviews in emoji only.

On smaller projects I've done code reviews with reaction GIFs (although always with people who knew me and my humour well).

Better support for pull request revision would be nice. In mailing lists, the entire patch set is resent when revisions are made. When I force push revised commits to my branch, the old commits are eventually garbage collected despite the existence of references to those commits in the pull request.

Also, it is important to discuss pull request etiquette. When is it appropriate to mention the maintainers? How to draw attention to a pull request?

It is a problem in mailing lists but GitHub has good tooling around it. Every time you force-push, GitHub keeps and provides direct links to the old commit, the new commit and the diff between the two commits. Every force push appears as its own diff on the GitHub PR page.

How is GitLab doing in this area? Does GitLab show diffs for force-pushes?

> Every time you force-push, GitHub keeps and provides direct links to the old commit, the new commit and the diff between the two commits.

Not in my experience. Here's one of my pull requests:

https://github.com/mchehab/zbar/pull/64

The maintainer reviewed some of the changes and I revised my commits as a result. The review is correctly marked as outdated. Clicking on the file name tells me the commit cannot be found.

> We went looking everywhere, but couldn’t find those commits.

> Sometimes commits can disappear after a force-push. Head back to the latest changes here.

Your pull request shows exactly what I am talking about. For every force-push it has direct links to the old commit, the new commit and the diff.

Pick the first force push in your PR. It says

> matheusmoreira force-pushed the matheusmoreira:binary-decoding branch from aec04b3 to 87a0b3c on 5 Nov 2019

Click on 'force-pushed'. That's the diff of force-push.

Click on 'aec04b3'. That's the commit before force-push.

Click on '87a0b3c'. That's the new commit in the force-push.

You're right. I never realized those messages contained links. They do lead me to the old commits.

I don't understand why the code review can't find the commit though.

> For every force-push it has direct links to the old commit, the new commit and the diff.

It's there a way to see the diff between commits that are/were not at the HEAD of the branch? That is, doing something like:

  git diff aec04b3^.. 87a0b3c^

> Better support for pull request revision would be nice

Github added a force push diff link feature not too long ago which shows the diff after doing a force push. Unfortunately, they don't show a per commit diff or a diff of the commit messages.

I was lucky enough to have a team member introduce me to this article early in my career:

https://mtlynch.io/human-code-reviews-1/

I still think about it when reviewing code, it's really sound advice over all

Discussed at the time: https://news.ycombinator.com/item?id=9717937

But can absolutely be revisited.

> @mention

Yes, GitHub would love for their namespace to be the implied namespace in commit messages.

In other words, please don't. Use names or e-mail addresses to refer to people.

....but GitHub mentions are actually useful, and send notifications to users to alert them to look at the post.

GitHub mentions are good—in the pull request thread you open on GitHub. In commit messages, not so much. The same rule applies for issue references; URLs should be preferred (or at least something like GH-XXXX) to simple hashtag numbers in commit messages, the improvement matters a lot when it does, but you can only get it if you do it from the beginning (Git commits being immutable).

Put them in comments on the commit on github. It's better to keep github stuff inside github. Your source code with your git history may move off github one day.

They're talking about pull requests, not commits.

> Use emoji to clarify tone. Compare “:sparkles: :sparkles: Looks good :+1: :sparkles: :sparkles:” to “Looks good.”

.. Just approve that damn thing. Don't decorate it in confetti ;)

“Be careful to not use only one sparkle as the maintainer might get offended”

Rebase, rebase, rebase. Shameless plug for a tutorial I wrote:

https://git-rebase.io

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.

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