Pull Request code reviews need redesign

[Eric Hill]

We've been using BitBucket for about a month, and there are a lot of good things about it.  Code reviews in pull requests is not one of them.

If you've never used other code review products, you might think BitBucket's pull request code reviews are okay.  If you've used other code review products, however, you will not.

We came to BitBucket from Perforce/Helix, which has Swarm as its code review tool.  Swarm is light years better than BitBucket for code reviews.  Here are the primary reasons why:

1. In a BitBucket PR, comments and tasks are two different things.  If you want to create a task, you have to create a comment first, and then, from the comment, create a task.  This is incredibly unhelpful. If I want to create a task that says "Fix bug here", I first have to create a comment that says "Fix bug here" and then create a task from that that says "Fix bug here".  Completely unnecessary duplication.  Also, the owner of the PR gets a notification for each comment and each task, so that's two notifications for each task. If you create a comment and *don't* make it a task, it doesn't show up in list in the right panel, so it will get lost. I know there is a BCLOUD jira for being able to see the full list of comments, but, again, it's silly that that is a different list from the tasks.
   
   In Swarm, you create a comment, which you can turn into a task, or not.  You can view the full list of comments in the review, and see which ones are tasks.  If you decide the comment is not a task after all, you just mark it as Not a Task. Genius.  After you mark a task as Completed, the person who added the task can check it and mark it Verified.  You can easily see which tasks are open, completed, and verified.  Swarm also batches up e-mail notifications, so you might get one e-mail notification for 10 comments/tasks that a reviewer entered,  You don't *have* to verify the tasks, but if you want to, the Verified status helps you keep track of which tasks you have verified and which you haven't.
2. Swarm code reviews have a state that can be things like Needs Review and Needs Revision, that any reviewer or the owner can set.  So a reviewer can create a few tasks and mark the review Needs Revision.  That sends an e-mail to all reviewers and the owner that it is now in that state.  So the owner can fix all the tasks and set the state back to Needs Review, which sends a notification telling reviewers that they should go back and finish reviewing.  This is an extremely helpful communication mechanism.
   
   In BB PRs, a reviewer can create tasks and then set his/her own vote to Changes Requested.  but the owner of the review cannot change it back, so has no effective way to communicate that s/he is finished with the requested changes.  Changes Requested should not be a reviewer state; it should be a state on the PR itself.
   
   
3. Swarm has the concept of Required reviewers and Optional reviewers.  All Required reviewers must vote up the review for it to pass.  This is incredibly valuable.  Different code reviews are more important for certain people to review than others.  Being Optional rather than Required means you know you can ignore if it you don't have time.  It also helps us to spread out the work.  "I set Bill as required on my last large code review, with Mary optional.  So, on my new large code review, I'll make Mary required and Bill optional to give Bill a break.
   
   With BitBucket, I just add a several reviewers and see who volunteers.  It's not great.

The team did note one thing that BitBucket does better than Swarm (possibly related to the Git underpinnings).  In Swarm, if all your required reviewers have voted up your review and there is even the tiniest of merges, Swarm negates all of the up-votes.  BitBucket seems to be better about only invalidating existing up-votes if there is a major change.

Please re-imagine your code review process.  It is by far the weakest aspect of BitBucket we have noticed.

Thanks.

[Michael Rüegg _Mibex Software_]

Hi Eric,

While I cannot comment on 1) and 2), you can satisfy 3) with the concept of Code Owners.

Code Owners are the people in your team that are responsible for keeping the quality high in your code base. They would be the "required reviewers" in your example.

All the other reviewers would be the interested parties. For them, it is good to know about the PR for various reasons (education, knowledge spreading, etc), but you do not require their approval to move ahead. Code review speed is key to ship continuously and to prevent long waiting times causing merge conflicts etc.

If you don't mind a commercial app, you can achieve this with Code Owners for Bitbucket.

By our experience, not all Pull Requests require the same level of scrutiny though. With the Ship, Show, Ask branching strategy, you can combine the advantages of Pull Requests and fast feature shipping. If you are interested, we have a community article about the Ship, Show, Ask concept. And we also have an app for it: DevSensei for Bitbucket.

Hope this helps :-)

Best regards,

Michael (app vendor)