From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id A3A7D1381F4 for ; Mon, 10 Dec 2012 03:02:14 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 181AA21C004 for ; Mon, 10 Dec 2012 03:02:13 +0000 (UTC) Received: from foo.stuge.se (foo.stuge.se [212.116.89.98]) by pigeon.gentoo.org (Postfix) with SMTP id 7F6AEE0698 for ; Mon, 10 Dec 2012 02:49:24 +0000 (UTC) Received: (qmail 20298 invoked by uid 501); 10 Dec 2012 02:49:22 -0000 Message-ID: <20121210024922.20297.qmail@stuge.se> Date: Mon, 10 Dec 2012 03:49:22 +0100 From: Peter Stuge To: Matt Turner Cc: gentoo-catalyst@lists.gentoo.org, zerochaos@gentoo.org, Sebastian Pipping , "Jorge Manuel B. S. Vicetto" Subject: Re: [gentoo-catalyst] On catalyst's development process Mail-Followup-To: Matt Turner , gentoo-catalyst@lists.gentoo.org, zerochaos@gentoo.org, Sebastian Pipping , "Jorge Manuel B. S. Vicetto" References: Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-catalyst@lists.gentoo.org Reply-to: gentoo-catalyst@lists.gentoo.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Archives-Salt: faa4c90a-2f19-4ebe-ad9d-e9cd9c6b70b8 X-Archives-Hash: 9d094c8d5f18376ef09c3a19ffb728db 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