4 ms·

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

by hubb 15y ago

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 committed? or do you commit, but you branch for every bug fix, and then merge when the review is completed? is there another clever way to do this that doesn't involve revision control?

moxiemk1 15y ago
Some tools (Crucible, at the least) allow you to code-review based off of a diff that specifies the version it is applied to.
wccrawford 15y ago
Proper branching. You check it into your branch, share it, code review it, fix it... Then squash the commits if you don't want all the 'mess' in the final repo. Then finally push it into the trunk. Personally, I've never bothered squashing. The points that you deploy the code are important, but the visual aspect of the history is not so important. On the other hand, if you want to know when and why a change was done, having the FULL history is a lot more important suddenly.
- gte910h 15y ago
  Okay, that's more reasonable than "nothing is checked in without a review".
masterzora 15y ago
> 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 committed? The easiest method is just passing around diffs, though more sophisticated tools exist. Review Board is one I know of off-hand, though my experience with it was not overly great. I've personally just tossed together a decent diff-viewer with a couple different view modes to account for when, say, a quick patch is sufficient vs. when you really need to see the code in context. But this is only one solution of many. I actually have seen solutions that involve source control systems, but I've always found them to be too hacky even for me.
skermes 15y ago
I'm not sure why it'd be a goal to find a way to do code reviews that doesn't involve version control. We review everything everything before it gets merged into our mainline branch, and the process is pretty much what you described. Every bug/feature gets a branch, and when it's finished whoever's working on it takes a diff and attaches it to the issue in our bug tracker. The reviewers look at the diff (or, if they want more detail, pull the branch in question and look at the commit history with more context) and then pull the author in to discuss it. Every once in a while we consider either some more clever process or some sort of integrated tool to automate more of the process, but we always conclude that what we have works well enough that it's not that big a deal. If your source control makes branching/merging painful enough that it's going to get in your way to do it on a regular basis you might want something different, but that seems like an argument for better source control rather than a need for clever reviewing strategies.
- aaronblohowiak 15y ago
  We pull the branch and have the reviewer run the relevant tests. Doing this was simpler than figuring out how to do fancy stuff with the ci server (which checks the integration branch after merge.). Having this step requires the reviewer to know which tests are relevant, which ensures that they were written or updated.
rimantas 15y ago
That's a part of the beauty of GitHub: pull requests.
Locke1689 15y ago
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.
- jleader 15y ago
  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/ http://codereview.appspot.com/
- btilly 15y ago
  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/ http://code.google.com/p/rietveld/ for more.
  - Locke1689 15y ago
    Perfect, I thought I was going to have to give up Mondrian if I left.
    - btilly 15y ago
      Oh, you likely still will have to give it up. You can tell people how well the system works, but the idea of having code review on every checkin sounds like such a heavy process that you'll never convince anyone else to do it.
      - Locke1689 15y ago
        Guess ill have to take some Googlers with me ;)
- 15y ago
DrJ 15y ago
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'.
bulletsvshumans 15y ago
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.
RandallBrown 15y ago
Where I work we invite the person over to our computer and they do the review right there. Obviously, this doesn't work when someone is working remotely, but Remote Desktop or VNC work just fine for that. For small one file code reviews we'll often send screenshots of the diffs. (We make screen capture software btw)
brown9-2 15y ago
how does the reviewer look at your diffs and code if your changes haven't yet been committed? Where I work we have a pretty simple script which diffs each file in the Perforce changelist against your local copy and sends it in an email to the team, with some pretty formatting for added/removed/changed lines. Discussion then takes place over email, which for 99% of changes is good enough since the teams are small.
somebear 15y ago
At work we have just rolled out Gerrit as the review tool. The setup is simply that you push your commits to Gerrit, then reviewers can comment on and approve (or not) the change in the web tool. Reviewers can also fetch your patch and run it on their own system. After a change has been approved, it is pushed into the CI system by Gerrit, and if/when it passes that it is pushed into the public repos. Before that all reviews were done by passing around diff files.
- neves 15y ago
  http://www.review-board.org http://www.review-board.org rules
  - somebear 15y ago
    There was a long discussion [1], and in the end Gerrit was chosen. I have used Review Board previously, but actually like Gerrit quite a lot now that I'm forced to work with it ;) [1] http://lists.qt-labs.org/public/opengov/2011-February/000260.html http://lists.qt-labs.org/public/opengov/2011-February/000260...
mkjones 15y ago
There are tools like Phabricator (http://phabricator.org/ http://phabricator.org/) that extract your diff and store it in a database, providing a web frontend that allows others to add inline comments and suggestions independent from the underlying revision control system. We use a version of this at Facebook for required pre-commit review, and I've found it to be quite nice. Check out e.g. https://secure.phabricator.com/D583 https://secure.phabricator.com/D583 for an example of a diff for phabricator itself that has some inline comments and other input.
MrKurtHaeusler 15y ago
In person. Why resort to tooling when simple collaboration and communication suffice?

by hubb 15y ago

moxiemk1 15y ago

Some tools (Crucible, at the least) allow you to code-review based off of a diff that specifies the version it is applied to.

wccrawford 15y ago

Proper branching. You check it into your branch, share it, code review it, fix it... Then squash the commits if you don't want all the 'mess' in the final repo. Then finally push it into the trunk. Personally, I've never bothered squashing. The points that you deploy the code are important, but the visual aspect of the history is not so important. On the other hand, if you want to know when and why a change was done, having the FULL history is a lot more important suddenly.

gte910h 15y ago
Okay, that's more reasonable than "nothing is checked in without a review".

masterzora 15y ago

> 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 committed? The easiest method is just passing around diffs, though more sophisticated tools exist. Review Board is one I know of off-hand, though my experience with it was not overly great. I've personally just tossed together a decent diff-viewer with a couple different view modes to account for when, say, a quick patch is sufficient vs. when you really need to see the code in context. But this is only one solution of many. I actually have seen solutions that involve source control systems, but I've always found them to be too hacky even for me.

skermes 15y ago

I'm not sure why it'd be a goal to find a way to do code reviews that doesn't involve version control. We review everything everything before it gets merged into our mainline branch, and the process is pretty much what you described. Every bug/feature gets a branch, and when it's finished whoever's working on it takes a diff and attaches it to the issue in our bug tracker. The reviewers look at the diff (or, if they want more detail, pull the branch in question and look at the commit history with more context) and then pull the author in to discuss it. Every once in a while we consider either some more clever process or some sort of integrated tool to automate more of the process, but we always conclude that what we have works well enough that it's not that big a deal. If your source control makes branching/merging painful enough that it's going to get in your way to do it on a regular basis you might want something different, but that seems like an argument for better source control rather than a need for clever reviewing strategies.

aaronblohowiak 15y ago
We pull the branch and have the reviewer run the relevant tests. Doing this was simpler than figuring out how to do fancy stuff with the ci server (which checks the integration branch after merge.). Having this step requires the reviewer to know which tests are relevant, which ensures that they were written or updated.

rimantas 15y ago

That's a part of the beauty of GitHub: pull requests.

Locke1689 15y ago

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.

jleader 15y ago
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/ http://codereview.appspot.com/
btilly 15y ago
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/ http://code.google.com/p/rietveld/ for more.
- Locke1689 15y ago
  Perfect, I thought I was going to have to give up Mondrian if I left.
  - btilly 15y ago
    Oh, you likely still will have to give it up. You can tell people how well the system works, but the idea of having code review on every checkin sounds like such a heavy process that you'll never convince anyone else to do it.
    - Locke1689 15y ago
      Guess ill have to take some Googlers with me ;)
15y ago

DrJ 15y ago

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

bulletsvshumans 15y ago

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.

RandallBrown 15y ago

Where I work we invite the person over to our computer and they do the review right there. Obviously, this doesn't work when someone is working remotely, but Remote Desktop or VNC work just fine for that. For small one file code reviews we'll often send screenshots of the diffs. (We make screen capture software btw)

brown9-2 15y ago

how does the reviewer look at your diffs and code if your changes haven't yet been committed? Where I work we have a pretty simple script which diffs each file in the Perforce changelist against your local copy and sends it in an email to the team, with some pretty formatting for added/removed/changed lines. Discussion then takes place over email, which for 99% of changes is good enough since the teams are small.

somebear 15y ago

At work we have just rolled out Gerrit as the review tool. The setup is simply that you push your commits to Gerrit, then reviewers can comment on and approve (or not) the change in the web tool. Reviewers can also fetch your patch and run it on their own system. After a change has been approved, it is pushed into the CI system by Gerrit, and if/when it passes that it is pushed into the public repos. Before that all reviews were done by passing around diff files.

neves 15y ago
http://www.review-board.org http://www.review-board.org rules
- somebear 15y ago
  There was a long discussion [1], and in the end Gerrit was chosen. I have used Review Board previously, but actually like Gerrit quite a lot now that I'm forced to work with it ;) [1] http://lists.qt-labs.org/public/opengov/2011-February/000260.html http://lists.qt-labs.org/public/opengov/2011-February/000260...

mkjones 15y ago

There are tools like Phabricator (http://phabricator.org/ http://phabricator.org/) that extract your diff and store it in a database, providing a web frontend that allows others to add inline comments and suggestions independent from the underlying revision control system. We use a version of this at Facebook for required pre-commit review, and I've found it to be quite nice. Check out e.g. https://secure.phabricator.com/D583 https://secure.phabricator.com/D583 for an example of a diff for phabricator itself that has some inline comments and other input.

MrKurtHaeusler 15y ago

In person. Why resort to tooling when simple collaboration and communication suffice?