Code Reviews and Bad Habits

I couldn't disagree more.

First of all, Gerrit groups the related commits together in two ways:

1) As a list of dependencies: "This commit depends on; this commit is depended on by."

2) If you develop your feature on a topic branch (and you should), when you push the commits to gerrit you can (and should) specify the topic. The related changes are then grouped by topic. ("git push origin HEAD:refs/for/master/topic_name")

Second, the reason this workflow is superior is that it requires each commit to be individually reviewed (and hopefully verified), and what ends up getting merged is a clean topic branch, with no bad commits.

Contrast this to the other workflow, and I'll use GitHub's Pull Request as the example. Typically what's reviewed is not the individual commits on a topic branch, but rather just the diff between the tip of that branch and the merge base between it and where it's to be merged. In this workflow the individual commits are often not even looked at. So you can have this series of commits on a topic branch:

   1. Commit A
   2. Fixup to commit A
   3. Commit B
   4. Commit C
   5. Fixup to commit B

When reviewing that in GitHub, you'd typically just review the total diff introduced by all 5 commits, and if it looks good, you merge all 5. Also, typically the CI system builds just 5 or the result of 5 merged into its destination branch. So now your history has the two fixup commits in it that don't belong there.

With the Gerrit-style workflow the fixups never enter the main branch, since instead you'd have squashed them into their respective commits. You'd also hopefully configure your CI system to verify each commit individually.

Further, on a decently sized topic, I find it easier to review a series of smaller changes, vs a huge diff.

It should be noted that this style of reviewing individual diffs is inspired by how reviews are done over mailing lists (Linux, Git, etc).

That said, once all the commits on a topic have been reviewed and perfected, it is often nice to be able to see the final diff of what is to be merged. With Gerrit you'd have to do that externally (via gitweb or by downloading the topic branch and diffing it locally against its destination).

Have this as a feature in Gerrit proper has been discussed and is somewhat contentious among its developers - https://groups.google.com/forum/#!topic/repo-discuss/FBJl5wW...

We've been using Opera Critic (https://github.com/jensl/critic) where I work for about half a year. We did trial runs with Gerrit, Barkeep and Phabricator before settling on it. My impression is that its almost completely unknown outside of Opera, which is surprising given the feature set, clean UI and active development. The site used to review branches for the Critic project is also a demo: https://critic-review.org

It supports the model of having many atomic, reviewable commits on a topic branch. Where it seems to exceed Kiln is in the rebase/fixup workflow. You can push fixup commits in response to issues left by a reviewer. A fixup commit will automatically mark an issue it addresses as resolved when you push based on the lines it changes (the fixup must also be reviewed). You can later rebase the branch to integrate the fixups. It's pretty clever, and the way fixups are shown lets you see at a glance how the branch has evolved.

As a reviewer, and as someone who often digs through five year old commits, I want feature branches to follow the most linear path possible. Approaches that are started and abandoned, and commits made on top of a branch in response to review feedback are noise. A well-crafted branch often has little to do with what happened during development.

I disagree with this; you make one patch for your reviewer to review. If you want to do two things, make two patches.

The history of how you made that patch should be irrelevant; if you care, your patch is too big. Split it up.

I will admit that this takes some getting used to. When I use git alone, I commit for every little thing I change. But that's not actually useful for other people to look at; what people really want are high-level patches that represent logical changes, not just physical edits of the source code.

I've also gotten myself into a lot of trouble by committing a lot of things and then splitting them up for review. This always takes a very long time.

So just focus on one thing at a time, send it for review, and make sure your coworkers believe that reviews are the most important thing they can be doing. Then you get the small commits you like, but also get a more readable history, and better feedback from reviewers.

> The whole point of a source control system is to tell you the history of how code got to be the way it was.

No, the point of source control is to record incremental improvements in a codebase. This is a subtle difference, but critical: the history of how codebase got from point A to point B isn't valuable in itself - the logical changes that altered the behavior of the project are.

A series of broken commits might show the developer's journey from zero to 100%, but the idea that each commit is a non-broken incremental addition enables powerful automated workflows for regression testing.

For what it's worth, Review Board has a way to visualize multiple revisions in a single code review request. I think it is primarily intended for the patch writer and reviewers to go back-and-forth (like "ok, here's my fix based on you comments"), but with some scripting you could make your own tool that uploads a whole series of revisions to Review Board.

100% agree. It's especially hard for teams that are learning both Git and Gerrit at the same time. I both administer and provide support for the Gerrit instance where I work and it has become a huge exercise in babysitting permissions configuration on the server side and dealing with angry users. They get themselves in to lots of "stuck" situations due to a lack of understanding, and it is now possibly building in to a revolt. For the record, I didn't pick Gerrit! :-) Fun times.

I'm pretty surprised to read this, I've used Rietveld (Gerrit is a fork from Rietveld) and it is entirely possible to review more than one commit at the time (with the option to review them as a unique patch or as individual steps)

But what I love is the option of squashing the commits (or perform any other operation), as the review is independent from the repo, meaning that I can push a clean commit instead of adding a lot of commit fixing issues, which gets a dirty commit tree.

That can be considered as "not showing the full code history", but I prefer to think about it as "only pushing to the repo code that sort of makes sense instead of lots of confusing ver small commits fixing typos"

What are the advantage to using Gerret, Kiln, etc for code review? Is it for people who using hg/git/etc without a web interface?

The way I normally do them is you submit your pull request on github or bitbucket and everyone on the team makes suggestions and asks questions until they are satisfied.

Once they are happy they comment "LGTM". Once you have a LGTM from your team members, you pull your pull request.

tl;dr Gerrit only allows you to comment on one commit at a time, not a set of commits.

This sounds to me like a case of a people problem being blamed on tools.