W dniu 2012-07-17 16:14, Andreas Fritiofson pisze: > Speak for yourself, I would certainly not consider avoiding giving > negative review because of a rule like this.
"Rule" is a wrong word here - I'm not a native speaker in English, so maybe I have problems expressing what exactly I meant by this patch - it's really just a "last resort" solution when the person who gave negative review cannot be reached and does not re-review the updated change. It should not be seen as a "hard rule" which gets executed with sub-second precision after the timeout passed... > 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. Good point! First of all it's not that a "-2" will be completely ignored - another reviewer sees the comments so he/she (any "she" here? (; ) will pay extra attention to these concerns. It's also not a rule which makes the changes submitted immediately after the timeout period is over - it's just a guideline/info. > 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. +1 (or even +2 (; ) from me! "-2" seems very harsh and the fact that it's not reset when new change version arrives makes it something like you say - this idea (whatever form it takes) will never be submitted. > 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. Again +1 from me - I veto any vetos! If 5 maintainers agree and 1 does not I think that the change should be merged... > 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. +1 - this patch did not break anything, merging it would bring following benefits: 1. it could be tested by 100x more people, for example people who download binaries from my site, not only by 10-20 ppl who follow list and gerrit. 2. Any minor improvements could be carried by anyone (I feel it's a bit wrong to take someone's patch and change it yourself...) later 3. Original author would feel happy that his/hers (?) change would be merged, thus more inclined to do more. (Andreas writes about that too). The only thing MPSSE patch changed in rest of the code is use of libusb-1.0 for stlink and jlink (probably) - not a big problem imho. > 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. I don't care much about the style, as long as it's not a complete mess (; > 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. Agreed - Linux is not a software for very specific purpose as OpenOCD. > 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?) First of all I'm not a PC programmer. I also don't think it's the right thing to do to change someones patches - in "major" things, "minor" stuff would probably be OK (like order or patches, some typos, marking parts of code as tested when they were marked as experimental, etc.). 4\/3!! ------------------------------------------------------------------------------ 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
