New issue
Advanced search Search tips

Issue 737128 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

TBR makes it too easy to skips owner checks. Proposal: TBR[content/foo] = owner

Project Member Reported by primiano@chromium.org, Jun 27 2017

Issue description

Forking this form  Issue 736215  comment #24 and adding some extra thoughts I had recently on the topic

Problem
-------
I see the practical need for TBR but there are two general problems:

1) It's not immediate for reviewers getting TBR'd to figure out what are they TBR'd for. Not every author is nice enough to write "TBR foo@ for content/browser/bar.cc". And even when they do, it's quite hard to scrape these in CLs that have too many comments.

2) TBR completely bypasses owners check, even when the author doesn't mean it.
Concrete example: in [1] I'm TBR-ing two people because I am doing silly mechanical changes in chrome/browser/metrics/* and net/url_request/url_request_quic_perftest.cc.
The rest of the CL is not mechanical though and I don't want to accidentally bypass the actual reviewers. For instance in that CL I am touching a mojom file that requires a SECURITY_OWNER stamp. I really don't want to accidentally skip that.

Proposal
--------
Rework the TBR format to be very explicit. For instance in that CL I would have loved to:
TBR[content/browser/metrics/*] = asvitkine
TBR[net/url_request/url_request_quic_perftest.cc] = xunjieli

And leaving the possibility to do 
TBR[*] = foo
In cases of urgent things (e.g. reverts) 

And, of course, TBR[*] should be extremely socially unacceptable for anything other than reverts.

[1] https://chromium-review.googlesource.com/c/544976/

 

Comment 1 by thakis@chromium.org, Jun 27 2017

I think just "TBR=name1,name2" is simpler. Assume name1 and name2 lgtm'd for the purpose of owner checking. If you need something urgent, you can list a toplevel owner (or a list of more detailed owners) plus the nonparent owners. That matches what we do for normal CLs.
Cc: aga...@chromium.org
Components: Build
Status: Available (was: Unconfirmed)
I think it's just a bug that TBR is bypassing owners checks. I don't remember offhand if I did that intentionally and, if so, why.

Comment 3 by aga...@chromium.org, Jun 27 2017

It's a "feature" that TBR bypasses OWNERS checks, and it has been that way for as long as I can remember. But yes, the system should be revamped to simply include anyone on the TBR line in the list of reviewers for the sake of computing OWNERS coverage, and if the set still isn't covering, reject the CL.
Components: -Infra>CQ Infra>SDK
Gerrit CQ will no longer care about TBR. presubmit support which is part of depot_tools does.
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 4

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -OS-Mac
Status: Available (was: Untriaged)

Sign in to add a comment