Re: Rietveld review

[David Kastrup]

Carl Sorensen <address@hidden> writes:

> On 5/3/10 1:02 AM, "David Kastrup" <address@hidden> wrote:
>
>> 
>> I don't see that you stand a chance with the standard processes here.
>> You don't have commit access.  The gold standard here (to the
>> exclusion of all other workflows) is a patch review on Rietveld.
>> That basically limits feedback to persons with a Google developer
>> account.
>
> I don't have a Google developer account.  I did need to use a gmail
> account.  So yes, if you want to comment, you need to have some kind
> of google account.  But I didn't find it difficult to establish a
> gmail account and have that account forward mail to my standard email
> address.

I do not have a gmail account, but a Google account was required.  A
separate mail account would be even more of a no-go for me.  I get
enough spam as it is.  Having to register and sign in in order to be
able to comment or contribute is a hurdle.

>> At the point of acceptance, the change is pulled in as a single
>> commit touching lots of areas simultanously.  Since mainline
>> development continues, there is no sensible strategy for
>> merging/rebasing for anybody without a branch history.  So the only
>> person able to work on this will be yourself.  People will be able to
>> check your work only when the patch set applies.  But that means that
>> mainline changes in the same areas need to be tracked by you and will
>> also pollute the Rietveld review area.
>
> Mainline changes don't need to pollute the Rietveld review area.  The
> post to Rietveld is in the form of a git diff, and you are free to
> specify any commit as the head point for the diff.

For an ongoing review process, you have the choice of picking the same
reference commit for every patch set, in which case the patch set will
at some point no longer apply to upstream development, or to track the
current upstream development (by rebasing or merging), in which case the
differences between patch sets will be polluted by the mainline changes.

> It's pretty simple to do the work in git, and merge/rebase with the
> main tree as necessary, and use git-cl to upload the diff of where
> your branch diverges from the main branch.  I haven't had problems
> with this is the past.  I'm not saying they don't exist (it's
> impossible to prove a negative), but in my experience the system works
> pretty well.

For short review periods and/or confined changes.  For that kind of
change which usually are done in a separate branch because they require
a lot of time between the conception phase and production readiness,
this will not work.  git provides extensive merge tracking and conflict
resolution strategies.  With a single humongous change, they become
inoperative.

>> I don't see that a task like this can be sensibly done except in a
>> separate branch.  But working like that is a git workflow rather than
>> a Subversion one, and does not fit with the Rietveld processes.  Of
>> course you can set up your own Lilypond repository for pulling, but
>> the way I see it, the relevant developers would refuse to participate
>> in "non-standard" processes like that.
>
> In the past we have had developers with an individual branch hosted on
> Savannah that were not part of the main tree; we didn't find them
> useful.  Why do you need to "set up your own Lilypond repository for
> pulling"?  Why not just create a branch in your local repository and
> go ahead and do the rebase/merge as necessary?

In that case a single person will be doing all the development work.
Which is just what I told Boris to expect.

>> I don't see that humongous changes like that can be usefully
>> integrated in a single non-merge commit.
>
> I don't understand this emphasis on "a single non-merge commit".  If
> you want to propose a patch set that requires multiple commits, you
> can do so.  It won't show up on Rietveld that way, but you can email a
> patch as a series of commits leading of the main trunk.

Certainly.  That's part of git workflows, for small to medium sized
changes.  For large changes, the version graph becomes an important part
for conflict resolution, and mailed patches don't contain that.  In that
case, separate branches make more sense.

I've mailed non-trivial patch sets in the past, and have been told by
developers that they won't look at them since the acceptable workflow
was to put stuff up on Rietveld.  Search the mailing list if you are in
the mood of reading me throw another tantrum.

>> There must be infrastructure changes and so on, and you'll need to
>> set up reviews for each of them, without an apparent goal being
>> accomplished before the last commits make it.
>
> I agree that this can be an issue.  Infrastructure changes that are
> not needed are asked to be postponed until the need is apparent.  But
> they're not rejected.

They'll need to be updated with the mainline development.  Keeping four
reviews updated instead of one branch is going to be a major pain.
Either you need to develop four separate branches which won't run until
merging them, or the reviews will not be independent.

>> Basically you'll be on your own going against the tide until you are
>> finished.  The work flows here are designed to achieve code quality
>> by making it harder for a would-be contributor to get sub-par code
>> through, not by making others participate with the work.
>
> I think this is an interesting comment.  Do you believe that it would
> be preferable to let sub-par code through?  Or do you believe that
> there are other workflows that would be as effective at blocking
> sub-par code but would be more inviting to a new contributor?

Accepting sub-par code can't be a goal.  New contributors of code will
tend to produce confined changes with a limited review time for which
the current processes are not all too bad.  What I am concerned with are
several hurdles at different levels.

Contribution     hurdle

comments         Google account
testing          review has to be kept uptodate respective current development
minor changes    Google account
major changes    review has to be kept uptodate respective current development
                 maintaining more than one review impractical
                 active development strictly confined to one person

> This is a serious question I'd like to ask you.  If you were the king
> of LilyPond, what would you establish as the workflow?  I'd really
> like to hear your opinion.

I'd not prohibit any work flow that can be maintained with single-line
git commands.  That includes posting patch series (using
git-format-patch and git-mail) and private and public branches.  There
are graphical tools for dealing with git branches and commits, there is
Emacs.  The established development environment for Lilypond already is
a GNU/Linux machine (even if it is a virtual one), so the usual caveats
about git workflows on Windows machines don't apply.

There are tools for visualizing git branches/patch sets into HTML.
Those can be used for review purposes by people without the desire to
pull, or the desire to maintain their own development tree.

<URL:https://git.wiki.kernel.org/index.php/InterfacesFrontendsAndTools#Web_Interface>
might give some suggestions about how to provide graphical access.

There will be no signin-free solution for offering HTML reviews: someone
has to host the stuff, after all.  If that turns out to be an
impediment, a review master might pull patch series from mails off into
temporary branches on a review server.  Centralized HTML reviews come at
a price, and the local tools available with every git installations work
pretty well, anyway, as long as you _have_ a local repository.  And
parallel development in contrast to limited-time patch series needs
maintaining branches (including the commit DAG) in some manner, anyway.

The Linux kernel development is a good example of what can be done
employing the full power of git (without much reliance on HTML and
similar).  Of course, Torvalds itself and several other core developers
are employed full-time on the project, but it does show that things like
"realtime kernels" can be kept reasonably in synch with the mainline for
years, with stabilized parts of them being moved into the mainline once
they reach the required quality.

What Boris described here _does_ sound to me like it was meddling with a
non-trivial set of Lilypond internals, and so this would not be totally
dissimilar.

Would Lilypond benefit from changing the infrastructures in that way?
Depends on how often a situation like that of Boris occurs, and how
desirable it is to deal with it in a manner that does not impede a
would-be contributor more than necessary, and makes it reasonably easy
for others to help not just with advice, but also with patch suggestions
of their own.

In the time it takes to write: "I'd encapsulate this and that
functionality in a function called so-and-so", you can do it yourself.
But if there is no no-brain way to pass the resulting patch back to a
review, the contributor still needs to do the work himself.

I am not king of Lilypond, and many people will be glad about that.  I
am just telling Boris what to expect for the kind of work he is thinking
about, in my opinion and experience.

If I am wrong, so much the better.

-- 
David Kastrup