I've just went through the patch upload process (for proposed Code of Conduct https://codereview.appspot.com/575620043/) and I agree that it should be changed. It was obscure and confusing even for me.
cheers, Janek wt., 4 lut 2020 o 22:57 Han-Wen Nienhuys <hanw...@gmail.com> napisał(a): > 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 >
