
Ask HN: What is the best pre-commit code review workflow? - rocky1138
I&#x27;m a lead developer who runs a small team of 3 full-time, in-office developers and one designer.<p>I&#x27;m a fan of reviewing code before it accepting a pull request and expect developers to properly rebase so they don&#x27;t push conflicts onto anyone else.<p>I don&#x27;t want to block my team but at the same time I don&#x27;t want to switch to post-commit reviews since I can&#x27;t ensure code quality.<p>What&#x27;s the fastest and highest code quality pre-commit review system you&#x27;ve ever used? Note that the answer doesn&#x27;t have to be technical. It can be a simple behaviour change.<p>Thank you :)
======
hacker_9
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 :\

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

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

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

------
piotrkaminski
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](https://reviewable.io), as it
integrates smoothly and requires nearly no setup. (Disclosure: I'm the
founder.)

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

~~~
delinquentme
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/](http://www.wired.com/2012/05/torvalds_github/)

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

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.

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

------
njs12345
Phabricator is great, can't recommend it enough.

~~~
rocky1138
Can you edit your comment to include why?

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

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

------
nahname
How is everyone not using github pull requests for this?

git checkout -b my-new-feature

<write code>

git add --all :/

git commit -m "added this awesome new feature'

git push origin my-new-feature

<open PR>

<wait for feedback>

<address feedback>

<merge PR>

<ship>

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

<wait for feedback>

<address feedback>

<merge PR>

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

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

