As promised in several code reviews, here my take on the current development process. I wrote it as a google doc first, so you can also go here https://docs.google.com/document/d/1BSffzjiQKMTTmr988ezMbsJyfwT9S-rxGRbYSBTv3PM/edit for inline comments.
Context: - https://codereview.appspot.com/561390043/#msg32 - https://docs.google.com/document/d/1zYwygWEKJt21rl-bKL41IpjFmiBjlfob6eS63jd-62I/edit# My experiences with the current Lilypond development process. For context, I have a busy daytime job. I work 80% so I can set aside a couple of hours of concentrated hacking time on Friday. When I am in my element, I can easily churn out a dozen commits in a day. Those often touch the same files, because a fix often needs a preparatory cleanup (“dependent changes”). My annoyances so far are especially with the review/countdown process : - Rietveld + git-cl doesn’t support dependent changes. So to make do, I explode my series of changes in individual changes to be reviewed (I currently have 34 branches each with a different commit so git-cl can match up branch names and reviews). For dependent changes, I have to shepherd the base change through review, wait for it to be submitted, and then rebase the next change in the series on origin/master. - Because the review happens on the split-out patches, I now switch back and forth between 34 different versions of LilyPond. Every time I address a review comment, I go through a lengthy recompile. The large number of changes clogs up my git branch view. - The countdown introduces an extra delay of 2 days in this already cumbersome process. - The review process leaves changes in an unclear state. If Werner says LGTM, but then Dan and David have complaints, do I have to address the comments, or is change already OK to go in? - We track the status of each review in a different system (the bug tracker), but the two aren’t linked in an obvious way: I can’t navigate from the review to the tracker, for example. Some things (eg. LGTM) are to be done in the review tool, but some other things should be in the bugtracker. - Using the bug tracker to track reviews is new to me. It is common for a bug to need multiple changes to be fixed. It also adds another hurdle to new developers (setting a sourceforge account and getting that added to the project). - I have to keep track of my own dashboard: once changes are pushed, I have to look them up in Rietveld to click the ‘close’ button. - Rietveld and my local commits are not linked. If I change my commits, I update my commit message. I have to copy those changes out to Rietveld by hand, and it took me quite a while to find the right button (“edit issue”, somewhere in the corner). Some of my complaints come from having to manage a plethora of changes, but I suspect the process will trip new contributors up too, to note: - Seeing your code submitted to a project is what makes coders tick. It is the Dopamine hit Facebook exploits so well, and so should we. The key to exploiting it is instant gratification, so we should get rid of artificial delays - We use “we’ll push if there are no complaints” for contributions. I think this is harmful to contributors, because it doesn’t give contributors a clear feedback mechanism if they should continue down a path. It is harmful to the project, because we can end up adopting code that we don’t understand. - The whole constellation of tools and processes (bug tracker for managing patch review, patch testing that seems to involve humans, Rietveld for patches, countdown, then push to staging) is odd and different from everything else. For contributing, you need to be very passionate about music typography, because otherwise, you won’t have the energy to invest in how the process works. David uses a patch <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=17474> he made to GUILE as an example that code can be stuck in a limbo. I disagree with this assessment. To me it shows the GUILE community considering and then rejecting a patch (comment #26 and #40). I imagine that David was not happy about this particular decision, but I see a process working as expected. If anything, it took too long and was not explicit enough in rejecting the patch. Also, in cases like these, one would normally build consensus about the plan before going off and writing a patch. David suggests that I like getting patches in by fiat/countdown, but I disagree. If you look at the past 2 weeks, you can see that I have actually tried to address all code review comments so far, and again, it is more important for the project to have explicit consensus than that individual contributors that go off in possibly conflicting directions. Here is what a development process should look to me - Uncontroversial changes can be submitted immediately after a maintainer has LGTM’d it, and automated tests pass. Merging such a change should be an effortless single click, and should not involve mucking with the git command line. Because submitting requires no effort, we won’t have patches stuck because we’re too lazy to do the work of merging them. - There is a central place where I can see all my outstanding code reviews, my own, but also from other people. This means that I can do reviews if I have some time left. - The review tool supports chains of commits natively. - The review tool supports painless reverts. When it is easy to fix up mistakes, contributors will feel less afraid to make changes. - Right now, results from build/test are available during review. This is a good thing, and we should keep it. - There is no “lack of feedback means it goes in”. By accepting a code contribution we implicitly take on the duty to maintain it and fix bugs in it. If no maintainer can be convinced a piece of code is worth taking it on, then that is a long-term maintenance risk, and it should be rejected. - Coordinated, larger efforts across the code base should start out with a discussion. The mailing list could work here, but I find discussion in an issue tracker is often easier to manage, because it is easier to search, and by its nature, discussion in an issue tracker drifts less off-topic. - We have a patch meister whose job it is to hound the project maintainer to look at patches that don’t find traction or otherwise. -- Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen