On 08/04/2011 04:24 PM, 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, 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.
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. 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, 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.
OK, go! :)
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
I do agree that we should give commit access to wider audience. It's not the case now (or at least it wasn't a few months back) and I think we should change that.
However, I do personally like the code -> patch -> review -> commit process.
This may stem from me being very young and inexperienced, but I have found incredible value in others' feedback to my patches. And thanks to me not pushing everything I've sent to the list, the git log looks much saner.
In turn, I have provided reviews to others' code that they found very helpful (or at least said so).
Doing this over the mailing list may be arcane and I'm all for better tooling (Mark noted there is one coming soon). But I still prefer reviews before the code is pushed to the master repo.
If only because it does incentivise people to do the reviews and makes the history look cleaner.
As for the "you ack mine, I'll ack yours" situation: anecdotally, I've never seen the pathological version of this happen here. It may become an issue as we grow but I haven't seen any evidence that it is one now.
Yes, I do trade reviews with the guys here, but as far as I can tell, they are no less diligent when they know I hold their patch in my hands. I don't think I'm softer either, though that may be a bias. Please point it out to me if that is the case.
thomas|shadow