Hi,
TL;DR: Gerrit would allow to keep Code-Review votes across * rebases and * commit message modifications of patch sets.
Thereby dropping need to re-review “trivial” changes on patch sets.
Shall we turn that feature on?
-------------------------------------------- Longer version.
Currently, upon uploading a new patch set for a change in gerrit, votes get scrubbed. Especially, all Code-Review votes except -2 are gone.
However, for new patch sets that * are a plain rebase, or * only change non-code parts (commit message, ...) gerrit would allow to reapply votes of the previous patch set to the new patch set.
People asked me to turn this feature on gerrit-wide for the Code-Review label.
But I would not want to turn it on without giving people a chance to discuss it beforehand.
So ... example: Assume for a given change the current patch set's votes are:
+-------------+-------------+----------+ | Reviewer | Code-Review | Verified | +-------------+-------------+----------+ | Foo | +2 | | | Bar | | +1 | | Baz | +1 | -1 | | Qux | -2 | | | jenkins-bot | | +2 | +-------------+-------------+----------+
Assuming we turn the feature on in gerrit, and I upload a plain rebase of the current patch set, the votes for the plain rebase would be
+-------------+-------------+----------+ | Reviewer | Code-Review | Verified | +-------------+-------------+----------+ | Foo | +2 | | | Bar | | | | Baz | +1 | | | Qux | -2 | | | jenkins-bot | | | +-------------+-------------+----------+
right after uploading the rebase to gerrit (instead of the current behaviour of an empty table with only Qux CR-2). Same if I only edit the patch set's commit message.
But if I upload a patch set that is changing the patch set's diff, the table of votes would of course still get scrubbed of all votes except -2 on Code-Review (as it also the case right now):
+-------------+-------------+----------+ | Reviewer | Code-Review | Verified | +-------------+-------------+----------+ | Foo | | | | Bar | | | | Baz | | | | Qux | -2 | | | jenkins-bot | | | +-------------+-------------+----------+
Should we turn that feature on?
Best regards, Christian
On Friday, April 11, 2014, Christian Aistleitner christian@quelltextlich.at wrote:
Hi,
TL;DR: Gerrit would allow to keep Code-Review votes across
- rebases and
- commit message modifications
of patch sets.
Thereby dropping need to re-review “trivial” changes on patch sets.
Shall we turn that feature on?
Yes. This'd be great.
J.
What if someone -1's due to something in the summary? It's odd that fixing it with a new commit would still show -1 on the reviewer's dashboard. I'm fine with it for automatic rebases though.
-- View this message in context: http://wikimedia.7.x6.nabble.com/Keeping-Code-Review-votes-across-trivial-ch... Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Amending a commit message really shouldn't remove existing votes. I personally would like this on the MobileFrontend repository. I'm not sure if this can be done on a per project basis if necessary but I want this.
On Fri, Apr 11, 2014 at 9:29 AM, Aaron Schulz aschulz4587@gmail.com wrote:
What if someone -1's due to something in the summary? It's odd that fixing it with a new commit would still show -1 on the reviewer's dashboard. I'm fine with it for automatic rebases though.
-- View this message in context: http://wikimedia.7.x6.nabble.com/Keeping-Code-Review-votes-across-trivial-ch... Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hi,
On Fri, Apr 11, 2014 at 09:35:15AM -0700, Jon Robson wrote:
I personally would like this on the MobileFrontend repository. I'm not sure if this can be done on a per project basis if necessary but I want this.
Since it's basically “yes”-votes up to now, I hope there's no need to, but if necessary, we can limit it to projects.
Have fun, Christian
On 11 April 2014 09:29, Aaron Schulz aschulz4587@gmail.com wrote:
What if someone -1's due to something in the summary? It's odd that fixing it with a new commit would still show -1 on the reviewer's dashboard.
Sure, but those are relatively rare for simple changes (normally you just fix it yourself, with a CR explaining why); if it's the edge case of the -1 being "I don't understand what the reasoning is for this change in the commit summary", then I think it's appropriate for the -1 to remain until the original reviewer can comment.
J.
On Fri, 11 Apr 2014 18:29:06 +0200, Aaron Schulz aschulz4587@gmail.com wrote:
What if someone -1's due to something in the summary? It's odd that fixing it with a new commit would still show -1 on the reviewer's dashboard. I'm fine with it for automatic rebases though.
I think cases of the -1 being for the commit summary being inaccurate or too brief are way less common than cases of fixing typos in the summary accidentally removing votes (and I'm one of the few people who would -1 for such summaries :) ).
+1 from me.
Op 11 apr. 2014 om 15:57 heeft Christian Aistleitner christian@quelltextlich.at het volgende geschreven:
Hi,
TL;DR: Gerrit would allow to keep Code-Review votes across
- rebases and
- commit message modifications
of patch sets.
Thereby dropping need to re-review “trivial” changes on patch sets.
Shall we turn that feature on?
+1
-- Siebrand Mazeland Kitano ICT
M: +31 6 50 69 1239 Skype: siebrand
+1
I never liked this, over and over I point out a fatal issue with a commit sometimes fundamental to the idea itself and impossible to fix, so I -1 it, then when a new patchset comes out, completely ignores the concerns I've pointed out, my -1 with the idea suddenly disappears as if the changeset was supposed to fix it. When I was drafting Gareth ideas I explicitly avoided that issue in my "plins". First allowing users to apply two types of -1/+1 one that went away each commit and another intended for fundamental objections to the idea which never went away. And second by making it so that the -1/+1s that go away don't actually go away, they would still show up in the list, semi-transparent and greyed out, to indicate that someone -1ed a prior version but they haven't reviewed the current commit and it may not apply any longer.
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]
On 2014-04-11, 6:57 AM, Christian Aistleitner wrote:
Hi,
TL;DR: Gerrit would allow to keep Code-Review votes across
- rebases and
- commit message modifications
of patch sets.
Thereby dropping need to re-review “trivial” changes on patch sets.
Shall we turn that feature on?
On Fri, Apr 11, 2014 at 4:08 PM, Daniel Friesen daniel@nadir-seen-fire.com wrote:
I never liked this, over and over I point out a fatal issue with a commit sometimes fundamental to the idea itself and impossible to fix, so I -1 it, then when a new patchset comes out, completely ignores the concerns I've pointed out, my -1 with the idea suddenly disappears as if the changeset was supposed to fix it.
That's what -2 is for. I think this is a social issue: we need to stop thinking of -2'ing a patch as a grave statement of disapproval. It should have a meaning matching its function: that is, a "sticky" -1 that isn't cleared automatically by a new revision. --scott
On Mon, 14 Apr 2014 18:02:43 +0200, C. Scott Ananian cananian@wikimedia.org wrote:
I think this is a social issue: we need to stop thinking of -2'ing a patch as a grave statement of disapproval.
Nope (or not only), most people don't even have the ability to give out a -2 (it currently comes "in a package" with +2 rights).
On Mon, Apr 14, 2014 at 9:07 AM, Bartosz Dziewoński matma.rex@gmail.comwrote:
On Mon, 14 Apr 2014 18:02:43 +0200, C. Scott Ananian < cananian@wikimedia.org> wrote:
I think this is a social issue: we need to
stop thinking of -2'ing a patch as a grave statement of disapproval.
Nope (or not only), most people don't even have the ability to give out a -2 (it currently comes "in a package" with +2 rights).
That's easily adjustable if people so desire.
-Chad
On Friday, April 11, 2014, Christian Aistleitner christian@quelltextlich.at wrote:
Hi,
TL;DR: Gerrit would allow to keep Code-Review votes across
- rebases and
- commit message modifications
of patch sets.
Thereby dropping need to re-review "trivial" changes on patch sets.
Shall we turn that feature on?
Yes please.
Longer version.
Currently, upon uploading a new patch set for a change in gerrit, votes get scrubbed. Especially, all Code-Review votes except -2 are gone.
However, for new patch sets that
- are a plain rebase, or
- only change non-code parts (commit message, ...)
gerrit would allow to reapply votes of the previous patch set to the new patch set.
People asked me to turn this feature on gerrit-wide for the Code-Review label.
But I would not want to turn it on without giving people a chance to discuss it beforehand.
So ... example: Assume for a given change the current patch set's votes are:
+-------------+-------------+----------+ | Reviewer | Code-Review | Verified | +-------------+-------------+----------+ | Foo | +2 | | | Bar | | +1 | | Baz | +1 | -1 | | Qux | -2 | | | jenkins-bot | | +2 | +-------------+-------------+----------+
Assuming we turn the feature on in gerrit, and I upload a plain rebase of the current patch set, the votes for the plain rebase would be
+-------------+-------------+----------+ | Reviewer | Code-Review | Verified | +-------------+-------------+----------+ | Foo | +2 | | | Bar | | | | Baz | +1 | | | Qux | -2 | | | jenkins-bot | | | +-------------+-------------+----------+
right after uploading the rebase to gerrit (instead of the current behaviour of an empty table with only Qux CR-2). Same if I only edit the patch set's commit message.
But if I upload a patch set that is changing the patch set's diff, the table of votes would of course still get scrubbed of all votes except -2 on Code-Review (as it also the case right now):
+-------------+-------------+----------+ | Reviewer | Code-Review | Verified | +-------------+-------------+----------+ | Foo | | | | Bar | | | | Baz | | | | Qux | -2 | | | jenkins-bot | | | +-------------+-------------+----------+
Should we turn that feature on?
Best regards, Christian
-- ---- quelltextlich e.U. ---- \ ---- Christian Aistleitner ---- Companies' registry: 360296y in Linz Christian Aistleitner Gruendbergstrasze 65a Email: christian@quelltextlich.atjavascript:; 4040 Linz, Austria Phone: +43 732 / 26 95 63 Fax: +43 732 / 26 95 63 Homepage: http://quelltextlich.at/
Le 11/04/2014 15:57, Christian Aistleitner a écrit :
TL;DR: Gerrit would allow to keep Code-Review votes across
- rebases and
- commit message modifications
of patch sets.
Thereby dropping need to re-review “trivial” changes on patch sets.
Shall we turn that feature on?
<snip>
Thank you Christian to raise this publicly :-]
Gerrit knows about two different trivial changes:
- trivial rebase: the new patchset is the same diff but the parent has changed. This is often fine but the new patch my not be working anymore. Though if that happens tests will catch it.
- no code change: only the commit summary has changed. Ie the proposed code is exactly the same and should have the same behavior.
Details: https://gerrit.wikimedia.org/r/Documentation/config-labels.html#label_copyAl...
I am fine reapplying scores for both types.
On 14 April 2014 12:06, Antoine Musso hashar+wmf@free.fr wrote:
Le 11/04/2014 15:57, Christian Aistleitner a écrit :
TL;DR: Gerrit would allow to keep Code-Review votes across
- rebases and
- commit message modifications
of patch sets.
Thereby dropping need to re-review “trivial” changes on patch sets.
Shall we turn that feature on?
<snip>
Thank you Christian to raise this publicly :-]
Gerrit knows about two different trivial changes:
- trivial rebase: the new patchset is the same diff but the parent has
changed. This is often fine but the new patch my not be working anymore. Though if that happens tests will catch it.
- no code change: only the commit summary has changed. Ie the proposed
code is exactly the same and should have the same behavior.
Details:
https://gerrit.wikimedia.org/r/Documentation/config-labels.html#label_copyAl...
I am fine reapplying scores for both types.
It looks to me like we have rough consensus for doing this. Make it so?
J.
Hi,
On Mon, Apr 14, 2014 at 01:25:09PM -0700, James Forrester wrote:
It looks to me like we have rough consensus for doing this. Make it so?
Done.
Thanks to hashar for providing the patch set!
Have fun, Christian
wikitech-l@lists.wikimedia.org