Ask HN: What is the best pre-commit code review workflow? | Hacker News Reader

Ask HN: What is the best pre-commit code review workflow?

22 pointsrocky113818 comments

I'm a lead developer who runs a small team of 3 full-time, in-office developers and one designer.

I'm a fan of reviewing code before it accepting a pull request and expect developers to properly rebase so they don't push conflicts onto anyone else.

I don't want to block my team but at the same time I don't want to switch to post-commit reviews since I can't ensure code quality.

What's the fastest and highest code quality pre-commit review system you've ever used? Note that the answer doesn't have to be technical. It can be a simple behaviour change.

Thank you :)

View on HN

1. The reviewee should self-review his code before he calls you over - that means going over changed files and undo'ing those that are unneeded (i.e. where only a newline/space was entered). They should also read through their changes in the diff window, and add in any comments where the 'why' is not clear from the code.

2. There should already be a window open displaying how their changes affects the UI/behaviour of your product, so that you can immediately get into the right context/mindset when you come over. This way as they explain their changes you can compare with your mental model of how'd you'd have solved the problem, and notice any glaring problems along the way (like if they used a bunch of complex logic to parse a string when they should have used a simple regex)

3. Coders should expect to get reviews every one or two days, otherwise they will build up a substantial amount of changes that will take longer to process. Additionally they may even forget why they made earlier changes!

4. Once you've reviewed a team member a number of times you'll find that they 'get it' and have already predicted your questions and made the necessary changes. They are then ready to become a reviewer as well to lighten the load on you.

5. Post commit reviews do not work. The damage is already done by then. They tried this at a place I used to work and nobody learnt anything from it :\

>3. Coders should expect to get reviews every one or two days, otherwise they will build up a substantial amount of changes that will take longer to process. Additionally they may even forget why they made earlier changes!

NOT ACCEPTABLE.

Rationale should be documented in the ticket, and the ticket should be tagged in the commit message.

Any engineer rationale MUST be documented clearly with comments for all reviewers.

I find that 1-2 days, maybe 3, is a good number before getting a review. This doesn't mean they have to be finished with their piece of work, or that they even have to commit the code at the end of the review, this is just so a second pair of eyes can see it and both can agree that they are on the right track.

Then when the functionality is complete, the same reviewer can glance over the already reviewed sections before focusing mainly on the new changes.

Have you considered making smaller, more frequent commits.

Here's a strong opinion, based on my own experiences here at Labs. As is traditional on HN, it doesn't directly answer your question, but instead assumes the universal superiority of my own preferences.

1. Pair program.

2. Drive code from tests, outside in.

3. Rotate pairs frequently. Daily for short stories, no more than 2 days for long stories.

4. Review your commits together.

Every other reviewing scheme I've seen quickly becomes an unacceptable bureaucratic drag on forward movement.

At best you wind up with an average of around 2-3 days lag, giving plenty of time for the original engineer(s) to lose context and for the master branch to be well out in front.

At worst you have patches sprinkled around multiple branches of multiple repos, everyone is blocked on everyone else and continuous integration is thought of as a joke about mathematicians.

The original literature on code reviews is based on a very particular and extremely rigid manner of developing software. I think pair programming captures 80% (or more) of the value for less than 20% of the effort and disruption.

Besides all the good ideas suggested by others for high-level process optimization (small PRs, self-review first, prioritize reviews, have a larger pool of reviewers, etc.), you can also micro-optimize the time spent on reviews proper. A good tool can help a lot here: it will show incremental changes since the last review round (even if the branch got rebased), filter out changes due solely to rebasing, track resolution of any issues raised during the review, etc.

If you're using GitHub, the built-in pull request review system doesn't really do these things very well (or at all), so you end up wasting a lot of time and/or missing things. There are a number of third-party options that do much better but some of the best require you to host your own infrastructure, move your repo, etc. Selfishly (and again, only if you're using GitHub), I'd recommend checking out https://reviewable.io, as it integrates smoothly and requires nearly no setup. (Disclosure: I'm the founder.)

Good questions.

1. Everything must enter your main branch (master) via Pull Request 2. Everyone can review it. Sometimes more than one person is needed. 3. It's asynchronous - write comments on the code or sit together if you need to see UI / Changes. 4. Code Review should also review unit tests (which pass) 5. Merge PR

This shouldn't block your team and in a week of doing it people will adore this since it will save them at least once.

Is there something Im missing with pull requests? Or is it that even programmers like UIs?

1) Git log becomes useless.

2) You're merging code via a UI, without ANY granularity whatsoever.

3) Does it not just FEEL dirty doing this in a UI?

4) A little appeal to authority: http://www.wired.com/2012/05/torvalds_github/

5) and a bit more: https://github.com/torvalds/linux/pull/17#issuecomment-56637...

6) Efficiency lost w regards to minor changes. Youd actually write a comment to have someone change something and then RE-issue the pull request? Sure you would, because youre middle management.

Pull requests seem to be a hacky way for management to get involved with something they understand at the loss of control in arguably the most complex phase of coding: INTEGRATION.

You don't have to use the UI. If you're really anal about your git history, you can do the merge locally and squash everything into one commit. IMO, it's not worth the effort to do that, though. You probably don't actually care about the Git log - you care about the file history (i.e. git blame), which will usually point you to the right commit anyway, even if you have a bunch of extra merge commits in your log.

FWIW, you're not Linus. You don't get 17 million emails/day (exaggeration, I know). He has a very refined workflow based on many, many years of working with people all over the world on a gigantic project used by hundreds of millions of people.

Re point 6: You don't create another pull request. You just make another commit on that branch.

Github has a command line tool that lets you merge in pull requests by downloading and applying the PR's patch; that will keep your history clean. PRs offer a great UI for coffee reviews; just because you use them for that purpose doesn't mean you need to merge with the big green button.

Phabricator is great, can't recommend it enough.

Can you edit your comment to include why?

I do what you are suggesting. Some other things:

1. Always a PR that is ready to merge (rebased properly). I should be able to read commit-by-commit and it all makes sense.

2. All reviewers are committed to review in < 24 hours, mostly the same day. Reviewing is considered an important high-priority activity for developers.

3. Keep them small - we reserve the right to reject a PR on size -- split it down (in reality, this is rarely done, because we know this)

4. Only make a comment if you think the code is unacceptable without the change. You can be convinced otherwise later, but no subjective comments -- it's either acceptable as is (no comment) or not.

What I wish we did (and will probably try to make happen)

1. Work off a checklist. I have a couple of things I always check for -- e.g. i18n and accessibility, but we could do this a lot more.

2. Have a common understanding on what acceptable test coverage might be

Ugh. I would quit if I had to work in a pre-commit code review system. The idea should be that your team is so aligned that for 95% of all commits, you don't need any code review at all. If I refactor three common pieces of code into a utility function, why waste time code reviewing something that obvious?

> I don't want to block my team but at the same time I don't want to switch to post-commit reviews since I can't ensure code quality.

You should have thought of that when you hired those developers. Now if you think that you cant trust them to produce quality code on their own then you have a problem.

How is everyone not using github pull requests for this?

git checkout -b my-new-feature

git add --all :/

git commit -m "added this awesome new feature'

git push origin my-new-feature

<ship>

We are. This is the part I want to optimize:

Is the problem a function of the size each PR is? If each PR is small, it shouldn't take much time to review.

Sometimes. Sometimes it's also a case of the first developer working on a feature which the second developer needs but is stuck in code review.

Reply on news.ycombinator.com