Hacker News Reader

Comment by eridius | Hacker News Reader

I really want something that provides better code review than GitHub. The described code review features of Gerrit sound promising. But the article says you can't submit a series of commits for review as a unit, you only submit a single commit. Is that really true? That seems like a rather awful limitation of the system. Sometimes my changes work well as a single commit, but often, especially when doing more complicated things, it's much more preferable to use a handful of related commits, all of which should get reviewed and merged as a batch. Does Gerrit not support this?

View on HN

With Gerrit you can chain up a series of dependent changes. The OpenStack Developer's Guide summarizes the workflow pretty well - http://docs.openstack.org/infra/manual/developers.html#addin....

The OpenStack community uses Gerrit pretty widely across our various projects. It might help to check out a busy project like Nova (https://review.openstack.org/#/q/project:openstack/nova,n,z) to get a feel for how Gerrit works in practice. Or a less-busy project like Bandit, which I'm involved in (https://review.openstack.org/#/q/project:openstack/bandit,n,...).

Phabricator[0] is awesome. Diffs (like PRs) contain several commits, but are reviewed, discussed, and "landed" as a unit with an auto-generated commit message referring to the Diff description and a link to read its history, but "master" is a linear sequence of Diffs being landed. You can also configure all kinds of rules like "X person must sign off on any changes to this file" or "do not merge code to master unless it is approved by N people other than the author." The UI is a bit clunky but I love it, and it's open source and very flexible.

[0] http://phabricator.org/

I used phabricator at my last job and was really impressed. Now I'm using Gerrit and I'm less impressed (but still getting used to it).

You say clunky, but Phabricator's UI makes Gerrit look like a WAP app.

When you say a linear sequence of Diffs being landed, do you mean it commits squashed merges, or does it create actual merges (so the history of master includes all the commits in each Diff)?

AFAIK this is configurable. We did squashed merges, which I quite liked, because "git log" for master read nicely but you could go look up the more granular history if you wanted to.

hopefully it's slightly less clunky today, just wrapped up the semi-annual refresh this past week. Still... we have our fair share of "-isms".

You might want to check out https://reviewable.io. It has most (if not all) of the goodness of Gerrit, but is trivial to set up (SaaS) and integrates smoothly with GitHub. Every PR becomes a review and gets automatically updated whenever you push to the branch.

Disclosure: I'm the founder.

Worth noting that Servo uses Reviewable, if someone's looking for an example of its use in a large, active project with many contributors: https://github.com/servo/servo/pulls

Wow, the described feature set sounds pretty good. I'll definitely look into this.

However, I will say the demo is a bit odd. It's pretty much impossible to look at the code diff because there are comments everywhere. And the code diff appears to default to not actually showing a diff (the left and right diff bounds are both set to the latest version), which is especially confusing when it shows side-by-side since it's showing the same revision on both sides.

Sorry about the mess on the demo review -- since everybody gets write access to try things out, it tends to get messy over time. I just reset it now so it looks clean again, and should probably just stick the reset script in a cron job...

It's really odd that you got a nil default diff range. I can't reproduce it with either anonymous or authenticated access. If you can, could you please open an issue with more details so I can debug? Thanks!

Thanks, the demo review is now much easier to read.

Also, now that you've reset it, all the diff ranges are now defaulting to the widest view instead of defaulting to just the current revision. Given that I can no longer reproduce it, I'm not sure there's any more detail I can add (beyond the fact that I'm using Safari 9.1 on OS X 10.11.4).

Our team started using reviewable.io a few weeks ago, and I quite like it so far. It solves the "squash/rebase->force-push->lose history" problem with Github PR review.

I've been working on a code-review tool that does exactly what you want: https://www.omniref.com/code_review

It works seamlessly with GitHub, and provides code reviews that end up "sticking" to your code, and documenting its development. Every pull request you create can become a review automatically, or you can pick-and-choose which pull requests to review. And you don't lose reviews or comments when you push to a branch under review.

Also, it lets you dive into the history of a single line of code. For example, here's a line from GitHub's libgit project, annotated with the pull request that created it: https://www.omniref.com/repositories/libgit2/rugged/files/li...

Looks promising. Does it handle force-pushes to PRs? It says it doesn't lose comments when pushing new commits, but it doesn't mention how it handles history rewriting.

It doesn't lose track of state when you force push. You'll see the old commits in the history, and the force-pushed commit, and the comments will still be on the old commits, the issues will still be active, etc. This isn't perfect, but it's a start, and it's better than GitHub.

It's on my feature short-list to write the code to make a best-effort to migrate review comments for rebased commits. But this will always be a bit of a heuristic, and error-prone. If the diff changes substantially as part of a rebase, it's really hard (i.e. theoretically impossible) to always know where to move annotations.

Ultimately, all you really know when you're on the receiving end of a force-push is that some commits were orphaned by a new commit. You can detect this and identify the orphaned commits, but knowing where to move the sticky notes on those orphaned commits is challenging.

You can push up a series of commits, but each one is it's own code review so you want to do it less often. Another thing is if you have to add something to the HEAD^^ you have to rearrange your history and squash things together, but in the end the history is really clean. Also, each push of of each commit is versioned so you can easily tell what has changed between the last code review and the current; this is impossible with GitHub if you want to keep your history clean and compare different versions of the code review.

I'd suggest you take a look at phabricator too which has great code review and issue tracking. It does unfortunately still lack support for a first class notion of a patch series.

It does. Dependant changes/commits can be pushed at once, and if they are submitted out of order, they will be queued until the dependant changes are submitted/merged. In recent gertit releases, you can even submit (merge) all changes in a particular topic at once.

Reply on news.ycombinator.com