development process

[Han-Wen Nienhuys]

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 - address@hidden - http://www.xs4all.nl/~hanwen