Changing policy for TBR of mechanical changes

[Gabriel Charette]

Hello all,

I'd like to propose a change to the official policy for the TBR of mechanical changes.

The official policy is effectively:

1. Get an owner of the mechanical API change to review
2. Get someone (usually same owner) to review the side-effects (and/or the script that did them).
3. TBR owners of each affected directory

My problem is (3.). It feels absolutely pointless, for example, to TBR 50 owners when renaming a method or include in //base. It's time consuming for the CL owner to find all the owners and it's noise for the 50 owners who receive a review they pretty much have no say upon.

(note that top-level src/owners strongly object to be the sole reviewer for (3.) -- rightly so IMO)

What I've been doing instead on such CLs is TBRing the owner from (1.) after they've LGTM'ed the mechanical change. The TBR overrides the owners rules and lets the CL land as if the API had been named like that in the first place, no need to tediously involve 50 people.

Example CL : https://chromium-review.googlesource.com/c/chromium/src/+/1327441

As such I propose changing (3.) in the docs

from:

• Add the owners of the affected downstream directories as TBR. (In this example, reviewers from //chrome/browser/foo/OWNERS, //net/bar/OWNERS, etc.) 

to

• TBR the owner of the lower-level code you're changing (in this example, //base), after they've LGTM'ed the API change, to bypass owners review of the API consumers.

Thanks,

Gab

PS: This is obviously only for purely mechanical changes. Anything that changes semantics should use git cl split and get proper owners review from individual API consumers in focused CLs (and I'll update the doc to reflect that too).

[Alexei Svitkine]

+1

I agree that manually adding a bunch of leaf OWNERS is not a useful use of time for anyone.

--
--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com.

[dan...@chromium.org]

+1 to decreasing friction without decreasing meaningful code review.

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

[Peter Kasting]

LGTM

PK

[Sébastien Marchand]

+1, for the CL mentioned in Gab's email I got an LGTM from a base owner and then as I was simply fixing an include in a bunch of files (~350) I TBR'd jam@ as a root owner (which is a bad idea I agree), then as jam@ told me that this isn't the policy I used 'git cl owners' to find a set of owners for my CL. I don't think that adding these people to my CL added any value (as the base/ changes had already been approved), so removing this restriction would indeed simplify the process (for pure mechanical changes only) .

Thanks for suggesting this Gab.

--

Sébastien Marchand | Software Developer | sebma...@google.com 

[Lambros Lambrou]

+1

TBR for per-directory owners feels unnecessary if top-level reviewer approves.
It's a reality of Chrome development: any change to base/ will impact lots of code throughout Chrome, regardless of whether the change touches a file somewhere else. It's unnecessary to spam TBRs everywhere.

I think author+top-level-reviewer should make the determination of whether a change is really mechanical or not, and should agree whether to split it up for per-directory review.

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

[Dirk Pranke]

I don't have a strong opinion on this from a policy standpoint, but -- assuming I understand what is being proposed correctly --  from a mechanical standpoint I'm not wild about this, because it confuses the record in the CL and makes it look like someone needs to review something after the fact when that's not actually the intent.

I suggest we do something like add a new "Large-Scale-Change-Reviewer:" header and make the tooling understand that instead.

-- Dirk

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

[Chris Hamilton]

This seems like a reasonable compromise. FWIW, I imagine the CL record is already littered with these kinds of "fake" TBRs, as people have been unofficially using this workflow for a while (forever?) despite it not being strictly in accordance with the letter of the law.

Cheers,

Chris

[Dirk Pranke]

Yes, I would expect so as well, but that doesn't mean we can't improve things now ;).

-- Dirk

[Gabriel Charette]

Glad everyone likes it :)

@Dirk : IIUC you dislike using TBR to bypass the owners check as it means "to be reviewed" whereas the review already occurred? I'd be inclined to use another label that bypasses owners check if infra wants to add one. e.g. RESTRICT_OWNERS=base or something that still forces an owners stamp for the core change but bypasses owners check for side-effects? The core owner would then see that too and explicitly know what his review entails. 

Since this thread is more about making an unofficial practice become the official policy, I don't think we should block it on such a label being added however. We can update the policy once such a label is added. I don't think we have tooling that detects/counts/takes action upon TBRs (besides presubmits?), but even if we did this proposal is a no-op from the current policy to TBR 50 owners.

[Dirk Pranke]

On Wed, Nov 14, 2018 at 4:35 PM, Gabriel Charette <g...@chromium.org> wrote:

Glad everyone likes it :)

@Dirk : IIUC you dislike using TBR to bypass the owners check as it means "to be reviewed" whereas the review already occurred? I'd be inclined to use another label that bypasses owners check if infra wants to add one. e.g. RESTRICT_OWNERS=base or something that still forces an owners stamp for the core change but bypasses owners check for side-effects? The core owner would then see that too and explicitly know what his review entails. 

Yes, more or less. I had been thinking of a header that was a boolean or an email address, but I supose something like a path expression could work as well.

I know I said I didn't have a strong opinion on this from a policy standpoint -- and I don't -- but I will note that in general we've tended to try and follow Google's internal code review practices for things like OWNERS, and they don't do what's being suggested here. Instead, they have a combination of things: the first is a tool to help manage splitting CLs for approval, similar to what we have with `git cl split` but (I think) with some better support that we could probably work on. The second is an explicit list of people who are authorized to approve large-scale changes, which isn't necessarily the same as the people who are otherwise top-level OWNERS. We could do the latter now if we wanted. But, I'm not sure if that's really a better proposal than what gab@ is proposing, just more paranoid ...

-- Dirk

[Gabriel Charette]

Re. having large-scale owners : those are effectively //base owners in practice (for base and general C++ changes) but I wouldn't want to have a rule that dictates that as an API change in say //net should be vetted by a net owner. I think it makes the most sense in all cases to have the API owner stamp the change and its incurred side-effects (the side effects are just broader when coming from //base and alike).

Re. git cl split : agreed that the internal equivalent is much fancier and that it'd be great to import some features. But that's beyond the scope of this discussion (though I will add a note about git cl split in the modified docs for scripted but not strictly mechanical changes).

[Gabriel Charette]

FYI, this policy change landed @ https://chromium-review.googlesource.com/c/1340262