On Thu, Aug 04, 2011 at 12:16:30PM -0700, Steven Dake wrote:
On 08/04/2011 09:26 AM, Tomas Sedovic wrote:
On 08/04/2011 04:53 PM, Mark McLoughlin wrote:
Hi Simon,
On Fri, 2011-08-05 at 00:24 +1000, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process,
Yes, there is. A tradition of opening up every commit to review and discussion on the mailing list.
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code.
Note that even those who have commit rights must get an ack before pushing.
What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people,
I could be wrong, but I think that actually is our model. If it's not, then I agree with you.
or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
I think that having patches go across the mailing list before committing is still a fine idea.
But I do agree that we should have a low bar for giving people push rights. The patch review process and the ability to revert give us enough of a safety net. We should trust and empower new contributors.
I also agree folks should be a lot more proactive about acking. What might be part of the problem here is that we seem to require patches be applied and tested before being acked on this project. I don't see why people shouldn't ack patches after just reading them.
Note that some of us aren't hugely experienced and thinks do slip through out attention.
An important part of the review process is actually testing the functionality and making sure that nothing breaks. I think that's a good thing to have.
Disagree.
If committers end up testing every review of every patch, committers become a single point of integration and become overloaded (and ultimately disinterested). An experienced maintainer should learn who to trust and apply their patches if they look sanitary.
To address your concerns of a regression introduced by a patch, projects typically implement a trailing commit regression testing system to identify specific patches that introduced a regression. If a regression was uncaught by automated testing requiring a developer to bisect the root cause of a regressoin, the patch offender should be held responsible for adding a regression test case.
Regards -steve
Hmm this is a very interesting discussion, glad we're having it.
I am at least partly responsible for the "test before ack" policy on Aeolus. It is a throwback to oVirt, when we suffered many, many days of delay trying to integrate patches that failed not because the submitter hadn't tested them, but because they hadn't tested them on a clean install or their dev box was non-standard in other ways that masked failures. At this point the team has become completely accustomed to the routine of "apply this patch and smoke-test it before acking it."
We also give pretty much everyone on the project commit, once they've proven they understand what "tested" really means.
If the assembled multitude here agrees that test-before-ack is too onerous, given that we do have a reasonably good automated test framework in place, then I wouldn't object to a change. But it has served us fairly well for a long while, so don't consider dropping it lightly.
Take care, --Hugh