Wednesday, January 2, 2008
I got Karl Fogel's excellent Producing Open Source Software for Christmas and have been enjoying reading and learning from it. One suggestion he has is to have a svn-commits list that includes full diffs to allow for fast, easy code reviews. I brought this up on the GNOME infrastructure list, where Olav Vitters raised some concerns and asked me to solicit planet-wide opinion. As a project leader and/or svn-commits subscriber, would you like to see full diffs?
By e3000, used under the CC BY-SA license.
I think this simple idea could help projects (like Banshee) add more committers and increase the pace of progress since every commit is reviewed by many eyes, most commits will be good, and you can always revert ones that aren't.
Assuming there is enough support for the idea, the next step will be modifying the script to pull configuration data from some yet-to-be-implemented DOAP setup.
Subscribe to: Post Comments (Atom)
I would be in favor of full diffs in mail, and use mailman to limit mail size to under some reasonable size... like 40k.ReplyDelete
We use full diffs in our commit mail at work and it has proven very useful, many users reviewing all he commits.
yeah, I'd actually like that tooReplyDelete
Mono has been using this (full patches emailed to the list) for about 5 years now.ReplyDelete
Patch sizes are barely a problem, they do happen every once in a while, like every six months, but they are in general very manageable.
Having patches posted is great, it allows for better async development (commit, then review) and it is also useful to see what is happening beyond ChangeLogs.
It is hard to imagine going back to patch-less commit messages.
There's an even better way: Email links to the diff in ViewCVS (or whatever). Example:ReplyDelete
This keeps the mail small, plus you are even more flexible.
Samuel, yes, I'd rather just have them on the website. I don't even need an email telling me about new changes if I can check the website.ReplyDelete
But, as your link shows, viewvs doesn't show the full diff - it just gives you links to diffs for each file. This makes code review hard. The old bonsai/lxr system did this wonderfully and I haven't been able to do proper code review since we lost it.
I like getting the diff in my mail box. At least if the diff is short (say, shorter than 10k).ReplyDelete
As for archives being big that Olav raised, well, svn-commits-list is not archived anymore.
One of the projects I'm involved in uses Trac as a web interface for project management, and this has a VCS log interface. Part of my regular work-flow is to look through this (including reading the diffs) and see what is going on. Of course, annotation is an issue.ReplyDelete
As to commit-then-review, some people seem to be amazingly hostile to the idea. I think it probably depends on how one can test the code in question.
The HAIKU project (in which I'm involved) uses a commit-mailing list, and it is very useful in my opinion. I think every project should use such as feature.ReplyDelete
behdad: better check before trying to correct me ;)ReplyDelete
I don't agree with it being archived though. We're replicating information already provided by viewvc.
Note that viewvc links are provided, although they don't show the whole diff. Up to upstream to fix that... development is a bit slow.
If you are trying to encourage many people to closely, regularly follow the changes in svn, you simply can't expect them to click one (or more) link for every commit - they want to read it right then and there in their browser, to be able to hit r to reply, and have the patch already there to cut up.ReplyDelete
I can see that not every project would want to try this; I think it should be optional.
I assume Anonymous was Olav. Right, seems like svn-commits-list is indeed archived. It was cvs-po-commits-list that got its archival disabled. I just remember one of the commit archives being disabled and I argued against it. Anyway, if svn-commits-list archives are an issue, they can be disabled. Right?ReplyDelete
As for click or not click, I use Evolution exactly to have my mail locally on my laptop when I'm in a train or coffee shop or any other gazillion places without internet connection. The needed click totally negates the point. And like people pointed out, it's not a matter of one click even.
If the idea is to review code, why not use something like reviewboard --ReplyDelete
I little commit-mail to review request creation script, and you're good to go. The nice part being that comments can be left on a line/block basis and are archived with the diff rather than in the mail thread.
We typically review code on bugzilla. commit-list is really that: commit list. That is, I want to see anything committed to this and that and that other module, even if I didn't participate in the review process, I want to do sanity check of what went in, or just to keep updated.ReplyDelete
With patch in the mail, I can simply hit reply and say so if anything's wrong.