Meet Critic: Code Inspection System in Opera Software

The internal Critic source code inspection system used by Opera Software was uploaded to Github last night under the Apache License 2.0.

Sometimes code inspection systems are scolded for being completely unsuitable for the commercial development process. This is not about Critic. Critic was tested in the process of commercial software development in large projects of a large company, and proved to be excellent. I highly recommend you to try this wonderful tool.

Download Critic source codes here: github.com/jensl/critic .

Critic was written by Swedish developer Opera by the name of Jens Lindström. He was not happy with the existing code inspection systems, so he decided to write his own. By the way, Jens is famous not only for this. Those who are interested can search in the internet for his posts about the Carakan Opera JS engine.

Critic was written for the specific needs and tasks of the Opera. The author of this tool was also its user. He tried to make the tool practical and useful for himself and for other developers. As a Critic user watching him grow up and growing with useful features, I think Jens succeeded. Critic turned out to be a convenient and effective tool that saves the time of its users.

Critic is a web application and is written in Python. Critic is tightly integrated with Git. Your commits are added to review as soon as you push them into the Git repository Critic is watching.

The inspection cycle is as follows:

1. The review author commits his code to a Git repository that Critic oversees.
   
2. Depending on the commit method, review can be created automatically with git push, or manually through the Critic Web interface.
   
3. As soon as the review is created, inspectors and observers of the code that underwent the change will be notified by e-mail.
   
4. Inspectors and observers can add problem notes and code notes. Only inspectors can mark modified code as inspected. The code inspected does not mean approved. This means that the inspector checked it and created records of all the problems that he found.
   
5. The author of review participates in the discussion of problems and notes, and also pushes new commits into the review that fix the problems found. New commits are marked as uninspected. Items 4-5 can be repeated many times.
   
6. When all commits are inspected and there is not a single open record of problems - review is considered approved.
   
7. An approved review can be rediscovered, as well as, for example, a bug in BTS. Closed problem records may also be rediscovered. And of course, you can add new problems to the code already inspected. Philosophy is like in BTS. There is a certain workflow, and it is usually followed, but if necessary, you can return to the previous phase of this workflow.
   

Some notes:

1. Problem records can refer to the entire review, to a commit, or to specific lines of code. These lines of code may not be part of the commit, i.e. can be part of the “untouched” code, for example, when a commit changes the behavior of the untouched code. If a problem record refers to certain lines of code and these lines are fixed in the next commit, the problem is marked as “addressed” by this commit. The new commit, however, is marked as uninspected, so that all review cannot automatically become approved. This is one of the features that saves time when working with the system.
   
2. Critic supports "rewriting history." It sounds scary, but it is useful, especially when the “rewriting” takes place on a parallel Git branch via git rebase -i. For example, if your review has grown to 50 commits, and many of these commits — such as small things like minor bug fixes, compilation fixes, variable renaming, comment editing, and other “clean-ups” —will trash your story in VCS. In such cases, it’s useful to “stick” all these intermediate fixes to the commits that they fix before giving your branch for integration into the main project code. Thus, in the main branch of the project there will always be a beautiful story, not a mess. Well, or almost always. So, if you performed such an operation and turned 50 “bad” commits into 5 “good” ones, with the same content - Critic will not ask you to inspect this code a second time. For everything to work out, the total diff of 50 “bad” commits should be 100% identical to the total diff of 5 “good” commits. The rewritten story will be published in review.
   
3. Critic also supports changing branch review points. Suppose you branch the review branch from the top of the main branch of the project. During the inspection, the main branch of the project went forward. And your branch needs to change the code in accordance with the new realities of the main branch. What to do, create a new review and lose all the work already done on this review? No matter how. Critic will help you here. All commits can be copied to a new branch point using the same git rebase, and tell Critic where exactly you moved the branch point. And he will understand everything! And he won’t ask to inspect the same code a second time. And he won’t ask you to inspect tons of code from the main branch. But he will notice that you have resolved merge conflicts, and will ask the inspectors to check whether you did it correctly. Well, isn't that great?
   
4. Critic supports extensions. “It's fashionable right now.” Extensions can obviously extend Critic's functionality and save you even more time. Extensions can implement various specific functionality that is not needed by all users. Therefore, each user installs for himself those extensions that he needs. Suppose you are working on project X. Fix bugs. In review, there was initially 1 commit for the bugfix, but the inspectors were very strict, and I had to commit 5 more fixes to the fix in order to satisfy them. Finally review approved. And you have a little simple, but boring formal work to commit commit. Someone from your project decided to make life easier for their teammates and developed an extension for your project. The extension adds a button “Integrate bugfix into project X” into the web interface.
   
   • Collects your 6 commits in 1.
   • Will offer him a comment from the first commit.
   • Will offer to edit the comment.
   • Checks if there are merge conflicts with the main branch of the project that has gone ahead, or the branch for bug fixes.
   • Commit the fix to the branch for bug fixes.
   • Closes the review in Critic.
   • Will write something good in BTS.
   • Closes the bug in BTS.
   
   
5. Critic is not a wrapper around the main project repository, such as Gerrit. Therefore, Critic can be used for both pre-commit review and post-commit review, as convenient for your organization. Critic, however, can pull up code from the main repository, or share one repository with it. And also push the code there with the help of extensions, as shown in the previous paragraph.
   

I hope Critic will be useful to you too. Good luck in the development!