On Tue, Jul 17, 2012 at 12:17 PM, Peter Stuge <[email protected]> wrote: > Freddie Chopin wrote: >> It's not a hard rule that a negative review will ALWAYS be discarded >> after ANY explanation and 2 weeks. > > I think you realize that noone will ever bother to give negative > review because the proposed policy makes it OK to sidestep any > negative review if it considered to be inconvenient. > > Not the purpose of review.
Speak for yourself, I would certainly not consider avoiding giving negative review because of a rule like this. >> It's just for situations that someone gives -2 and looses interest >> in project a while after, > > I think this is naïve, or malicious. If someone goes through the > trouble to do actual review it is highly unlikely that they will > ever lose interest in that commit. They have, after all, loaded > the code into their head already, so they are in a significantly > better position than those who have not done any review, to discuss > how the change evolves. But as you say below, most of us do this voluntarily and we may need to prioritize other things than OpenOCD for extended periods of time without prior notice. It's quite natural that ongoing reviews will be left unattained by the original reviewer from time to time. That's why we are more than one maintainer, so that anyone can pick up the task of reviewing and submitting. Yes, it requires duplicating the review effort already made by the original reviewer. But that's the purpose of review; to get as many eyes on the code as possible. >> while the change has been improved (sometimes several times) - what >> shall we do in that case - force this person to re-examine the >> change? > > Start by trying to communicate, while at the same time understanding > that most people contribute to open source projects voluntarily, in > their spare time. This applies not only to maintainers, but contributors as well. I think it's rude of us to leave a contribution which is updated after initial feedback unattained for several weeks. It does not encourage further contributions. And we live on those. >> This is a perfect example - http://openocd.zylin.com/#/c/467/ >> You gave "-2", author of the change answered and improved the patch >> week after, and sice then (3 months) the change is blocked by your "-2". > > Can you imagine that I paged out that commit and thus didn't notice > the updated patch set. Someone sent a review request for the new > patch set (communicated) and so it appeared on my radar again. Good. > I spent some time on it today already, but I am busy doing three-four > other things, so it will probably stay open in my browser for some > hours still. Yeah, and so any other maintainer should be able to take your feedback into account, review and possibly approve that change, regardless of your -2 which is for an older patch set. If you have simply forgotten the patch you will get an email reminder upon the approval and a chance to discuss the new patch and perhaps air your remaining concerns before it's submitted. I think the problem lies partly in that a -2 stays across an updated patch while a -1 and positive review is reset. If all review was reset it would force "do not submitters" to reexamine the change or rely on another maintainer to make the judgment. Doesn't sound too bad. If it works as today, I would like to reserve -2 for patches that won't be submitted in any form, because they are fundamentally wrong. -1 should be used for patches that are in principle a good idea but have issues and can be merged after fixing them. A separate part of the problem is that a -2 "vote" is not a vote but a veto. I don't think any one of us maintainers should have vetoes. If maintainers disagree enough to differ 4 "voting" steps (has never happened so far but who knows what the future will bring) it should be a democratic process to decide whether to submit or not. Negative feedback is noted in the commit as a reference for people who want to complain later. >> SWD change is going to be drastic compared to MPSSE > > It doesn't have to be. It all depends on how the OpenOCD changes > for supporting LibSWD are done. If they're done well then it is > easy to introduce them into the tree. Believe me, any SWD support that I will approve of will be magnitudes more invasive than the MPSSE patch. >> which took 4 months to merge... Hrm, it did live quite a while on the mailing list before ending up in gerrit as well. I think I wrote the bulk of it the very first weeks of the year. > What happened during that process exactly? If you haven't already > reviewed all patch sets and read all comments then I urge you to do > so before complaining about how it took too long to "merge." The ftdi driver rewrite took waaay too long to get submitted, but of course I'm a bit biased since the are my own patches. It's easy to diff the different patch sets in gerrit. If you do (and match with the comments), you'll see that most of the changes are fixes for minor style issues, which by them self didn't warrant a -2 IMO. You do have provided significant feedback regarding the libusb handling and found a few bugs there also. But with everything taken into account, I don't think it's reasonable for a non-invasive patch like this to be on hold for almost half a year when it could have been submitted more or less from day 1 without ill effects, with issues fixed afterwards. Sometimes real-life testing of imperfect code is worth more than review, as long as it doesn't break anything that worked before. Now the reason for the long MPSSE delay was not your or anyone else's negative feedback, but the lack of feedback at all! All feedback was much appreciated. I'll only speak for myself, but I'm a lot more inclined to produce follow-up work once the initial patches are merged. Until they are, every spin-off idea I get that bases on the change is mentally put on hold. I envisioned a bunch of rather invasive architectural changes, of which the ftdi rewrite would be a vehicle for my own testing. Seeing that the community lacks the energy to review (architecturally and not style-wise) significant patches (and contributing in pre-patch discussions), I'm kind of put off by the work required. > OpenOCD is a much smaller project, but there's no reason not to use > the same principle of review [as linux], the purpose of which is (again) to > have maintainable and releaseable code in the repo. Sometimes it > takes time to arrive at that, especially for significant > contributions like the mpsse layer and ftdi driver. I think it's pointless to draw any conclusions from what works for a project with the momentum that linux has, and apply them to a project like this. > If you want to help make something happen then you are most welcome > to do so. I think it is very cool that anyone can push a new patch > set to an open change! I think I suggested that you do so, but you > declined. Or I beat him to it... The last change was minor and he could probably have done it. (Still you changed your feedback 4 steps on account of the change, maybe in hindsight it could have been changed to -1 a lot earlier?) /Andreas ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ OpenOCD-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openocd-devel
