Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

How to write the perfect pull request (2015)

github.blog

Post reply on HN
Print view View original

31–40 of 40 posts

Re: How to write the perfect pull request (2015)

#31

Post by ProZsolt »

hcarvalhoalves wrote: post #24
davidrm wrote: post #22

> 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 prod…

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-fo…

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

Re: How to write the perfect pull request (2015)

#32

Post by u801e »

dimes wrote: post #28
polyphonicist wrote: post #20

Earlier quoted context omitted.

> 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 w…

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 ar…

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

Re: How to write the perfect pull request (2015)

#33

Post by u801e »

matheusmoreira wrote: post #4

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…

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

Re: How to write the perfect pull request (2015)

#34

Post by u801e »

chenhan wrote: post #8
matheusmoreira wrote: post #7

Earlier quoted context omitted.

> 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 foun…

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…

> 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^

Re: How to write the perfect pull request (2015)

#36

Post by klohto »

bilekas wrote: post #35

> 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”

Re: How to write the perfect pull request (2015)

#38

Post by tr1coder »

polyphonicist wrote: post #20
dimes wrote: post #11

Earlier quoted context omitted.

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…

> 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 w…

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.

Re: How to write the perfect pull request (2015)

#39

Post by Leherenn »

dimes wrote: post #11
chenhan wrote: post #6

Earlier quoted context omitted.

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 goo…

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…

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.

Re: How to write the perfect pull request (2015)

#40

Post by dharmab »

xvector wrote: post #26
Kwantuum wrote: post #14

Earlier quoted context omitted.

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).
Post reply on HN