
How to write the perfect pull request (2015) - xiaodai
https://github.blog/2015-01-21-how-to-write-the-perfect-pull-request/
======
airstrike
> 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

~~~
Kwantuum
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/](https://google.github.io/eng-practices/review/)

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

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

------
davidrm
> 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.

~~~
hcarvalhoalves
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.

~~~
ProZsolt
> 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.

------
matheusmoreira
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?

~~~
chenhan
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?

~~~
matheusmoreira
> 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](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.

~~~
chenhan
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.

~~~
matheusmoreira
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.

------
tommit
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/](https://mtlynch.io/human-code-
reviews-1/)

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

------
dang
Discussed at the time:
[https://news.ycombinator.com/item?id=9717937](https://news.ycombinator.com/item?id=9717937)

~~~
Drakar1903
But can absolutely be revisited.

------
teddyh
> _@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.

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

~~~
deepersprout
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.

------
bilekas
> 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 ;)

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

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

[https://git-rebase.io](https://git-rebase.io)

------
chenhan
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/](https://chris.beams.io/posts/git-
commit/)

2\. Following the pull request workflow correctly. Follow this guide:
[https://github.com/susam/gitpr](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.

~~~
chenhan
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.

~~~
dimes
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.

~~~
polyphonicist
> 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.

~~~
dimes
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.

~~~
u801e
> 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.

