Re: [gentoo-catalyst] On catalyst's development process

[Peter Stuge]

Sorry for the long mail. I am hugely supportive of using Gerrit.


Matt Turner wrote:
> I find this model tedious

Why tedious? It sounds like patches do get near-instant review, which
is highly desirable if at all possible.


> and lends itself to rapid-fire question and answer debate about a
> proposed change, partly due to the medium,

Why is that a bad thing?


> partly due to pastebin'd patches having a significantly lesser
> chance of containing a meaningful commit summary.

Why does that matter much? I expect that they will come with a good
commit message when they go into the repo?


> I claim that if patches were sent to the mailing list they would
> receive better review from a larger reviewer base and the committed
> results would contain fewer errors.

Sorry, but I do not agree at all.

I am a very strong proponent of code review being a hard requirement
for anything being included in any repository. I have used various
systems for accomplishing or encouraging review in a few different
teams.

As much as I wish that code quality and code review would only
require the right process, it doesn't. It all comes down to the
people. No matter what the process is, if people want to commit
then they will commit.


> The process of git format-patch/send-email forces the author to at
> least look at the patch before committing,

No? The process of git diff or git show allows the author to look at
the patch before publishing it, but it is absolutely impossible to
force an author to critically review anything. Authors will review
their commits only if they want to, and if they want to they do not
need to be forced. Unfortunately my experience is that nearly noone
wants to, or can, review code. Neither their own nor someone else's.


> and reviewers are likely to give a better review to something they
> find on a mailing list than in IRC.

I disagree with this too. I think the reviewers should consider how
on IRC they are actually *in* the process of the developer doing the
work, that is the absolutely ideal spot for review. Once the email is
out, the developer has task switched away from that commit.

Another way to view this is that somehow email-driven review by
neccessity means longer idle periods between each run of the task for
everyone involved, both developer and reviewers. I find this
extremely undesirable. I prefer working fast and hard on something,
finish it perfectly, and move on. I think the key to success with
that is to have small enough somethings that there are no blockers.

I want review to be part of it, I really do want review, but that
requires people who are open to task switching and review sprinting,
if you will, for my commit.


> At least personally, I find that I'm much more likely to forget
> about something asked of me on IRC than via email.

IMO the problem with email is that the queue is very very long and
for many people (at least for me) it is constantly growing.


> Patch review isn't about a power structure. It's about maintaining
> quality and catching mistakes before they could affect anyone.

Of course. But unfortunately everyone who does not already want to do
review percept review to be *exactly* about a power structure. There
is a huge gap, and closing that gap is a difficult training problem,
which has nothing to do with coding or reviewing. :(


> git is also not a review tool. Once it's in git, it's history, and I
> don't like my mistakes being part of historical record. :)

Yes and no. Gerrit is a git-native review tool. It allows to iterate
changes at the cost of a Change-Id line in the commit message so that
mistakes do not have to make it into the repository.


> Chrome, I think, and other Google projects use a web-based review
> tool called gerrit.

Gerrit originated from the Android project.

Note that Gerrit is *not* strictly web-based.

Gerrit is a Java .war file with lots of stuff built-in, it does offer
an HTTP server, but it also offers an SSH server as well as a Git
server. It has native implementations of all these protocols and it
allows to create very nice - though not perfect - workflows.

I am a huge fan of Gerrit. It is by far the best system for code
review that I have seen.


> Either of these would be large improvements, with the mailing list
> system having lower cost and probably the same effectiveness.

The way I like to use Gerrit is as follows:

I use git to push my commit(s) to Gerrit.
Gerrit sends email(s) to the mailing list including the commitdiff.
People can use the web interface *or* SSH to comment on the commit.
People can use the web interface *or* SSH to include or veto the commit.

There are two major things that Gerrit is missing:

* The web interface allows making inline comments on every diff line.
  The SSH interface does not allow making inline comments.

* When coupling Gerrit with a tool like Jenkins to do verification of
  commits (anything from a build smoke test to running test suites)
  it is unfortunately not possible to instruct Gerrit to include a
  commit until *after* the verification has completed. I am missing
  the possibility to say "if verification passes, please include."
  It can kinda-sorta be accomplished, but not as nicely as I'd like.

Those two problems aside, I very much like using Gerrit, and in at
least two projects where it has been introduced it has in fact led
to more review and participation.

In both projects it has done nothing (and can do nothing) to keep the
people who do not care much about code quality from committing shit.

The only way to filter what goes into the repo is by skilled human.

Skilled humans are rare, and on top of that everyone who cares less
about code quality than the skilled human will get pissed off by how
(perceived) negative and/or critical and/or conservative and/or
demanding the skilled human is. That's also not a lot of fun.


//Peter