[gentoo-catalyst] On catalyst's development process

[gentoo-catalyst] On catalyst's development process

[Matt Turner]

Zero_Chaos and I had a discussion on IRC about catalyst's development
model. Part of the discussion was whether proposed changes should go
to the mailing list for review or if the current IRC+pastebin-based
review was sufficient. As a proponent of a mailing-list-based model, I
thought it best to attempt to continue the discussion on the mailing
list itself.

Since Andrew Gaffney stopped catalyst development in early 2010
catalyst has seen only small developments. Jorge has been mostly
single-handedly keeping catalyst going by supplying patches and bug
fixes.

Without many active developers (catalyst works okay, why change it,
right?) it's been hard to get others to do patch review and easy to
miss a tiny problem in your own code. For instance, I made a mistake
in commit c3db9351 (fix: 793079a0) that would have probably caused
mips/o32 stages to fail to build. Other problems have slipped through
as well: Zero Chaos was forced to revert three commits in order to
release a version of catalyst (2.0.12.1) that actually worked. At
least twice in the last 50 commits, changes have been made that
contained a syntax error that simply would have prevented catalyst
from working. Both times they've been fixed promptly and probably
didn't cause anyone any harm, but:

I think this leads to the question of how well our development model is working.

As it currently is patches are committed ad-hoc, recently with at
least some review in the form of giving a reviewer a pastebin link on
IRC. I find this model tedious and lends itself to rapid-fire question
and answer debate about a proposed change, partly due to the medium,
partly due to pastebin'd patches having a significantly lesser chance
of containing a meaningful commit summary.

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. The process of git
format-patch/send-email forces the author to at least look at the
patch before committing, and reviewers are likely to give a better
review to something they find on a mailing list than in IRC. At least
personally, I find that I'm much more likely to forget about something
asked of me on IRC than via email.

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

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. :)

Other projects handle review in different ways. The kernel and X.org
for instance require that a patch have at least one Reviewed-by tag,
stating that a third-party checked over the patch and found it
appropriate. Chrome, I think, and other Google projects use a
web-based review tool called gerrit. Either of these would be large
improvements, with the mailing list system having lower cost and
probably the same effectiveness.

See http://www.x.org/wiki/Development/Documentation/SubmittingPatches
for a description of the process.

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

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

[Matt Turner]

On Sun, Dec 9, 2012 at 6:49 PM, Peter Stuge <peter@stuge.se> wrote:
> 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.

[snip]

> 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.

I think these are the core points of your email. I agree that if
people have commit access that they -can- still commit shit. I don't
think this is a case worth worrying about now. We have other areas of
Gentoo covered by commit-only-if policies and they mostly work. Let's
start from the assumption that people won't intentionally disobey a
policy.