I personally think we should completely drop support for TBR=.

It is not secure (never notifies who is TBRed) and I am not a fan of anything we have to text parse from the description.
Gerrit allows us to add people to the reviewers list and +1 the change ourselves if we want to land it. The reviewers list will get email notifications and can later add any comments if they would like to.
There is one case I am currently depending on: When you upload a CL adding TBR=xyz it automatically +1s the Code-Review label. We can get around this by instead calling SetReview with labels={'Code-Review': 1} in gerrit_util. There is some weirdness with auth here that I will get into later.

See what I wrote in Tips for Skia regarding TBRs: https://docs.google.com/document/d/1NPElUfKxL3_tZzEoKYihgHyj5j4TGMxLkLN3YFg4V9Q/edit#heading=h.7em71jefrgu1

I agree with you that we should get rid of TBR=, but I don't think we are in position to get rid of TBR in description just yet.  So, IMO we should strongly consider getting rid of TBR *after* Rietveld is read-only.

And till then, I think this is a confusing regression.
Also, presbmit still requires that, IIRC. 

Another strategy is to say we do not support it at all in Gerrit because of the additional flexibility it gives us over Rietveld. We would have to continue to support it for Rietveld ofcourse, but moving to a new framework gives us the opportunity to trim out things like this. It will be much harder to remove support for it once we add it to Gerrit.
Presubmit checks should be easy to change (atleast it was in Skia, not sure about the other projects).

While I'd love to get rid of TBR, this is extra friction to Gerrit adoption. I heard it frequently enough that I filed a bug, yet I certainly could be very biased - those who are fine with things as they are wouldn't ping me to say so :)

Anyways, I thought and still think that adding a simple check in js to set CR+1 seems much simpler than fixing all the presubmits and making everybody re-learn.
Besides, if we are to get rid of TBR, we definitely need to warn people to stop setting TBRs, and similar check would be needed anyway.

Agree with the extra friction to Gerrit adoption. It was probably easier to pull it off in Skia because there are smaller set of committers and contributors.

I think the best solution is for git-cl to detect TBR= lines in commit messages, remove them from the message, set the addresses on the line as reviewers, and set the CR+1 label. I don't think this should be done in JS, since then it *looks* like we still support it in gerrit. Instead, do it in git-cl.

Aaron, git cl *already* does all that except that it doesn't remove it from description, because that's what presubmit is checking.

My point here is that people do sometimes edit description and add TBR by habit, and then they forget that they also need to do CR+1 and wait for presubmit to fail to tell them that. So, what I'm suggesting to is to help these people do the right thing.

I am supportive of the same function to also remove TBR from description and add to reviewers instead so long as presubmit scripts are fixed too.

Does this make sense?  

Ahhh, I wasn't thinking about the "edit description on the web to add TBR=" use case.

Yeah, what you're saying makes sense to me then.

 Issue 666764  has been merged into this issue.

My plan is to display a prompt when a 'TBR=' is added with no CR+1. Something like:
You have added an "TBR=". In Gerrit, you have to also CR+1 the change for the CQ to land it.

Another option would have been to automatically CR+1 when 'TBR=' is detected but I would rather not automagically set such labels triggered by text in the commit message.

+1 for prompt. I'm against automatic too, because I really want to deprecate TBR= in the description :) 

Sent out for review-

* https://gerrit-review.googlesource.com/c/92513/: https://critique.corp.google.com/#review/140878468.
* cl/140878468 : Use the new commitmsgchange event to warn when TBR= is set without approval.

Submitted https://gerrit-review.googlesource.com/c/92513/ : Add hook for notifying when the commit message changes.

Submitted cl/140878468 : Use the new commitmsgchange event to warn when TBR= is set without approval.

The fix is live. Screenshot is attached.

Deleted: Screen Shot 2017-01-09 at 10.54.31 AM.png 50.0 KB

	Screen Shot 2017-01-09 at 10.54.31 AM.png 50.0 KB View Download

Thank you.