Sticky +2 on reviews?

[Simon Glass]

Hi,

In google3 when you get an LGTM it is sticky, meaning that you can upload a new commit and keep the LGTM, so long as yo don't add/remove any files.

How about we enable this in Chrome OS? Perhaps we could restrict it to just to commits by full committers if people are worried about it?

Reasons:
1. The follow-up LGTM does not add a lot value in the vast majority of cases. It is more of a rubber stamp, and one that costs time and unnecessarily delays CLs going in.

2. Many times people add their own +2 (carrying it forward), but this means that the actual reviewers are lost and do not appear in the commit

3. People already see a lot of email with test patch revisions, test failures and the like, and this would lighten the load a little

Regards,
Simon

[Aaron Durbin]

On Mon, May 21, 2018 at 2:11 PM, Simon Glass <s...@chromium.org> wrote:
> Hi,
>
> In google3 when you get an LGTM it is sticky, meaning that you can upload a
> new commit and keep the LGTM, so long as yo don't add/remove any files.

Edits to existing files between patch versions may change logic. Why
would you want the +2 be sticky in that situation?

>
> How about we enable this in Chrome OS? Perhaps we could restrict it to just
> to commits by full committers if people are worried about it?
>
> Reasons:
> 1. The follow-up LGTM does not add a lot value in the vast majority of
> cases. It is more of a rubber stamp, and one that costs time and
> unnecessarily delays CLs going in.
>
> 2. Many times people add their own +2 (carrying it forward), but this means
> that the actual reviewers are lost and do not appear in the commit
>
> 3. People already see a lot of email with test patch revisions, test
> failures and the like, and this would lighten the load a little
>
> Regards,
> Simon
>

> --
> --
> Chromium OS Developers mailing list: chromiu...@chromium.org
> View archives, change email options, or unsubscribe:
> https://groups.google.com/a/chromium.org/group/chromium-os-dev

[Brian Norris]

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

At least for the latter problem, your #2 becomes a feature, not a bug :D

Also, if you were to restrict this to just full committers, then what's the difference from today, with people being forced to put their own name on the "carried" +2?

--

[Simon Glass]

On 21 May 2018 at 14:17, Aaron Durbin <adu...@chromium.org> wrote:

On Mon, May 21, 2018 at 2:11 PM, Simon Glass <s...@chromium.org> wrote:
> Hi,
>
> In google3 when you get an LGTM it is sticky, meaning that you can upload a
> new commit and keep the LGTM, so long as yo don't add/remove any files.

Edits to existing files between patch versions may change logic. Why
would you want the +2 be sticky in that situation?

I assume a certain level of trust that people don't make significant changes (they are mostly +2 reviewers themselves, after all), and they have the option of requesting PTAL.

[Simon Glass]

On 21 May 2018 at 14:20, Brian Norris <brian...@chromium.org> wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

At least for the latter problem, your #2 becomes a feature, not a bug :D

Yes, but I suppose people can always +2 their own patch, so perhaps we should rethink that at the same time? 

Also, if you were to restrict this to just full committers, then what's the difference from today, with people being forced to put their own name on the "carried" +2?

Actually I would like to keep the existing +2 when uploading a new patch, not remove it. This is how google3 works.

- Simon 

[Brian Norris]

On Mon, May 21, 2018 at 1:22 PM, Simon Glass <s...@chromium.org> wrote:

On 21 May 2018 at 14:20, Brian Norris <brian...@chromium.org> wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

At least for the latter problem, your #2 becomes a feature, not a bug :D

Yes, but I suppose people can always +2 their own patch, so perhaps we should rethink that at the same time? 

Is that to say you can't self+2 in other systems (e.g., google3)?

 

Also, if you were to restrict this to just full committers, then what's the difference from today, with people being forced to put their own name on the "carried" +2?

Actually I would like to keep the existing +2 when uploading a new patch, not remove it. This is how google3 works.

I understand what you're proposing, but I'm just saying that your proposal (if restricted to full committers) can be implemented today with a self+2. And my claim was that if it's necessary, maybe that's a good thing -- I personally wouldn't want my +2 put onto the aforementioned CL (which I had to NAK).

If such a solution were in place, I'd have to be even more aggressive at paying attention to reuploads, to avoid rubber stamping changed (and broken) logic. Today, I know that the most harm someone can do is change the commit message or perform no-op rebases (I think?).

Anyway, that's a single instance. There probably were significantly more where a carried +2 would do just fine.

Brian

[Joel Hockey]

+1 for sticky +2's.

It would be a big help for teams like ours in SYD where review cycles to other timezones are often more than 24hrs.

This level of trust has worked well in google3 and chromium.

[Daniel Erat]

If someone leaves a +2 with minor comments on your change (and there are no other reviewers on the change whom you're still waiting on), I think it's okay to assume that the reviewer trusts that you'll address the comments in a reasonable manner before committing. It's fine to fix nits and self-+2 after re-uploading, in my opinion. If you end up making non-trivial changes, please reply with a "PTAL" so the reviewer can double-check the new code.

If the problem is that people aren't committers, well... we can always use more people on the tree-guardian rotations, and it's nice for each team to contain at least a few people who are committers so they're not wholly dependent on outside teams for all of their code reviews. :-)

Re code reviews across time zones, you have my sympathy -- it's frustrating to have long iteration times on reviews. I hope that reviewers keep this in mind and try their best to avoid blocking code authors unnecessarily. For example:

- Try to send comments (especially high-level design comments) during overlapping work hours so the author can ask clarifying questions.

- Instead of just saying, "don't do it like this", offer suggestions so the author doesn't need to guess (possibly incorrectly).

etc.

As a code author, I've found that sending smaller changes (possibly in parallel) can also encourage reviewers to send comments more quickly.

[Simon Glass]

Hi Brian,

On 21 May 2018 at 14:32, Brian Norris <brian...@chromium.org> wrote:

On Mon, May 21, 2018 at 1:22 PM, Simon Glass <s...@chromium.org> wrote:

On 21 May 2018 at 14:20, Brian Norris <brian...@chromium.org> wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

At least for the latter problem, your #2 becomes a feature, not a bug :D

Yes, but I suppose people can always +2 their own patch, so perhaps we should rethink that at the same time? 

Is that to say you can't self+2 in other systems (e.g., google3)?

Yes.

 

 

Also, if you were to restrict this to just full committers, then what's the difference from today, with people being forced to put their own name on the "carried" +2?

Actually I would like to keep the existing +2 when uploading a new patch, not remove it. This is how google3 works.

I understand what you're proposing, but I'm just saying that your proposal (if restricted to full committers) can be implemented today with a self+2. And my claim was that if it's necessary, maybe that's a good thing -- I personally wouldn't want my +2 put onto the aforementioned CL (which I had to NAK).

Are you sure? From what I see the reviewers' +2s get lost when you upload a new patch, no matter how minor the change to the commit.

If such a solution were in place, I'd have to be even more aggressive at paying attention to reuploads, to avoid rubber stamping changed (and broken) logic. Today, I know that the most harm someone can do is change the commit message or perform no-op rebases (I think?).

Yes that's right. If you were nervous about someone's work you could watch the emails carefully!

 

Anyway, that's a single instance. There probably were significantly more where a carried +2 would do just fine.

Well perhaps we should try it for 3 months and see which people prefer, perhaps with a survey? Any other ideas on how to determine the cost/benefit?

[Mike Frysinger]

i wouldn't cite google3 as Chromium already does this

if we want to have OWNERS support, then we probably won't have a choice wrt sticky +2

if there's a concern that someone is going to (ab)use your sticky +2 to upload something completely different, then, as others have noted, they can already do that today.  if people aren't following our guidelines, then that's a social problem to take up & resolve with the specific people rather than trying to have a technical solution for it.

my biggest concern would be with non-committers uploading new changes after getting a +2, and their new change is evil.  i'm not sure if gerrit has ACLs to differentiate between uploaders ... i think we'd want the policy of +2 is sticky when the uploader is a person who already has +2 access, and for everyone else, they'd need a re-review.  i assume the Chromium project has already handled this somehow, so we could sync with them on it.

-mike

[Jonathan Brandmeyer]

On Mon, May 21, 2018 at 11:35 PM, Mike Frysinger <vap...@chromium.org> wrote:

my biggest concern would be with non-committers uploading new changes after getting a +2, and their new change is evil.

This risk is mitigated somewhat in Google3 by the email that the system sends out when a CL is submitted (`submit` in this case is analogous to `commit` in Git).  If there are any changes in between the last LGTM and the time it was submitted, then Critique/Piper specifically calls out those changes in the email.

-Jonathan

[Mike Frysinger]

Chromium can summarize that too, but i don't think these two scenarios are that comparable ... people committing to Google3 are employed by Google and so there's a lever to pull if they do something evil.  there is no such guarantee with open source projects ... the only barrier to contributions is creating a Gerrit account which only needs an e-mail address (iirc), so anyone can spin up throw away accounts at anytime.

-mike

[Jonathan Brandmeyer]

On Mon, May 21, 2018 at 8:58 PM, Simon Glass <s...@chromium.org> wrote:

Hi Brian,

On 21 May 2018 at 14:32, Brian Norris <brian...@chromium.org> wrote:

On Mon, May 21, 2018 at 1:22 PM, Simon Glass <s...@chromium.org> wrote:

On 21 May 2018 at 14:20, Brian Norris <brian...@chromium.org> wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

At least for the latter problem, your #2 becomes a feature, not a bug :D

Yes, but I suppose people can always +2 their own patch, so perhaps we should rethink that at the same time? 

Is that to say you can't self+2 in other systems (e.g., google3)?

Yes.

The situation is not the same.  In Google3:

- If the CL's author is not listed as an owner for this particular code, then the CL must be `approve`d by an owner for that particular project.  Approval is mostly used to ensure that at least one of {reviewer, author} is familiar with the particular project in question.

- The CL in question must be LGTM'ed.  An author cannot LGTM their own change.

- If the CL's author does not have `readability` for the programming languages in use by the CL, then the CL must be `approve`d by someone who does have readability for that language.

So in the typical case, the author is listed as an owner and they have readability for the language, so all your reviewers need to do is LGTM the changes before they are submitted.

[Jonathan Brandmeyer]

On Tue, May 22, 2018 at 8:42 AM, Mike Frysinger <vap...@chromium.org> wrote:

On Tue, May 22, 2018 at 10:37 PM Jonathan Brandmeyer <jbran...@chromium.org> wrote:

On Mon, May 21, 2018 at 11:35 PM, Mike Frysinger <vap...@chromium.org> wrote:

my biggest concern would be with non-committers uploading new changes after getting a +2, and their new change is evil.

This risk is mitigated somewhat in Google3 by the email that the system sends out when a CL is submitted (`submit` in this case is analogous to `commit` in Git).  If there are any changes in between the last LGTM and the time it was submitted, then Critique/Piper specifically calls out those changes in the email.

Chromium can summarize that too, but i don't think these two scenarios are that comparable ... people committing to Google3 are employed by Google and so there's a lever to pull if they do something evil.  there is no such guarantee with open source projects ...

By "lever to pull if they do something evil", I assume you mean disciplinary action.  That's true, but I'm not sure its important as far as project safety is concerned.  By far the most common lever to be pulled is that CL's are reverted (similar to git revert && git commit together), and the most common reason to pull that lever is that the CL broke another team's code halfway ~~around the world~~ across the company.  Usually these aren't caused by post-review changes, but if your reviewer really doesn't like how you addressed their feedback, they could revert the CL in the same way.

 

the only barrier to contributions is creating a Gerrit account which only needs an e-mail address (iirc), so anyone can spin up throw away accounts at anytime.

At least for Chromium, users need to sign a CLA.  I'm not familiar with all of the verification that goes on for CLA's, but I suspect that throw-away accounts won't meet the bar to pass them.

[Mike Frysinger]

On Tue, May 22, 2018 at 11:07 PM Jonathan Brandmeyer <jbran...@chromium.org> wrote:

On Tue, May 22, 2018 at 8:42 AM, Mike Frysinger <vap...@chromium.org> wrote:

On Tue, May 22, 2018 at 10:37 PM Jonathan Brandmeyer <jbran...@chromium.org> wrote:

On Mon, May 21, 2018 at 11:35 PM, Mike Frysinger <vap...@chromium.org> wrote:

my biggest concern would be with non-committers uploading new changes after getting a +2, and their new change is evil.

This risk is mitigated somewhat in Google3 by the email that the system sends out when a CL is submitted (`submit` in this case is analogous to `commit` in Git).  If there are any changes in between the last LGTM and the time it was submitted, then Critique/Piper specifically calls out those changes in the email.

Chromium can summarize that too, but i don't think these two scenarios are that comparable ... people committing to Google3 are employed by Google and so there's a lever to pull if they do something evil.  there is no such guarantee with open source projects ...

By "lever to pull if they do something evil", I assume you mean disciplinary action.  That's true, but I'm not sure its important as far as project safety is concerned.  By far the most common lever to be pulled is that CL's are reverted (similar to git revert && git commit together), and the most common reason to pull that lever is that the CL broke another team's code halfway ~~around the world~~ across the company.  Usually these aren't caused by post-review changes, but if your reviewer really doesn't like how you addressed their feedback, they could revert the CL in the same way.

yes, we have reverts on both sides.  when i say evil, i mean malicious, not buggy.  like Google, we don't care about "blame" when it comes to mistakes in Chromium.

as an employee, you have pressure to not do this because there's very obvious repercussions (firing/suing/criminal charges/etc...).

the only barrier to contributions is creating a Gerrit account which only needs an e-mail address (iirc), so anyone can spin up throw away accounts at anytime.

At least for Chromium, users need to sign a CLA.  I'm not familiar with all of the verification that goes on for CLA's, but I suspect that throw-away accounts won't meet the bar to pass them.

the CLA is basically a series of checkboxes & submit buttons.  there is no identity verification.  throw away accounts will get through this w/out a problem.

-mike

[Brian Norris]

Hi Simon,

On Mon, May 21, 2018 at 7:58 PM, Simon Glass <s...@chromium.org> wrote:

Hi Brian,

On 21 May 2018 at 14:32, Brian Norris <brian...@chromium.org> wrote:

On Mon, May 21, 2018 at 1:22 PM, Simon Glass <s...@chromium.org> wrote:

On 21 May 2018 at 14:20, Brian Norris <brian...@chromium.org> wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

At least for the latter problem, your #2 becomes a feature, not a bug :D

Yes, but I suppose people can always +2 their own patch, so perhaps we should rethink that at the same time? 

Is that to say you can't self+2 in other systems (e.g., google3)?

Yes.

 

 

Also, if you were to restrict this to just full committers, then what's the difference from today, with people being forced to put their own name on the "carried" +2?

Actually I would like to keep the existing +2 when uploading a new patch, not remove it. This is how google3 works.

I understand what you're proposing, but I'm just saying that your proposal (if restricted to full committers) can be implemented today with a self+2. And my claim was that if it's necessary, maybe that's a good thing -- I personally wouldn't want my +2 put onto the aforementioned CL (which I had to NAK).

Are you sure? From what I see the reviewers' +2s get lost when you upload a new patch, no matter how minor the change to the commit.

If you're referring to my earlier comment about retaining +2's: today, Gerrit will let you change the commit message and similar metadata and retain the +2, as long as the content diff doesn't change. (I don't remember if a no-change rebase retains +2.) Even trivial changes to the content will lose the +2.

 

If such a solution were in place, I'd have to be even more aggressive at paying attention to reuploads, to avoid rubber stamping changed (and broken) logic. Today, I know that the most harm someone can do is change the commit message or perform no-op rebases (I think?).

Yes that's right. If you were nervous about someone's work you could watch the emails carefully!

 

Anyway, that's a single instance. There probably were significantly more where a carried +2 would do just fine.

Well perhaps we should try it for 3 months and see which people prefer, perhaps with a survey? Any other ideas on how to determine the cost/benefit?

I'm not sure. Like I said, I don't have a strong opinion to voice. I just wanted to acknowledge that the existing system has positive features, and a sticky +2 can actually cause people more work. But a trial would be OK.

Brian

[Jason Clinton]

I think there's consensus here that OWNERS and sticky +2 are coupled because of the inability for contributors to self-+2 if the CL feedback is trivial. There are other pushes in other forums to enable OWNERS for general code health. Mike is the head of the newly reconstituted Build Team which owns the software build and repos. He's the decider in this case. If Mike wants to move forward with a trial, he has CI support.

[Luigi Semenzato]

This is a bit of a pipe dream, but it would be nice if the reviewers
could fix the nits themselves on the review site (i.e. without the
potentially messy step of downloading the patch). The CL owner would
then upload a new patch which would preserve the +2 as long as the
diffs were a subset of those proposed by the reviewers.

[Don Garrett]

The issue with throw away accounts that Mike mentions is a substantial security risk that doesn't exist in Google 3 because we somewhat trust Googlers.

If an external person can get builds run against an evil CL (even without it being submitted), they can utilize all of the credentials on our builds machines. In particular, that means elevated git and GS permissions, as well as the potential for persistent builder compromises. Our protection is that external CLs are never tested without internal approval.

If we can make +2's sticky for CL revisions that are uploaded by people with +2 rights, but not for uploads from people without such rights, I'm all for it.

A specific threat model to watch for:

  1) sjg@ uploads a good CL.

  2) vapier@ gives a +2.

  3) throwaway@ uploads an evil revision to sjg@'s CL.

  4) PreCQ runs.

[Mike Frysinger]

in your example, throwaway@ could do step 1 too, so the scenario of "taking over" a CL doesn't come up.  we have external contributors that post CLs that are indeed useful (and i don't just mean from partners).

-mike

[Don Garrett]

Yes, but presumably, you wouldn't give throwaways@ evil CL a +2. That scenario is a case where a sticky +2 could bypass the review requirements to reach a builder, if the sticky review isn't cleared when an external person uploads a CL.

[Mike Frysinger]

i've given +2 to CLs from external people who i don't know or otherwise plan on knowing.  i evaluate the quality of the CL they want to merge and nothing else.  i don't think we should be raising the bar for contributions.

the issue isn't that their first CL is evil, it's that after i give the +2, i go to sleep, and they turn around and upload another PS that is evil and the +2 is stuck.

-mike

[Don Garrett]

I agree with you.

That's why I'm saying that sticky +2 is good, but shouldn't apply to revisions uploaded by someone without +2 rights themselves. I don't know if Gerrit allows us to make that distinction or now.

[Aaron Gable]

Gerrit does not allow that distinction at the moment. Labels can be carried forward (or not) based on the content of the patch, but not based on the uploader of that patch: https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_copyAllScoresOnTrivialRebase

[Mike Frysinger]

thanks.  how does the Chromium project deal with this scenario ?

-mike

[Mike Frysinger]

i've opened https://crbug.com/852850 to track

in the mean time, i'll look at turning this on in chrome-internal as none of the security issues apply there

-mike

[Simon Glass]

Thanks Mike!

- Simon

[Kirtika Ruchandani]

On Monday, May 21, 2018 at 1:20:14 PM UTC-7, Brian Norris wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

Thanks for mentioning this. 

More details: 

Brian Norris has, in the past, found egregious mistakes in my later patch-set revisions, long after I got a +2, say PS#3 or PS#5.

e.g. CL:1034378 which I found by searching Gerrit for "your rebase wasn't great". 

This more likely means that I am a terrible developer, but I am sure I am not the only one who's tired or distracted after multiple attempts to get something right. 

I am grateful for reviewers like Brian, and this "feature" will take that away - reviewers don't have the same motivation to review if the same +2 stays sticky. 

How is an automated system useful if it relies on humans noticing and intervening in a situation like this: I make a trivial diff, have it +2ed by people I've never worked with, then change the diff to give the file a 'makeover'. 

They notice only when it lands. 

 

[Kirtika Ruchandani]

On Monday, May 21, 2018 at 1:21:08 PM UTC-7, Simon Glass wrote:

On 21 May 2018 at 14:17, Aaron Durbin <adu...@chromium.org> wrote:

On Mon, May 21, 2018 at 2:11 PM, Simon Glass <s...@chromium.org> wrote:
> Hi,
>
> In google3 when you get an LGTM it is sticky, meaning that you can upload a
> new commit and keep the LGTM, so long as yo don't add/remove any files.

Edits to existing files between patch versions may change logic. Why
would you want the +2 be sticky in that situation?

I assume a certain level of trust that people don't make significant changes (they are mostly +2 reviewers themselves, after all), and they have the option of requesting PTAL.

This is a terrible assumption. Significant changes can be introduced by other working on the file simultaneously whose commits got in first. 

In particular, this will make it way more difficult to folks to refactor files while other people are actively fixing bugs/adding code to it, because they have to go out of their way to 

notice the conflict and bug reviewers to review their conflict fix, while gerrit continues to show "CR +2". 

[Simon Glass]

On 15 June 2018 at 11:11, Kirtika Ruchandani <kir...@google.com> wrote:

On Monday, May 21, 2018 at 1:20:14 PM UTC-7, Brian Norris wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

Thanks for mentioning this. 

More details: 

Brian Norris has, in the past, found egregious mistakes in my later patch-set revisions, long after I got a +2, say PS#3 or PS#5.

e.g. CL:1034378 which I found by searching Gerrit for "your rebase wasn't great". 

This more likely means that I am a terrible developer, but I am sure I am not the only one who's tired or distracted after multiple attempts to get something right. 

I am grateful for reviewers like Brian, and this "feature" will take that away - reviewers don't have the same motivation to review if the same +2 stays sticky. 

How is an automated system useful if it relies on humans noticing and intervening in a situation like this: I make a trivial diff, have it +2ed by people I've never worked with, then change the diff to give the file a 'makeover'. 

They notice only when it lands. 

Have you tried removing their +2s? You can do that by removing them and re-adding them, but there might be a better way.

Regards,

Simon

[Mike Frysinger]

On Fri, Jun 15, 2018 at 4:38 PM Simon Glass <s...@chromium.org> wrote:

On 15 June 2018 at 11:11, Kirtika Ruchandani <kir...@google.com> wrote:

On Monday, May 21, 2018 at 1:20:14 PM UTC-7, Brian Norris wrote:

I won't give a strong opinion on this, but some anecdata and real data:

* +2 is sticky if the diff doesn't change (e.g., only commit message or metadata changed)

* I've very recently had to NAK people who "carried" my +2 incorrectly (their rebase was done incorrectly)

Thanks for mentioning this. 

More details: 

Brian Norris has, in the past, found egregious mistakes in my later patch-set revisions, long after I got a +2, say PS#3 or PS#5.

e.g. CL:1034378 which I found by searching Gerrit for "your rebase wasn't great". 

This more likely means that I am a terrible developer, but I am sure I am not the only one who's tired or distracted after multiple attempts to get something right. 

I am grateful for reviewers like Brian, and this "feature" will take that away - reviewers don't have the same motivation to review if the same +2 stays sticky. 

How is an automated system useful if it relies on humans noticing and intervening in a situation like this: I make a trivial diff, have it +2ed by people I've never worked with, then change the diff to give the file a 'makeover'. 

They notice only when it lands. 

Have you tried removing their +2s? You can do that by removing them and re-adding them, but there might be a better way.

you can always just ask people to take another look too

i don't think having sticky CR+2 effectively changes anything today.  there's no difference between you saying "inheriting earlier +2" & then self CR+2 your CL and having the existing CR+2 be sticky.  if you made a mistake and decided to move forward, then nothing is stopping you.

i think our approach has been and should continue to be "use your best judgement".  we don't operate a blame-based project (it's a complete waste of time, and at some point, every single one of us has or will have broken something), so if something does go wrong, we fix it and move on.

-mike

[Yves Arrouye]

There are a number of people (like me) who can't +2, and keep asking many times their reviewer to please +2 again after even trivial changes.

[Jorge Lucangeli Obes]

I would argue that if you find yourself needing to +2 your changes often, you should join a rotation and earn +2 rights.

[Yves Arrouye]

I don't, but I am sure it does bug my reviewers at times. I do try to minimize the churn though.