The first point to make about JCR is that it's built to support a particular style of code review process. The main points about the JCR process are:
- It assumes that there will be a Review Meeting, where all the reviewers (and the project owner) review all the comments made, discuss them, and decide what the action should be
- It's targeted at being able to review large chunks of code fast, rather than doing lots of very small reviews.
If your process is similar, JCR is likely to be a good fit. If not, you should look at another tool to see if it will fit your process better.
Some (potential) advantages of using JCR include:
- You can review any code, from any source. It doesn't have to be from your source control system
- You can review changes between any two tags/versions/baselines, not just one commit
- Provides full context - all files in the source tree can be viewed, and all of each file being reviewed is visible (not just the diffs). This means reviewers don't have to have access to the code branches being reviewed/diffed, or a development environment
- Tries hard to make the process easy and comprehensive for reviewers
- Allows tracking of required action (and action owner) for each comment, and recording of whether actions have been completed.
- Provides a useful report if you have to document all reviews that have taken place
- Basic support for linking references to other Java files
However, JCR also has its disadvantages:
- There is not yet any integration to source control systems (planned for version 0.7)
- Setting up a review project in JCR is probably slower than with other systems (mainly due to the lack of source control system integration)
- Display of diffs is a bit basic - it shows lines that have changed, but not intra-line changes
- JCR won't work when attempting to review projects containing spaces in paths
- Doesn't yet run on Windows (also planned for version 0.7).
I'm aware of four main competitors for JCR: Crucible, Review Board, Rietveld and Codestriker.
Crucible is a relatively new commercial tool from Atlassian. I haven't used it, although I've spent a while looking at what it does and how.
- Nicely put together
- An easy-to-use UI
- Very good integration with source control systems (limited range) and defect tracking
- Commercial, but not too expensive
- A little weak in tracking review actions (if that's important to you).
Review Board is an even newer open source tool from VMWare. I've only had a quick look at it, but it looks promising. It looks like an excellent tool for less formal code reviews, particularly where you may want to get the same files reviewed several times before the code is complete.
Rietveld is a new tool from Guido van Rossum (Python's Benevolent Dictator For Life). It's based on the Mondrian tool he wrote for Google, but is open source. It hasn't reached a stable release yet (as of May '08), but is somewhat similar to Review Board (which was also based on what was known of Mondrian).
Codestriker is another open source online code review tool. It's written in Perl (and beautiful perl at that - never thought I'd ever say such a thing about perl), and seems to be well designed and complete.
- Integrates with a number of source control systems
- Well-designed system for plugging in other functionality
- Seems to offer little control over who can do what, or tracking of who did what (last time I looked there was no authentication of users)
- We found reviewing with Codestriker to be quite slow, because of the way the user interface works.
I'm glad you asked ;}
There are all sorts of benefits from code reviews, and they're not always the ones you'd think. For my money, they include:
Education
Code reviews are a great way of pointing out good or bad techniques to other developers (particularly less-experienced ones) and making developers aware of internal or external libraries etc. that can be used instead of resorting to wheel-reinvention. Additionally, they give you an opportunity to discuss some of the issues, to make sure everyone's on the same page.
Oh, and you'll probably learn quite a bit yourself... In particular, you get a guided tour of parts of the codebase you may not know, from someone who's just been there.
Finding bugs
Yes, but not necessarily as important as you might think. If you've got good unit, integration and system test coverage (and if you haven't, what are you waiting for?), the main issues you might spot in a code review will be for non-functional aspects like concurrency issues, database transactionality, memory leaks etc.
Code and design quality
Very important. Reviews give you a chance to suggest better ways of doing it, whatever it is, and to discuss potential issues with the way the code has been written
Code standardisation
One of the tenets of agile methods is that all code should look the same - it shouldn't be possible to tell from the code who wrote it. That way you can concentrate on understanding the code, rather than grappling with trying to parse the code. While you can (and should) use static checkers like Checkstyle to ensure that your basic coding standards have been met, reviews will catch the issues that need human eyes to find.
Regulatory issues
At Dialect we operate under pretty tight security requirements, and we have to provide proof of appropriate peer code review to 2 separate external auditors. JCR provides everything we need for this.
Short answer: No, not directly (but it's planned). But you should be able to run JCR under Cygwin on Windows.
Slightly longer answer: JCR currently uses a number of Linux/UNIX/GNU tools (diff, tar, rm, grep etc.). It also assumes UNIX directory separators. If you fancy hacking up a Windows-capable version of JCR, look at the 2 modules under /jcr/model/vcs/tarvcs.
JCR doesn't yet integrate to source control systems (like Subversion, CVS, ClearCase etc.). The reasons for this were originally environmental (we didn't have the appropriate ClearCase install for the old Linux box that JCR originally ran on), and then it became an "If it ain't broke" issue.
However, there are also advantages in being able to upload the file sets you wish to review, since the file sets can come from anywhere, even 2 different source control systems.
Version 0.7 of JCR is is planned to include support for at least Subversion and ClearCase.
As someone who works in a secure environment, the short answer has to be 'Not particularly.'
The longer answer is that security on JCR is probably adequate for internal use, but I wouldn't like to guarantee it for open exposure on the internet. Here are a few notes to help you make your own decision, and to help you secure your installation:
MOST IMPORTANT: Ensure that the following line is present in the [app:main] section of the config file: set debug = false. If not, anyone who can trigger an exception can take over your JCR installation, including getting shell access as the JCR user
JCR should run as a specially-created user and group (referred to as the JCR user in the install guide). This user should have the minimum set of privileges necessary
Similarly, the database user should have minimum privileges
Database credentials (user and password) are stored in the clear in the config file, so ensure that others do not have access to this file
Passwords are sent in the clear on login.
- Pylons, the web framework underlying JCR, can be hidden behind a web server like Apache, so you can use that to provide SSL. See this Wiki page for more details
Passwords are stored as a salted MD5 hash in the database
In theory, all requests are checked for both authentication (via a cookie) and authorisation, based on 3 dynamic roles (administrator, project owner, and everyone else)
Most HTML should be escaped against XSS, but I don't guarantee that all fields are covered
All DB access uses bind variables (aka prepared statements), so SQL injection shouldn't be possible
Actions on uploaded file sets (untarring/unzipping, diffing, clearing etc.) are currently accomplished by shelling out to the operating system using Python's os.system call. This is probably not very secure, and it's also possible that sanitising of input could be better. Another good reason to limit the privileges available to the JCR user
The backupjcr.sh script uses a temporary file without protecting against tempfile attacks.
Because JCR works at the file level, it doesn't know that you've moved or renamed the file - instead, it thinks that you've deleted one file and added another. That is, unless you tell it about the relationship between the 2 files.
You need to use the files administration page for the project to set the original path for the file. Then, JCR will regenerate the diff for the file.
This is most commonly caused by changing the file line ending (e.g. between DOS and UNIX), or by changing between tabs and spaces.
The solution is to redo the diff for the project, specifying that diff should ignore changes in white space. This is done on the file sets administration page.
If you've got a lot of files in your project, or if there are some files that are much more important, you have 3 ways of passing this information on to the reviewers:
- Set the priority for some or all files on the files administration page
- Add a file note for important files on the files administration page
- Add a project note on the project summary page, explaining what you want the reviewers to know.
Note that setting file priorities is a little better than the other techniques, since the reviewer can sort files by priority.
You can add project-level notes on the project summary page, and file-level notes on the files administration page.
Note that you can include links in notes, to HTML pages, file:/// URLs, or to other files or Java classes within the review project. See links in notes and comments.
When you regenerate diffs for a project, the diffs for individual files may well have changed - JCR can't know what's still valid, so it applies the following logic:
- Any original paths on review files are cleared, and the change type for each file reverts to the change type calculated from the diffs
- Any file comments are moved to line 1 of the file (note that this can only happen if you started reviewing the project, then reverted its status to Selecting Files, and regenerated the diffs).
On the file review page, used the 'Show deleted lines' link in the file header.
Sometimes you'd like to include a link to another review file in a comment, for example to point out where this bit of code was copy-and-pasted from :-(. You can insert links using the jcrfile:/// or jcrclass:// URL formats. See links in notes and comments.
As well as links to other review files or classes, comments also renders external URLs as links. Supported URL types are http, https and file. See links in notes and comments.
The 'View any file' page allows you to search for and view any file in your project that has been added, changed or deleted. However, it doesn't yet provide access to unchanged files.
If you want to view an unchanged file:
- If it's a Java class, any reference to that class in a Java review file will provide a link to view the class
- If it's a Java class, you can add a jcrclass:// URL to a comment, and this will render as a link to the file. For this, you only need to know the class name
- If it's not a Java class, you can add a jcrfile:// URL to a comment, and this will render as a link to the file. For this, you need to know the full path to the file from the file set root directory.
See the documentation for external scripts.
If the log file path specified in you config file is invalid (directory doesn't exist, not writeable etc.), JCR won't start (and nor will the setup-app command work). Instead, you'll get a big stack trace, whose last line looks something like:
IOError: [Errno 2] No such file or directory: '/a/b/c/jcr.log'
Fix the path in the args setting under [handler_file], and all will be well.