> 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?

Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.

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.

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(a)lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel

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

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(a)lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel