Reasons why Crucible/Fisheye is better than Github (Enterprise) Pull Requests for reviewing code

GithubPullRequestDeficiencies.md

GENERAL:

• You can't include commits from different repos in a single code review using github pull requests, whereas you can with Crucible code reviews.
• It's risky to use a pull request for code that isn't ready to be merged yet. What if someone doesn't see the "DON'T MERGE" comment/title, and clicks the big green merge button?

VIEWING:

• You can't adjust the line width in which files are displayed, even if you make your browser window wider than the page. Some XML/HTML content extends well past 80 characters.
  • There are numerous Chrome extensions which purport to provide this feature, but I haven't been able to get any of them to work for Github:Enterprise.
• There is no toggle in the Github UI that lets you ignore white space, so if a change involved indenting a lot of lines, you'll have a hard time figuring out what changed. In Crucible, you can choose to ignore white space.
  • You can add ?w=1 to any diff URL to ignore whitespace, however. Not sure why Github hides this behind a secret query param, but thanks to @drapostolos for the tip.
• You can't see the full file name, if the file name and path are longer than ~70 characters, and the HTML element that contains the file name is absolutely sized so you can't even expand your browser window.
  • You can hover over the file name to see the full name, but then you can only see one file name at a time.
• No tree view to see files, so if your CR contains lots of files changing, it's a PITA to switch back and forth between them.
  • Crucible reloads the page when you view a different file, so you can use the browsers back button to quickly jump back to the last file you were viewing.
  • The github UI slows down considerably (e.g. when typing a comment) for pull requests with lots of changes (over 1K lines changing).

COMMITS:

• If there have been multiple commits on the branch that the code review is based off of (e.g. 1 commit to start, and subsequent commits to address code review feedback), Crucible provides me with a slider to choose the exact set of (consecutive) commits I want to view. Pull Requests display all of them, or I can view one at a time.
• Instead of displaying the actual date and time a commit was authored, Github displays "john.doe authored a month ago". That's fine some times, but other times I'd really like to know the actual date and time, because it matters.

COMMENTS:

• Adding a new commit to a PR that changes an existing file, hides all comments for that file. So if I make 3 comments to a file in a PR giving 3 different things to fix, and the developer addresses 2 of my comments and updates the PR with a new revision of that file, I can't compare my comments to his changes. This prevents me from seeing his 2 changes in the context of my original comments about them, and also I can't see that he never addressed my 3rd comment.
  • Crucible doesn't hide a comment unless the line it is tied to is changed.
• Top-level comments are not threaded, so you can't reply directly to a single comment. This can get confusing if there are a lot of top-level (i.e. not line) comments in the PR.

NOTIFICATIONS:

• Email notifications are sporadic. When someone comments on a PR, I USUALLY get an email notification, but not always.
• Github doesn't send you a notification email when a new commit is added to a PR, or when an existing commit is updated (i.e. via a force push). Unlike Crucible, Github has no built-in concept of a reviewer "finishing" his review, or tracking the percentage of files that a reviewer has seen, so it can lead to more ambiguity about the status of a code review.
• Comment notifications don't include the previous comments in the chain, so you'll get a notification about the author saying "Sounds good, will do" and have no context of what it's about unless you open up the PR. Crucible displays the full comment thread in notification emails.

BOTTOM LINE: Crucible seems like it's been purpose-built for reviewing code. Github Pull Requests seem like they exist to support a process for sharing code between users or repos, but not for actually making code reviews easier.

However, Crucible has problems/downsides as well, as compared to pull requests:

• The code that's reviewed in a Crucible code review is not guaranteed to be the same code that's merged to master once the review is finished. Nothing prevents the code author from making changes before or during the process of squashing the commits and pushing the single commit to master. (However, you could still use a pull request for actually merging the code, with someone performing a final once-over of the code.) This only affects usage of Crucible as a pre-commit review tool.
• Crucible works best if you use merges to pull the latest commits from master onto the code review branch, instead of rebasing. If you rebase your branch, and then later add another commit which changes an existing file in the code review, Crucible will lose track of which file in the review is changing, and add the file again. The result is that you'll see 2 Foo.java files e.g., with pre-rebase commits on the first file, and your commit after the rebase on the 2nd file. For this reason it's recommended to only merge commits on your code review branch, and save rebasing for when you squash your commits and are ready to merge them to master (via push or pull request).
• Crucible depends on your git repository being indexed by Fisheye. For an organization with a lot of repos, this means your personal fork is probably not going to exist in Fisheye. So in order to add your commits to Crucible, you'll first need to add each repository to Fisheye (one-time step per repo). Then you'll need to push your work branch to the main upstream repo that's indexed by Fisheye; i.e., the same repo that contains the master branch you'll eventually merge your code to. This requires every engineer who wants to create a Crucible code review to have push access to the upstream repo containing the code (because AFAICT git doesn't provide a way to grant per-branch permissions within a repo). If your organization wants to lock down permissions, and only provide push permissions to a subset of your engineers, then the other engineers will have to use pull requests instead of Crucible (or have someone else push their commits from their work branch to the main upstream repo).
• After you push your commit(s) to the upstream repo, it will sometimes take Fisheye 5-10 minutes to add your commits to its index, which is required before you can add them to your code review in Crucible. Whereas with github, within seconds of pushing your commit(s) to your remote repo, you'll be able to create the pull request.
• Crucible code reviews usually a little take more time to setup than pull requests. Both involve pushing your commit(s) to a remote repo, and clicking a couple of buttons on a web page. However, with Crucible you typically want to improve your code review's "objectives" section, choose 1 or more engineers to participate, and potentially add some comments about areas you want particular focus on, before starting the review. However, these extra steps can make for a better code review, IMO.
• Crucible can be slow, when the review contains hundreds of files. However, github pull requests suffer the same problem IME.

White space can be ignored in github by Adding ?w=1 to any diff URL.
See github-secrets.

Thanks @drapostolos, I updated the gist accordingly. Sorry I'm just seeing your commit now; github doesn't provide notification emails for gist comments. :-(

Initially I had one question: is it really necessary to manually add every commit to the review. The answer seems to be 'yes', and I think that's big drawback. Then I read the first bullet point, and that gave me pause. Why would you want to include another commit into a review? The summary indicates the answer; Crucible is good for reviewing change sets, not necessarily for evaluating feature code about to be merged.

@skingsland Re: Crucible & merge/rebasing: isn't mixing merges with rebases dangerous? If you encounter a merge conflict, anything you resolve in the subsequent merge commit will be lost when rebasing when you have to fix the issue in the context of the specific commit that caused it. You may not only lose history when doing so, but lose actual code in the original merge commit.

Is there a way around this? I would love to try using rebase with crucible, but as of now it just seems too risky.

Crucible is good for reviewing change sets, not necessarily for evaluating feature code about to be merged.

@pstephenwille: that's a fair assessment, but most "feature code about to be merged" needs some changes, often minor but occasionally more significant, before the code is ready to be merged. So the author can update the commit(s) and force push to his PR branch, or they can add a "fixup" commit which will need to be squashed before merging. If you go with the former, it makes it really hard to see what's changed in response to the comments. If you go with the latter, then Crucible works well for reviewing the change set across multiple commits, e.g. getting notified when the commit is added, easily being to able to see the changes just in that recent commit, etc.

@brandonw: I'm suggesting that you only use merges with Crucible, and then squash your commits (if necessary) and rebase off the latest HEAD of master before opening the pull request. That way you benefit from the code review features of Crucible, while still getting one last chance in the pull request to see exactly what's going to get merged to master.

Github doesn't send you a notification email when a new commit is added to a PR

Admittedly we're using GitHub Enterprise, but I do get emails about additional commits being pushed to a PR without anyone having to manually trigger them.

Initially I had one question: is it really necessary to manually add every commit to the review. The answer seems to be 'yes', and I think that's big drawback.

@pstephenwille, That is actually not true. You can add by branch name, and as Fisheye polls for changes to the repo, it will pick up changes within the branch and add them automatically to your review.