Han-Wen Nienhuys <hanw...@gmail.com> writes: > 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.
Changes belonging to the same topic should be committed to the same Rietveld review and Sourceforge issue. One can commit them in sequence to Rietveld when it is desirable to view them independently. This does not allow to view fixes resulting from discussion in the context of the ultimately resulting commit chain (which will usually be fixed per-commit with git rebase -i). For a sequence of commits belonging to one logical change, this is the somewhat defective way of doing stuff. It's not as bad as you happened to use it, but it definitely is a tool that grew on Subversion and added Git as an afterthought. Where commits do not belong to the same logical issue but are still interdependent, they cannot be logically disentangled even using a Git-specific tool like Gerrit. > 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. Recompiles tend to be very fast unless you "make clean" in between or check out, in the same work tree, vastly differing branches, like switching between stable and unstable branches. Or bisecting across a version change. It's annoying how much just depends on the VERSION file, but not actually something that the review tool will help with. > 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? The change is ok to go in when it has been approved for pushing. I find the idea of ignoring instead of addressing complaints weird. > 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. We don't need to rehash that the current system sucks. We had to amend our process after relying on a proprietary tool, namely Google Code, ended up hanging us out to dry and we had to replace the parts removed. Our available knowledge and volunteer power ended up with the current setup which was not intended as a keeper. It kept the project working, but I doubt many people will be sad to see it replaced. > 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). Then you have set it up incompletely or use it wrong. git cl upload will copy the current change set to Rietveld. I am impressed at the rate of change you manage to churn out without relying on the commands we use for that purpose, but this certainly can be done less painfully. > 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. They get feedback when the code is getting reviewed. If code does not get reviewed, having their changes dropped on the floor is not going to increase their enthusiasm. And just above you wanted to know when you are free to ignore feedback. > It is harmful to the project, because we can end up adopting code > that we don’t understand. - Most contributors leave the code in a better documented state than they got to work with. I am probably guilty for most contributions of "code that we don't understand" by condensing complexity into few places in order to create large user-accessible swaths of code people _can_ understand, like making music functions much more powerful and generic than they were, making large amounts of LilyPond accessible to Scheme programming and extension, at the cost of making lily/parser.yy quite more complex. In contrast to previous coding practice, my changes are quite thoroughly documented, but it's still a real piece of work. Much of that work got accepted not because reviewers understood it (few reviewers are into Bison) but because people trusted me. In the end, that tends to be a considerable part of the currency of work, but new contributors cannot rely on it. With regard to "code that we don't understand": I had to completely redesign the internals of several corners of LilyPond because code was entered that not even the committer did understand but that had become rather popular. Chord repeats come to mind, nested property overrides and reverts, overrides inside of grace passages and a few other things. I cursed a lot having to come up with replacements for things that I could prove not workable. > 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. The really big problem of many free software projects is that the people going all-in as developer do not have enough time left in their life to be serious users of the software they are working with. Rietveld et al are not helping, but power users on our forums still got drawn into contributing. And much of their contributions could bypass any central vetting process if we had a package repository where people can take other people's code and combine it effortlessly, or leave it. > 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). Nope. It is an Andy-only domain, and Andy does not respond. That's all there is to it. You don't see any "wontfix" tag or other form of rejection. The issue remains open. > I imagine that David was not happy about this particular decision, but > I see a process working as expected. There was no decision. There were some comments which I addressed and put into context. > 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. Uh, I am banished from the Guile developer list. There is no way to build consensus. And I was merely implementing what Andy Wingo stated in a comment should be implemented, in the manner he stated it should be done. But that's a side track. As I already stated: my initial experience with contributing involved patches to LilyPond was that they were ignored because nobody considered themselves able or motivated to review them. Even simple changes took weeks to get accepted. For better or worse, there just aren't people responsible for large parts of the code that would be able or willing to judge it on its merits in the context of the LilyPond code base. I can on occasion work with active complex projects: you'll find that the bulk of git-blame's internals has been rewritten by me (a dumb promise I made to the Emacs developer list when the non-scaling performance of git-blame was a large impediment to moving from Bzr to Git) and I got it in. But the project is much more modular and active than LilyPond, including a hierarchy of developers that we just don't have. > David suggests that I like getting patches in by fiat/countdown, but I > disagree. That was likely inappropriate by me, sorry for that. I just pointed out that what you considered detrimental would work in your interest here. > Uncontroversial changes can be submitted immediately after a maintainer > has LGTM’d it, Two problems with that: what is uncontroversial? And who is a maintainer? You want less opportunity for people to raise objections, but how can you decide about something being uncontroversial when people don't get to review a patch and consider objections? > 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. - Like which patch? > 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. > - It's a good thing, I agree on that. "We should keep it" sounds like it is a mechanical thing and a decision we can make. It involves significant work currently done by James. And the automation he has available to that purpose is in a decrepit state. That's really an embarrassment. So we should not just "keep it" but hopefully also fix significant parts of it to make them less manual. This visual comparison is something that is unique to LilyPond as a typesetter, and there may be some effort involved getting a good workflow designed and implemented working with a different tool. The current scripts were designed to work with Google Code, and the migration to Sourceforge has not really been anywhere to complete. Whatever we end up with, I hope it takes a lot more of the mechanical workload off whoever does the visual comparisons than what we have now. > 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. Who is we? > 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. - Who are maintainers? > 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. That is not their current job description. -- David Kastrup
