PSA: Code-Review is no longer sticky when files are added/removed in subsequent patchset uploads

[Jason Clinton]

tl;dr: Changing the list of files affected will need to be code reviewed.

After reviewing reports of problematic changes landing without review and consulting with our policy teams, for chromium/src, we have disallowed the Code-Review label to carry over to a newly uploaded patchset in cases where the uploader added a new file or removed a file. As a code reviewer, you can see the changed files needing code review by choosing the diff dropdown in the Gerrit UI:

All other repos on the chromium Gerrit host were already using this policy and there are no changes to those repos.

If you see a bug or have questions, please file a bug here:

https://bugs.chromium.org/p/chromium/issues/entry?template=Infra-CodeReview

[Jason Clinton]

Thank you to everyone who has provided feedback on this change; my apologies for the friction that this has caused. It's clear that this has caused too much productivity impact and we can make some improvements before we enable this again, so we're rolling back this change to chromium/src, for now. Before we re-enable, we will implement quality-of-life improvements like better visibility into vote removals and only requiring votes for the added/removed files.

In the meantime, to keep our codebase in a good state, we ask that code reviewers be diligent in checking the post-review diff emails that are sent by Gerrit to watch for changes that are outside of your expectations. These emails look like this:

[Jason Clinton]

We've improved the Gerrit user experience related to Code Review copying (and being required again) between patchsets. This coming Monday, we will enable requiring Code Review +1 again for chromium/src, if the list of files changes (files added or removed) in a new patchset.

The two most visible changes are that Gerrit will now explain when a patchset caused a Code Review to be dropped as can be seen in this screenshot (taken from ChromiumOS which uses CR+2 and Verified+1):

Additionally, Gerrit now shows individual file OWNERS approval as well as the file add/remove status as a small icon next to the file list, to help clarify that a file was added in a later patchset:

[Xianzhu Wang]

Does the policy apply to third_party/blink/web_tests/platform?

I often request review before web test rebaselining is finished. My reviewers often view web test results.html to see what baselines will change and approve if results.html on one or several platforms look good. It's more convenient than looking at the baseline files. I may finish rebaseline (which may add or remove files under third_party/blink/web_tests/platform) in subsequent patchsets until the bots are all green, which may take a long time because flaky tests cause flaky rebaseline results. If this policy also applies to third_party/blink/web_tests/platform, this review process will last longer.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACVW55hw9jA8DaABNwDk19TO7_RJRFGgSuJi%2B9O_HivmGrcHDA%40mail.gmail.com.

[Jason Clinton]

Hi Xianzhu,

Every file in an CL in chromium/src will need to be code reviewed starting on Monday, not just the files that were in the first patchset.

[Xianzhu Wang]

I would like to request an exception for third_party/blink/web_tests/platform because adding/removing files under it won't pose problems if reviewers approve subsequent rebaselines.

[Jason Clinton]

Hi Xianzhu,

If you happen to know which files you'll touch in later patchsets, you can include a trivial/whitespace change to mark those files as modified in patchset 1. Then, the code review will carry forward to future patchsets because your code reviewer will have reviewed that you intended to modify those files. This will work if you don't add or remove any files from your patchset.

Exceptions to the code review policy are not possible, for reasons that I cannot discuss here. You can ping me directly and I can provide some details. But, there's neither a policy or a technical way to implement this or similar requests.

[Brian Osman]

I'm sorry, but given the state of blink's rebaseline script and procedure, this is going to make landing many changes effectively impossible. Layout tests are incredibly flaky, and every attempt to rebaseline causes the script to churn on expectations files. Getting ahead-of-time LGTM on those expectations is the only thing that makes landing rendering changes possible. Adding review latency to the process where we already try (repeatedly, over the course of DAYS) to get the script to reach steady state is going to push this over the limit.

[Jason Clinton]

This was implemented, that's what currently live. Can you open a go/fix-chrome-git so we can investigate? Happy to follow-up there.

On Mon, Aug 1, 2022 at 4:59 PM Tim Sergeant <tser...@chromium.org> wrote:

Hi Jason, in your previous email you said,

"Before we re-enable, we will implement ... only requiring votes for the added/removed files."

Is there a reason why this was not implemented? The re-launched implementation is still very disruptive.

(The situation I ran into this morning: As a reviewer, if I ask for a change to a unit test I own, I'll delay the CL by at least a day since other international reviewers will need to re-stamp)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/89be17e1-139e-42f5-945c-72899a653a52n%40chromium.org.

[Ken Russell]

This seems to have happened on one of our team's CLs - a Code Owner's approval was invalidated by a subsequent patch set which didn't touch the approved files. Have filed http://crbug.com/1349548 with the details.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACVW55jLjjk4_4ZPGWc3jRhnMniCxbU6P%3D5oykxgXoHQ4HOtZA%40mail.gmail.com.

[Xiaohui Chen]

One of my team's CLs [1] dropped all +1s between ps2 and ps3 which rebased and fixed merge conflicts on existing files.  Is this expected?

https://chromium-review.googlesource.com/c/chromium/src/+/3786862

[Aaron Leventhal]

A suggestion would be to add a comment that explains which reviewers were dropped, and re-add them to the attention set, letting them know they may need to re-review.

Unless I've missed something, +1's feel like they just disappear.

A lot of times to increase everyone's velocity, the code reviewer will say LGTM with a few changes. But when the coder makes the changes, and then they can't submit. It's not obvious at first glance what actually happened.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/f012a5cb-d558-4558-8396-46dc52639815n%40chromium.org.

[Jason Clinton]

Two files were added in patchset 3; adding any new files resets the Code-Review label:

[Jason Clinton]

This confusionis my fault: in the haste to explain Code Review, I explained this poorly and assumed some behavior about Owners enforcement that's not true. Apologies for this. I've investigated this with the new folks working on Owners.

The correct explanation: What we deployed to chromium/src on Monday, is the same configuration that's been live on internal repos and all other repos on the Chromium Gerrit instance since Q2 2021 *with* the new UI and transparency improvements: Owners enforcement exclusively looks at the Code-Review label. And, *all* positive Code-Review label votes reset if a later patchset adds or removes files. This means that my explanation that Owners was a separate stickiness state was wrong. My apologies for the confusion caused.

We have this UI element in the message log, to specifically address your question:

However, we agree that this could be clearer. We have this bug tracking showing which of the criteria listed was evaluated to "true" causing the votes to be removed: https://buganizer.corp.google.com/issues/240377462 (only visible to Googlers, sorry).

[Christian Dullweber]

Why do *all* positive code-review labels have to reset when a file is added? Wouldn't it be sufficient to only reset related OWNERS? 

At least for files not owned by *, this would be a little less disruptive?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACVW55gq5cNh5f4zPStYv0rQW1YDxOE7t9%3DXOXK%2Bwm37r1ZD%3DQ%40mail.gmail.com.

[K. Moon]

I think the way this works internally is that only the added files require additional review. It seems like the problem here is that we can't track the votes at the level of individual files, so they get reset for the entire change.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKAfQi1at-dCc92PEjpzyubd54Z-9Oy62a8nb_6gVTwYXdkb2Q%40mail.gmail.com.

[Daniel Cheng]

This new behavior can be surprisingly disruptive and will encourage more use of Owners-Override and the review burden for OWNERS with Owners-Override powers.

https://chromium-review.googlesource.com/c/chromium/src/+/3805397/6..7 is the most recent example: I asked for a revert to one file in the CL, which reset the CR+1 from *three* other reviewers. This CL will likely land with the use of Owners-Override rather than asking all the reviewers to look again.

The author *could* split this CL up into much smaller CLs, but splitting up CLs has significant overhead (git cl split is overly granular; manually splitting CLs is time-intensive; trying to set up a chain of dependent changes with a split CL is similarly time-intensive; et cetera). There are also other developer workflows that are negatively impacted by this change.

In theory, it would be nicer if CR+1 was tracked per-file rather than per-CL. That way, newly-added files would require CR+1 without losing all the previous CR+1s. At the same time, I'm not sure what a reasonable model should be to require re-review if a file is removed from the CL: you can't really require CR+1 on a file that is no longer in the CL.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACwGi-59%2BZhvPLe4qSSFE0_DOoqmqEUorRJW4j23YnFx5LMTEw%40mail.gmail.com.

[Aaron Leventhal]

Maybe I'm missing something, but can't it just remove the +1's where they are an owner for the added file?

In addition, I would suggest that +1's don't need to be changed for removed files or test files.

Aaron

[K. Moon]

I think test files definitely need to be included (imagine I change the behavior of an existing file from "good" to "evil", then add and update the tests so it passes the CQ). I think the argument is weaker for removed files, although I think it's also theoretically possible to support a +1 for dropping a file (just maybe not easy to implement).

[Aaron Leventhal]

That's theoretically true, but is it worth the loss in velocity? I have a good amount of trust in my contributors — when I add "+1 if you do X", they do it. It saves me time and I get more done somewhere else. If they don't have a track record, then I wait until they're completely finished before adding my +1.

Aaron

[Aaron Leventhal]

Also, regarding the test file changes — presumably if it goes from good to evil, something other than a test will change.

[K. Moon]

I think it's fairly trivial to construct a plausible example:

verify_something.cc:

  if (!IsSameOrigin(url))

    return;

Becomes:

verify_something.cc:

  // Delete the same-origin check.

verify_something_test.cc:

  TEST(VerifySomething, SameOriginCheck) {

    // Empty this out, everything is fine now.

  }

To the larger point about trust in contributors, the whole point of this exercise is because we're not trusting people. There seem to be legal and policy reasons for that, so I'm not going to question the objective, just the implementation.

[Aaron Leventhal]

I understand. But let's find as many ways to keep velocity up that we can.

In your example, more than a test file changed.

[K. Moon]

Right, but we don't have to +1 modifications to existing files (which is a concession to velocity). Essentially, test files are not "less special" than any other file you'd want to add to your change, they shouldn't be treated differently.

[Daniel Cheng]

Imagine:

1. ap...@chromium.org owns //foo and //bar.

2. ban...@chromium.org owns //bar and //baz.

3. a change contains changes in //foo, //bar, and //baz.

4. apple@ LGTMs a change that contains //foo and //bar.

5. banana@ requests changes in //bar be changed in some way that adds or removes a file.

apple@'s LGTM is removed due to owning added or removed files in //bar, which also clears it from //foo.

Daniel

[Tim Sergeant]

Hi Jason, in your previous email you said,

"Before we re-enable, we will implement ... only requiring votes for the added/removed files."

Is there a reason why this was not implemented? The re-launched implementation is still very disruptive.

(The situation I ran into this morning: As a reviewer, if I ask for a change to a unit test I own, I'll delay the CL by at least a day since other international reviewers will need to re-stamp)

On Sat, 30 Jul 2022 at 16:00, 'Brian Osman' via Chromium-dev <chromi...@chromium.org> wrote:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/89be17e1-139e-42f5-945c-72899a653a52n%40chromium.org.

[Tim Sergeant]

Hi again Jason, 

I am fully supportive of the intended behavior, but if the currently launched implementation does not work as expected (with no sticky owners approval), can we consider turning it off again until it is fixed? As I mentioned before, the change is very disruptive for people who regularly do cross-timezone reviews.

Thanks, Tim

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Daniel Cheng]

Other examples of problematic workflows:

1. CL is LGTMed provisionally with a request to split out new stuff into a separate file.

2. CR+1 is cleared, despite the original reviewer being implicitly OK with this.

The net effect is:

- it makes it harder for a CL author that is remote to the OWNERS to contribute to that area of code

- it makes it harder to split code out into smaller files, which is something we already have problems with

Also problematic:

1. cl-reviewer@ LGTMs a CL.

2. cl-author@ uploads a CL mistakenly including some files they didn't mean to.

3. cl-author@ uploads corrected CL reverting those files.

LGTM is now gone. This is not theoretical: I saw an instance of this just this week, and I know I've done this by mistake at times too, when chaining CLs or juggling multiple CLs in different branches.

Daniel

[Matthew Menke]

This happens a lot to me as well.  Tooling catching that case is actually a good thing, but not restoring the +1s once the extra files are removed still makes life very painful.  +1s can sometimes take days to get the first time, and this potentially adds days more of nagging reviewers to +1 again.

I'm suspicious of the utility of this behavior in general - if the concern is malicious changes, then it seems I'd just need to make a legitimate CL touching files I want to modify (fixing typos, for instance), then get a signoff, and then make the CL malicious.  If the concern is accidentally introducing bugs, then adding new files doesn't seem significantly more likely to be a bug introducing change than modifying files already in the CL.

[Brian Osman]

Yes. The line between "subsequent changes to files that were already in the CL" and "subsequent changes to files that weren't already in the CL" seems entirely arbitrary and artificial. It's a construct that seems indefensible from a policy or technology standpoint. Either we want sticky +1, or we don't. 

[Brian Osman]

... actually, I'll take that back. I suspect the concern is that if you can get a +1 from someone that has OWNERS over a large portion of the repo (vs. someone that just has OWNERS over a specific component), then it's much easier to make far-reaching changes? But it's still just a matter of degrees.

[Peter Kasting]

On Thu, Aug 4, 2022 at 8:14 AM Aaron Leventhal <aleve...@chromium.org> wrote:

Maybe I'm missing something, but can't it just remove the +1's where they are an owner for the added file?

+1 -- I very commonly do large CLs with many OWNERS and this change has been unnecessarily disruptive for me, repeatedly, in the last week.  Making the above change would significantly improve the ergonomics.

PK 

[Sky Malice]

This change has been slightly painful and annoying over the last few weeks. Finally got frustrated today and found this thread, and it was interesting to finally understanding the criteria for when it removes all +1's from CLs. Over the course of today, I find myself reacting to feedback on CLs differently. Knowing that if I modify/add a new file, I'm going to lose all +1s, I'm now very hesitant to add a new test or metric to an existing CL that has any +1s on it. Maybe this will be beneficial and these tests/metrics should have really gone into new smaller CLs. Dunno.

[Avi Drissman]

I am an owner of several top-level directories, and do ~2000 reviews a year. In the time that this has been active, my review queue has roughly doubled, and half the time I go to a CL for review, I've +1ed it but that was lost. In a case like that, I don't have much of a choice but to trust that the author was responding and making changes to suit other reviewers, and unless they call me out with specific changes that they want me to look at again, I will deliver a +1 stamp.

Avi

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/d9182b1b-353c-4d12-99f5-26539dd9a1b3n%40chromium.org.

[Fabrice de Gans]

While I do not have the same amount of code reviews as Avi, I second their comment. This was a well-intentioned change but the consequences are negative for the codebase. I also find myself rubber-stamping changes across patch sets if I have previously +1'd the CL. This just adds to churn and discourages doing proper refactors / adding tests.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACWgwAaN2KG8GLUqbD9U_1kUEojpHnGBtLMALSF-vJ4hfvX1Nw%40mail.gmail.com.

[Peter Kasting]

To be concrete: I now find myself resisting adding tests and doing refactors because I know that it will reset all the +1s on my CL.  This is a toxic effect to code quality.

PK 

[Ken Rockot]

Is the team considering any longer-term alternatives to satisfy these requirements with maybe less collateral damage to productivity? Naively, something like per-file CR bits sounds about right.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFA9fWTFGg6Zz%2BAr35e%2BWzsJ307VhCQni3vYHjkOQ62d5Q%40mail.gmail.com.

[Jason Clinton]

I'll reply privately with more details and context.

[Ryan Hamilton]

I recognize that when dealing with security issues it's often not possible to have everyone "in the know" about all the details. But at the same time, it's challenging to understand this discussion when details and context are delivered privately off-thread. If these details or context can't be public, is there some wider forum they could be shared in, perhaps Google-internally?

Cheers,

Ryan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACVW55ixp0x72DcDxyBzn4b8rXR7QaZpq8VyH6%2BArY7XcDgHfg%40mail.gmail.com.

[Christian Dullweber]

+1, I find it pretty disappointing that there have recently been a couple of announcements to chromium-dev that intentionally left out the actual reason why these changes are happening. Of course this starts a huge discussion. Similar to this change, the "inactive owners removal" had some hidden goals that could only be figured out through a private email discussion.

Would it be possible that changes with a huge impact like this are discussed with a design doc with the whole background e.g. on chromium-dev-internal@ or chrome-design-docs@?

Even with the explanations from Jason, I still don't understand how this change helps against a malicious person who tries to submit bad code since you can just do that in any file in your CL that got approval. If the real reason is just that we must make some arbitrary changes because of some audit, then it would still be nice to understand.

[Nicolas Capens]

On Fri, Aug 26, 2022 at 4:20 AM Christian Dullweber <dull...@chromium.org> wrote:

+1, I find it pretty disappointing that there have recently been a couple of announcements to chromium-dev that intentionally left out the actual reason why these changes are happening. Of course this starts a huge discussion. Similar to this change, the "inactive owners removal" had some hidden goals that could only be figured out through a private email discussion.

Would it be possible that changes with a huge impact like this are discussed with a design doc with the whole background e.g. on chromium-dev-internal@ or chrome-design-docs@?

Even with the explanations from Jason, I still don't understand how this change helps against a malicious person who tries to submit bad code since you can just do that in any file in your CL that got approval.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKAfQi1nQPR%2BBRvwo--mfU1YoYmX%2B24NWaN3XpqmL6gxEEgEDQ%40mail.gmail.com.

[Mason Freed]

It's of course hard to trade off velocity/quality concerns against security concerns. But I think from this thread, it is fairly clear that this change is negatively affecting velocity. I would submit that it also negatively affects quality. I am used to addressing all of the concerns of my reviewers, even if they've LGTM'd the change already. Now I'm extremely sensitive to what I change after receiving an LGTM, for fear of touching the wrong file. I'm more inclined to push back, to avoid having to re-ping reviewers whose +1's are wiped away. That's generally bad. This will lead to more patches landing with not all comments addressed, either forever or until a future patch lands.

Having said the above, within Blink, the negative impact of this change could be at least partially alleviated by exempting the third_party/blink/web_tests/platform folder. That is not a security-impacting change. Tests can already be disabled through several means, which means that changes to those tests should not be locked down in the same way as changes to code or even unit tests. So I'd like to second (or third?) the request to exempt that folder.

Thanks,

Mason

On Thursday, August 18, 2022 at 11:05:41 AM UTC-7 Peter Kasting wrote:

[Tommy C. Li]

Hello -- are we able to roll back this change until the crbug and b/ are fixed?

https://bugs.chromium.org/p/chromium/issues/detail?id=1351009

b/242255115

I think this change would be a good chance once Gerrit has good per-file approval tracking, but as-is, when every +1 is removed after a file is added, I found it to painful for reviewing CLs.

[Julie Parent]

Hey folks,

I just wanted to provide a bit of an update, as we hear you loud and clear that this is disruptive and don't want a lack of response to indicate that we are not paying attention.  We are.  We are working on comms with further context and next steps that we'll be able to share (unfortunately only internally) very soon, which will include ways you can provide feedback as we navigate these necessary changes.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/07fc3adf-f476-4a3f-97d7-d7fe60153792n%40chromium.org.

[Kyle Horimoto]

Hi Julie, checking in on this topic. Is there any update regarding next steps for improving this code review flow? Thanks!

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Julie Parent]

Hi Kyle,

Thanks for checking in, and sorry for the radio silence on this thread.

To provide context, we made these changes to comply with Google policy. We cannot revert the changes, but we are working with our policy and Gerrit teams to see if we can comply while providing a better experience for Chromium developers than the current behavior.

Some amount of re-review is going to be required ongoing (Code-Review+1 will be dropped when the list of files changes). But we are working on getting approval for OWNERS approval to be sticky per-file, such that the amount of re-review needed would be substantially less: if files are only removed any committer can re-approve, and if files are added re-review additionally needs to provide OWNERS approval for the added files if the author is not an OWNER. I want to emphasize that we cannot yet guarantee these improvements - we're still seeking approval.

We've heard the requests for an even more permissive policy, e.g., specific exceptions or going back to complete stickiness, but we cannot support that at this time. We're doing our best to make this system as flexible as possible for you within our constraints.

Thanks,

Julie (on behalf of many, many others)

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[David Black]

Is there a presubmit that runs to warn "By uploading this patchset all/some votes will be reset" so that I can abort before the votes are removed? Feels like usually when votes are reset on my CLs its due to avoidable circumstances, like a bad rebase introducing a new file to the CL that I failed to notice locally. Maybe a presubmit warning already exists and I've just never noticed it :)

- David

[K. Moon]

I'm willing to wait and see what the teams working on this come up with, but I think it'd be really cool if you got the votes back whenever the current patchset matches a previously-approved one.

[Josip Sokcevic]

On Thursday, October 20, 2022 at 3:00:18 PM UTC-7 km...@chromium.org wrote:

I'm willing to wait and see what the teams working on this come up with, but I think it'd be really cool if you got the votes back whenever the current patchset matches a previously-approved one.

If we get an approval for OWNERS approval to be sticky per-file, I think this won't bring much value. There are some technical challenges with this idea (e.g. how Gerrit saves and drops votes information), and probably security too.

 

On Thu, Oct 20, 2022 at 10:49 AM David Black <dmb...@google.com> wrote:

Is there a presubmit that runs to warn "By uploading this patchset all/some votes will be reset" so that I can abort before the votes are removed? Feels like usually when votes are reset on my CLs its due to avoidable circumstances, like a bad rebase introducing a new file to the CL that I failed to notice locally. Maybe a presubmit warning already exists and I've just never noticed it :)

We don't have such presubmit check. It's an interesting idea, but implementing it in depot_tools would be a challenge as we would have to replicate Gerrit's logic for votes carrying. OTOH, adding a simple warning that votes may be dropped is not that useful IMO.

[dan...@chromium.org]

On Mon, Oct 24, 2022 at 7:36 PM 'Josip Sokcevic' via Chromium-dev <chromi...@chromium.org> wrote:

On Thursday, October 20, 2022 at 3:00:18 PM UTC-7 km...@chromium.org wrote:

I'm willing to wait and see what the teams working on this come up with, but I think it'd be really cool if you got the votes back whenever the current patchset matches a previously-approved one.

If we get an approval for OWNERS approval to be sticky per-file, I think this won't bring much value. There are some technical challenges with this idea (e.g. how Gerrit saves and drops votes information), and probably security too.

 

On Thu, Oct 20, 2022 at 10:49 AM David Black <dmb...@google.com> wrote:

Is there a presubmit that runs to warn "By uploading this patchset all/some votes will be reset" so that I can abort before the votes are removed? Feels like usually when votes are reset on my CLs its due to avoidable circumstances, like a bad rebase introducing a new file to the CL that I failed to notice locally. Maybe a presubmit warning already exists and I've just never noticed it :)

We don't have such presubmit check. It's an interesting idea, but implementing it in depot_tools would be a challenge as we would have to replicate Gerrit's logic for votes carrying. OTOH, adding a simple warning that votes may be dropped is not that useful IMO.

What if it was printed out after uploading, since that communicates with gerrit (if it does)?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/911cae1f-aff3-468c-b9be-4035068d9c96n%40chromium.org.