[chromium-dev] 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
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed>
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.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[d***@chromium.org]

+1 to decreasing friction without decreasing meaningful code review.

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful use of
time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful use
of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
Sébastien Marchand | Software Developer | ***@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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful use
of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[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

On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful use
of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-
HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXW
U5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%
3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[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

Post by 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
On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful use
of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Dirk Pranke]

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

-- Dirk

Post by 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

Post by 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
On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful
use of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-
HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXW
U5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%
3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[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.

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

Post by 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

Post by 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
On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.
On Wed, Nov 14, 2018 at 2:42 PM Alexei Svitkine <

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful
use of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Dirk Pranke]

Post by 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.

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

Post by Gabriel Charette
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.

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

Post by 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

Post by 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
On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.
On Wed, Nov 14, 2018 at 2:42 PM Alexei Svitkine <

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful
use of time for anyone.

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%
40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXW
U5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%
3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[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).

Post by Dirk Pranke

Post by 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.

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
paranoid ...
-- Dirk

Post by Gabriel Charette
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.

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

Post by 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

Post by 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
On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.
On Wed, Nov 14, 2018 at 2:42 PM Alexei Svitkine <

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a useful
use of time for anyone.
On Wed, Nov 14, 2018 at 11:40 AM, Gabriel Charette <

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR of
mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Gabriel Charette]

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

Post by 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).

Post by Dirk Pranke

Post by 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.

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
paranoid ...
-- Dirk

Post by Gabriel Charette
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.

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

Post by 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

Post by 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
On Wed, Nov 14, 2018 at 11:58 AM, Lambros Lambrou <

Post by 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.

Post by d***@chromium.org
+1 to decreasing friction without decreasing meaningful code review.
On Wed, Nov 14, 2018 at 2:42 PM Alexei Svitkine <

Post by Alexei Svitkine
+1
I agree that manually adding a bunch of leaf OWNERS is not a
useful use of time for anyone.
On Wed, Nov 14, 2018 at 11:40 AM, Gabriel Charette <

Post by Gabriel Charette
Hello all,
I'd like to propose a change to the official policy for the TBR
of mechanical changes.
The official policy
<https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#TBR-To-Be-Reviewed> is
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.
https://chromium-review.googlesource.com/c/chromium/src/+/1327441
As such I propose changing (3.) in the docs
- 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).
--
--
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
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BA81ZjbkyRU215hU%2Bu83dweSPTuxmqG-gN9u_D1hkiDg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SBByNrwpLGi7KDMAGrh-HcDi8siVqe-jkfe9Pa4b_huuA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRck7KWDsjT9AfW41rVnNqXWU5yQM0Rtxdu%2BErxcKywJQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
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/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHbUkqHB3Lg7mvnVWE7Ntc-%3DLjv8DaiJAwCqBVf7Vfi9%3D0Jtkw%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Peter Kasting]

LGTM

PK