Re: development process

[Han-Wen Nienhuys]

On Wed, Feb 5, 2020 at 12:11 AM David Kastrup <address@hidden> wrote:

> 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.
>

Oh, but you can. You can review the change as part of the dependent change,
but then rebase or cherry-pick them if they can be submitted separately.

>    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.
>

Guess what you have to do when working on GUILE 2? You edit central headers
(recompile everything!) and after it works, compile against GUILE 1.8
(recompile everything!) to make sure it doesnt break the build.

>    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.
>



>    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.
>


>
> > 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.
>

To me, a phrase like "I don't have time now to continue arguing with you
about this issue. Sorry." means "No"


> > 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.
>

The process is not designed to address the lack of people. Rather, the idea
is that contributing to LilyPond is an experience that is easier and more
rewarding. From there, we might suck in more people because contributing is
fun and rewarding.

It is possible that we go back to a place where we can't accept changes to
intricate code (markup macros, music functions?) unless they drastically
simplify things, because no maintainers want to take on responsibility for
something that is hard to understand. I think that is an acceptable
tradeoff if we get more developers for the project overall.


> 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.
>

I have said this before, and I'll repeat: I find your assumptions about my
interests offending. Please stop making them. I have dedicated 10 years of
my life to LilyPond. I wrote most of its code, I spent countless hours as a
maintainer, community leader, and user support person. What do you think my
interest is?

>    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?
>

Maintainer: see below.

Here is a definition: patches are uncontroversial when a maintainer says
it's OK.

Here is another definition: a patch is uncontroversial when it improves
something and doesn't create long-term liability.

Here is a practical example: when you look at, say
https://codereview.appspot.com/577410045/, there is considerable discussion
about a detail of using references vs. pointers, but the overall cleanup
(adding comments, renaming variables) is a pretty straightforward
improvement.

I would call this patch uncontroversial, because it doesn't change a lot of
code, and doesn't create future liabilities. In this case, the change could
have been submitted, with further touch ups in follow-up changes.

Another example is https://codereview.appspot.com/561350044/ . This changed
a return type in a couple of places. It went in, but I complained
afterwards. It was reverted and redone in a different way. This patch
doesn't take on future liabilities, so it doesn't need an extensive waiting
period. We can fix it up afterwards if something is wrong..

An example of a controversial patch: any kind of change to the input
syntax. The input format is a user accessible part of the system, so if you
expose a new feature, people will start using it and depending on it. If
such a change stays available for long enough, we can't change our mind and
back it out because we'd screw over our users. Syntax changes often also
impact existing scores, and friendly neighbor projects, like Frescobaldi.
Changes like this need thorough scrutiny.

>    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?
>

Weren't you complaining of patches that didn't get feedback? I had the
impression that part of the problem is that accepting patches used to
require a non-zero amount of work, and this caused contributions to
languish. Is that the case?

(maybe not for your patches, that were too complicated, but for other
patches?)


> >    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.
>

Yes, I completely agree, and we should improve the infrastructure here.

>    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?
>

The project maintainers, see below.

>    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?
>

People who

* agree to be committed with the project for the long term
* have sufficient knowledge to make technical decisions to further the
project goals
* have trust from the developer community.

In the distant past, that would be me & Jan. In the less distant past, that
is Graham and you. There are other people well-versed in subcomponents that
could be maintainers for particular aspects, eg. Dan for C++ specific
questions, or Jonas for Python related issues.

I call these people maintainers as opposed to contributors, who would come
and go more frequently.

>    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.
>

Yes, we would change the role of the patch meister. We could call it scrum
master or something else.

-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen