Last modified: 2012-12-19 14:38:24 UTC
I would like registered user to be able to CodeReview -2 (do not submit) changes submitted in Gerrit. That is a recurring complaint about people sending a patch only to get review on it. If people are able to mark they change as do not submit, that will save sometime to the reviewers that can just skip the red crossed changes. :-]
I was actually just thinking about this the other day. My use case was this: 1) Joe Developer submits a change 2) Jane User sees a really big problem, so sets it to -1 and says so 3) Jim Approver comes along, doesn't notice the problem, and +2/Submits the change 4) We've now got a broken commit that's been merged. I think the +1 permissions should remain as-is, but granting -2 to the same group seems reasonable to me...anyone should be able to say "Don't merge, because this is fundamentally broken." For cases where we'd really like to override a -2 (which I would hope would be few and far between), we can cross that bridge when we come to it. Maybe worth floating the idea on wikitech-l in case anyone has any objections, otherwise this is a < 2 minute change.
(In reply to comment #1) > I was actually just thinking about this the other day. My use case was this: > > 1) Joe Developer submits a change > 2) Jane User sees a really big problem, so sets it to -1 and says so > 3) Jim Approver comes along, doesn't notice the problem, and +2/Submits the > change > 4) We've now got a broken commit that's been merged. > > I think the +1 permissions should remain as-is, but granting -2 to the same > group seems reasonable to me...anyone should be able to say "Don't merge, > because this is fundamentally broken." For cases where we'd really like to > override a -2 (which I would hope would be few and far between), we can cross > that bridge when we come to it. > > Maybe worth floating the idea on wikitech-l in case anyone has any objections, > otherwise this is a < 2 minute change. I think the problem here is with Jim Approver, rather than with users being unable to set -2.
(In reply to comment #2) > I think the problem here is with Jim Approver, rather than with users being > unable to set -2. Agreed, Jim Approver shouldn't be in such a rush to approve things that he overlooks the comments. But Jane Commenter should be able to said "Hang on a minute..." and keep broken shit from getting merged, IMHO.
That happened to me a few time yesterday, I did review a change but did not spot some trivial mistakes. Was not going to kill the site though, but still. That might just happen :-]
Mail sent to wikitech-l : http://lists.wikimedia.org/pipermail/wikitech-l/2012-June/061275.html
I think there are two "-2"'s hidden here: 1. "Please treat this change as work in progress - comments welcome, do not merge" 2. "This is so broken that it must not be merged" For (1) I think gerrit "drafts" would be a better solution. I argue in bug 37115 that drafts should be visible to everyone. (2) raises some political problems (who can "veto" a change and for how long). Can I "veto" a change that breaks unit tests or kills support for my favourite non-MySQL database?
I think it would be only feasible if it was possible to override others' -2s.
(In reply to comment #6) > 1. "Please treat this change as work in progress - comments welcome, do not > merge" > > For (1) I think gerrit "drafts" would be a better solution. I argue in bug > 37115 that drafts should be visible to everyone. People can use (draft) just like (bug xx) in the commit message summary and irrespective of bug 37115 it can be achieved. People can remove (draft) when they want it change to be reviewed/merged.
(In reply to comment #7) > I think it would be only feasible if it was possible to override others' -2s. I can override a -2 review by removing the reviewer from the reviewer list (by clicking the x button next to their name). However, I'm both a project owner for MediaWiki core and a Gerrit administrator, so I don't know why I have this ability and who else has it.
(In reply to comment #9) > (In reply to comment #7) > > I think it would be only feasible if it was possible to override others' -2s. > I can override a -2 review by removing the reviewer from the reviewer list (by > clicking the x button next to their name). However, I'm both a project owner > for MediaWiki core and a Gerrit administrator, so I don't know why I have this > ability and who else has it. Only people who can do that are Admins, the change owner (I think), and the Project Owner. Ideally, this should be a delegatable right, but it's not right now :\
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #7) > > > I think it would be only feasible if it was possible to override others' -2s. > > I can override a -2 review by removing the reviewer from the reviewer list (by > > clicking the x button next to their name). However, I'm both a project owner > > for MediaWiki core and a Gerrit administrator, so I don't know why I have this > > ability and who else has it. > > Only people who can do that are Admins, the change owner (I think), and the > Project Owner. Ideally, this should be a delegatable right, but it's not right > now :\ This is now grantable https://gerrit-review.googlesource.com/#/c/37840/ :) Dunno if it'll make 2.5.
You are my hero !
We're not going to be granting -2 to everyone--that was a bad idea. WONTFIX.
