RFC: xserver commit process changes

[Peter Hutterer]

On Thu, Feb 28, 2013 at 01:58:15AM -0800, Keith Packard wrote:
> Peter Hutterer <peter.hutterer at who-t.net> writes:
> 
> > This has worked reasonably well since we started for server 1.8. If 
> > nothing else, it has made git master a lot more reliable. However, Keith 
> > is the bottleneck to get anything into master. The delay to get a pull 
> > request merged has been quite random, depending on Keith's other 
> > workload. I'm struggling with this a lot at times. Bugs cannot be closed 
> > even though the work is done, local branches cannot be merged and 
> > finalised. Apparently others face similar issues.
> 
> Yeah-- my delay is somewhere between an hour and a week, depending on
> other things going on. And, the process for merging patches is pretty
> formalized now that we've done this a lot. It's not entirely mechanical
> though, the key is to have some idea whether a patch has 'sufficient'
> 'credible' review, and whether there are any outstanding questions about
> it.

Unfortunately, the delay is on top of the review delay. There's been at
least two cases in the last cycle where a patch was lingering for weeks,
unreviewed, and only got attention when I sent a pull request for unreviewed
patches.

e.g. "dix: don't allow disabling XTest devices", sent to the list on 11 Oct,
sent as part of the pull request on Oct 25. That did get a review within a
day then. (and I only picked this example because it's the first one that
popped up in my mailbox)

> > * leave the current window of 3/2/1-ish months for the different devel 
> > stages
> > * leave the requirement for a reviewed-by
> > * one RM, calling the shots for when releases are made and generally 
> > being the reviewer of last resort and arbiter where needed
> 
> > * 3-5 people with commit access during the devel and general bugfix 
> > windows. They scoop up pull requests and commit them, if the patches 
> > have rev-by tags [3]
> 
> It's a more subtle process than you might imagine; I really do read
> every comment made during review and try to get a sense of whether the
> code being merged is really ready.
> 
> This is really trying to do two things:
> 
>  1) Let people learn how to review code, both by watching others and by
>     trying their own hand at it. When someone starts reviewing code that
>     they haven't seen before, the first several review messages often
>     catch C semantic and/or syntactic issues, but it isn't until the
>     reviewer has seen a bunch of the code that they start to catch more
>     subtle issues.
> 
>     Yes, I really do read all of the review comments that float past in
>     email, and keep track of both who writes and who reads all of the code.
> 
>  2) Ensure that discussion about changes has come to rough consensus
>     before changes are merged to master. Often times a patch will
>     appear, seem reasonable to most people and get reviewed rapidly only
>     to have someone pop up with valuable questions.

Both of these are welcome and nothing changes in that regard.
I don't think that it needs to all fall on you though, there are others that
can do the same as well and we should share the review burden much more.

> > * 2 people during the last bugfix window (emergency fixes only). The 
> > second person as backup to the RM to make sure we don't see delays.
> 
> I'd say the tree should probably get locked to the RM for the last week
> or so just to make sure things don't change unexpectedly.

Yeah, that seems like a sensible plan. However, having a second person as a
backup is a useful thing to avoid the bus factor.

> > Volunteers for committers are welcome. Though note that this is not to 
> > get your own patches in quickly, you'll also have to scoop up and review 
> > other patches. (fwiw, I volunteer)
> 
> In fact, as a general rule committers to master shouldn't be pushing
> major chunks of their own code to ensure that at least two people are
> thinking closely about the development process. This will help me a
> bunch as I've often felt a bit uncomfortable merging my own development
> work into the tree, even with reviewed-by lines attached.

Agreed, there have been plenty of times where I wasn't sure what I was
cooking up. However, IMO there is still a need for some patches to go in
faster than others.

Cheers,
   Peter