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

Things Everyone Should Do: Code Review

scientopia.org

Post reply on HN
Print view View original

11–20 of 51 posts

Re: Things Everyone Should Do: Code Review

#11

Post by Locke1689 »

gte910h wrote: post #6

>At Google, no code, for any product, for any project, gets checked in until it gets a positive review. I can't believe that's true as stated. I am guessing "No code is put to a branch which is used by others without review or "No code is put into production without review". I can't imagine "You aren't allowed to check in things without getting signoff of others" working period.

You can always commit it to your home share but no, at Google all code that is checked in to version control must be reviewed, period.

Re: Things Everyone Should Do: Code Review

#12

Post by Locke1689 »

hubb wrote: post #2

this fellow is advocating code reviews before checking in changed code to revision control. i can appreciate the benefits of that -- less churn and junk in the repository, the commit history for most files will be succinct, and each change-set will contain a single change or fix. but doesn't that bring some logistical challenges? how does the reviewer look at your diffs and code if your changes haven't yet been commi…

Unfortunately, coming from Google he had some very neat tools to help do this that (as far as I know) don't have equivalent counterparts outside Google. It would be harder to do, but distributed version control could help considerably. One possibility would be to make everyone commit to their own local repos and then force a pull request every time they want to commit something to the main repo.

Re: Things Everyone Should Do: Code Review

#13

Post by DrJ »

hubb wrote: post #2

this fellow is advocating code reviews before checking in changed code to revision control. i can appreciate the benefits of that -- less churn and junk in the repository, the commit history for most files will be succinct, and each change-set will contain a single change or fix. but doesn't that bring some logistical challenges? how does the reviewer look at your diffs and code if your changes haven't yet been commi…

you should be branching for every bugfix, then when you have fixed the bug and the code passes all known tests + the one for the bug, you should merge it back squashing the branch commit history with a single merge (where you add your own comments instead of just using 'merged in blah blah'.

Re: Things Everyone Should Do: Code Review

#14

Post by bsergean »

Locke1689 wrote: post #11
gte910h wrote: post #6

>At Google, no code, for any product, for any project, gets checked in until it gets a positive review. I can't believe that's true as stated. I am guessing "No code is put to a branch which is used by others without review or "No code is put into production without review". I can't imagine "You aren't allowed to check in things without getting signoff of others" working period.

You can always commit it to your home share but no, at Google all code that is checked in to version control must be reviewed, period.

What does commit to your home share means in term of perforce ? cp .cc .h /tmp/backup ?

Re: Things Everyone Should Do: Code Review

#15

Post by FaceKicker »

Locke1689 wrote: post #11
gte910h wrote: post #6

>At Google, no code, for any product, for any project, gets checked in until it gets a positive review. I can't believe that's true as stated. I am guessing "No code is put to a branch which is used by others without review or "No code is put into production without review". I can't imagine "You aren't allowed to check in things without getting signoff of others" working period.

You can always commit it to your home share but no, at Google all code that is checked in to version control must be reviewed, period.

The exception being the "experimental" part of the code base, which can never be used in any production code without undergoing a full code review.

Re: Things Everyone Should Do: Code Review

#16

Post by gte910h »

Locke1689 wrote: post #11
gte910h wrote: post #6

>At Google, no code, for any product, for any project, gets checked in until it gets a positive review. I can't believe that's true as stated. I am guessing "No code is put to a branch which is used by others without review or "No code is put into production without review". I can't imagine "You aren't allowed to check in things without getting signoff of others" working period.

You can always commit it to your home share but no, at Google all code that is checked in to version control must be reviewed, period.

So you can commit it somewhere.

You just can't commit it to a certain repo.

Re: Things Everyone Should Do: Code Review

#17

Post by lpolovets »

gte910h wrote: post #16
Locke1689 wrote: post #11

Earlier quoted context omitted.

You can always commit it to your home share but no, at Google all code that is checked in to version control must be reviewed, period.

So you can commit it somewhere. You just can't commit it to a certain repo.

It's more like without a code review you can only commit to your personal scratch space.

A more accurate way to state "At Google, no code, for any product, for any project, gets checked in until it gets a positive review" would be that "No code goes into production without a positive code review."

Re: Things Everyone Should Do: Code Review

#18

Post by bulletsvshumans »

hubb wrote: post #2

this fellow is advocating code reviews before checking in changed code to revision control. i can appreciate the benefits of that -- less churn and junk in the repository, the commit history for most files will be succinct, and each change-set will contain a single change or fix. but doesn't that bring some logistical challenges? how does the reviewer look at your diffs and code if your changes haven't yet been commi…

We do code reviews in person, at the reviewee's workstation. It does take time, but we've found that it leads to lots of productive discussion that might not otherwise occur, and that it helps strengthen relationships between team members.

Re: Things Everyone Should Do: Code Review

#19

Post by jleader »

Locke1689 wrote: post #12
hubb wrote: post #2

this fellow is advocating code reviews before checking in changed code to revision control. i can appreciate the benefits of that -- less churn and junk in the repository, the commit history for most files will be succinct, and each change-set will contain a single change or fix. but doesn't that bring some logistical challenges? how does the reviewer look at your diffs and code if your changes haven't yet been commi…

Unfortunately, coming from Google he had some very neat tools to help do this that (as far as I know) don't have equivalent counterparts outside Google. It would be harder to do, but distributed version control could help considerably. One possibility would be to make everyone commit to their own local repos and then force a pull request every time they want to commit something to the main repo.

When I wanted to submit some patches to one of the Protocol Buffers projects, I was asked to submit them via Guido Van Rossum's Rietveld tool, which is a public re-implementation of an internal Google code-review tool: http://codereview.appspot.com/

Re: Things Everyone Should Do: Code Review

#20

Post by btilly »

Locke1689 wrote: post #12
hubb wrote: post #2

this fellow is advocating code reviews before checking in changed code to revision control. i can appreciate the benefits of that -- less churn and junk in the repository, the commit history for most files will be succinct, and each change-set will contain a single change or fix. but doesn't that bring some logistical challenges? how does the reviewer look at your diffs and code if your changes haven't yet been commi…

Unfortunately, coming from Google he had some very neat tools to help do this that (as far as I know) don't have equivalent counterparts outside Google. It would be harder to do, but distributed version control could help considerably. One possibility would be to make everyone commit to their own local repos and then force a pull request every time they want to commit something to the main repo.

The key tool does have an equivalent counterpart, written by the same people who wrote Google's tool. See http://code.google.com/p/rietveld/ for more.
Post reply on HN