Re: development process

[David Kastrup]

Han-Wen Nienhuys <address@hidden> 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