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.
I agree that it may be overkill for small, obviously correct patches, but if it's at least a bit more complex, I don't think we should abandon applying and testing the code.
Another thing - we should trust the author of patches to decide whether an ack is absolutely necessary. If you've sent a simple and obvious patch, waited a day and no-one has anything to say ... then IMHO, push it and ask for forgiveness later.
Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.
Cheers, Mark.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel