RFC: Streamlining TBR in Gerrit

[Aaron Gable]

Hey folks,

I have a proposal to change the way that developers specify TBR on changes. I think it's a win all around: developers keep track of less state, tools get simpler, and our ability to audit TBR'd changes increases.

Basically, I want to replace this:

$ git cl upload --tbrs=someone

<land change>

$ git show origin/master | tail

TBR=someone

Reviewed-by: mys...@chromium.org

with this:

$ git cl upload -r someone --tbr

<land change>

$ git show origin/master | tail

To-be-reviewed-by: som...@chromium.org

Please take a look at the proposal doc for more details.

Thanks,

Aaron

[Daniel Bratell]

The part mentioned seems like an overall improvement, but it looks like you want to increase the magic triggered by a self review. Most people (I sincerely hope) never use TBR so it can't be too magic once you need it to quickly land something that will unblock work.

Today entering "TBR=someone" in the commit message will trigger a self-review and the change will be ready to submit. You can learn about TBR= from watching other commits, but I'm not sure how someone is supposed to know that they have to self review a change if they've never done it before.

/Daniel

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAH58R2fJLPQVCbWv_cBvXFwf%2B%2BaOtdjMaTvDLeM-4Wa5%2BWHozg%40mail.gmail.com.

--

/* Opera Software, Linköping, Sweden: CET (UTC+1) */

[Colin Blundell]

Hi Aaron,

From the POV of someone who exclusively uses the WebUI to TBR, am I correct that the workflow change you're proposing is the following?

CURRENT WORKFLOW: Add X to reviewers, add TBR=X to commit message and commit.

PROPOSED WORKFLOW: Add X to reviewers, self-approve the CL and commit, tool adds TBR=X to commit message via analysis.

If I'm correct, I personally feel that it's somewhat strange and dangerous to give this much weight to self-approval. No one (I assume) writes "TBR=foo" by accident, but it's totally plausible to me to imagine self-approving by accident. I also feel that the semantics of writing "TBR=foo" are crystal-clear, whereas the semantics of self-approval are kind of murky (kind of in the sense of "why do we even have that lever?"). 

Thanks,

Colin

--
--
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/op.ze5hg6w6rbppqq%40cicero2.linkoping.osa.

[Aaron Gable]

On Wed, Feb 28, 2018 at 6:35 AM Daniel Bratell <bra...@opera.com> wrote:

The part mentioned seems like an overall improvement, but it looks like you want to increase the magic triggered by a self review. Most people (I sincerely hope) never use TBR so it can't be too magic once you need it to quickly land something that will unblock work.

Today entering "TBR=someone" in the commit message will trigger a self-review and the change will be ready to submit. You can learn about TBR= from watching other commits, but I'm not sure how someone is supposed to know that they have to self review a change if they've never done it before.

In Gerrit, it is already the case that someone TBRing a change must self-approve it. If they don't, the Submit to CQ button doesn't even show up, and is replaced by the quick-action Code-Review+1 button, which will self-approve the change. So this isn't adding another unintuitive step to their process, and folks who do TBR changes already know that they need to self-approve them.

You (and some people commenting on the doc) do raise a good point that folks are accustomed to having the tools automatically set CR+1 when they have TBR=someone in the commit description. So I'm going to slightly change the proposal:

Rather than removing all ability of folks to use "git cl upload --tbrs=someone" and put "TBR=someone" in the commit description, I'll simply unify the processing of those with the current processing of normal reviewers. If someone uses those to indicate that their CL should be TBRed, it will automatically be self-approved, but the TBR= line will be stripped from the commit message (just like the R= line already is) and the list of reviewers specified by TBR= and --tbrs will be merged with those specified as normal reviewers.

On Wed, Feb 28, 2018 at 7:10 AM Colin Blundell <blun...@chromium.org> wrote:

Hi Aaron,

From the POV of someone who exclusively uses the WebUI to TBR, am I correct that the workflow change you're proposing is the following?

CURRENT WORKFLOW: Add X to reviewers, add TBR=X to commit message and commit.

PROPOSED WORKFLOW: Add X to reviewers, self-approve the CL and commit, tool adds TBR=X to commit message via analysis.

If I'm correct, I personally feel that it's somewhat strange and dangerous to give this much weight to self-approval. No one (I assume) writes "TBR=foo" by accident, but it's totally plausible to me to imagine self-approving by accident. I also feel that the semantics of writing "TBR=foo" are crystal-clear, whereas the semantics of self-approval are kind of murky (kind of in the sense of "why do we even have that lever?"). 

You're almost correct, but not quite. The current workflow is "Add X to reviewers, add TBR=X (or anything really, doesn't have to correspond unfortunately) to commit message, self-approve the CL, and commit". As I mentioned above, Gerrit already requires self-approval in order for TBR changes to land. So this isn't adding any new requirements, it's just removing the double-bookeeping of self-approval and TBR= lines.

Aaron

 

[Colin Blundell]

Hey Aaron,

One quick reply inline:

I use TBR pretty frequently (for trivial client-side changes in response to API changes), and I don't explicitly self-approve my CLs. Is that because of what you wrote above about "the tools automatically self-approve"? In any case, my workflow in practice for this case does not include explicitly self-approving the CL. I do sometimes notice that I need to go in and explicitly set the CQ button in "Reply" rather than having the "Submit to CQ" button pop up, but I had never understood the logic of when that "Submit to CQ" button pops up, so didn't associate it with anything about needing to self-approve.

To be explicit: I TBR changes and didn't know that I needed to self-approve them ;).

[Aaron Gable]

Yes, this is because the tools (specifically, "git cl upload") automatically self-approve the change for you when you specify --tbrs= on the command line or have TBR= in the commit description. This proposal doesn't change that: if the developer expresses intent to TBR (either via a TBR= line in the commit description, the --tbrs= flag on the cli, or the newly proposed --tbr boolean flag on the cli), then "git cl upload" will automatically self-approve the change. The salient point of difference is that if you use TBR= in the commit description, then that line will be stripped from the description because it is redundant with that automatic self-approval, just like an R= line is stripped today.

[Sylvain Defresne]

On Wed, Feb 28, 2018 at 6:02 PM, Aaron Gable <aga...@chromium.org> wrote:

On Wed, Feb 28, 2018 at 6:35 AM Daniel Bratell <bra...@opera.com> wrote:

The part mentioned seems like an overall improvement, but it looks like you want to increase the magic triggered by a self review. Most people (I sincerely hope) never use TBR so it can't be too magic once you need it to quickly land something that will unblock work.

Today entering "TBR=someone" in the commit message will trigger a self-review and the change will be ready to submit. You can learn about TBR= from watching other commits, but I'm not sure how someone is supposed to know that they have to self review a change if they've never done it before.

In Gerrit, it is already the case that someone TBRing a change must self-approve it. If they don't, the Submit to CQ button doesn't even show up, and is replaced by the quick-action Code-Review+1 button, which will self-approve the change. So this isn't adding another unintuitive step to their process, and folks who do TBR changes already know that they need to self-approve them.

This does not match my experience. AFAICT, I see "Submit to CQ" button as soon as my CL received any +1 comment. This does not need to be a self-approved, and does not need to cover all OWNERS.

This is why I find the proposal confusing. My most frequent use case for TBR-ing a CL is that I do some refactoring of a class, get approval from one OWNER of the refactored class, and then TBR OWNERS of the affected client code if the changes are mechanical/trivial. In that case, I would see the "Submit to CQ" button. Currently I would just add TBR=... and then send to CQ. I don't know what I would have to do after the proposal.

 

You (and some people commenting on the doc) do raise a good point that folks are accustomed to having the tools automatically set CR+1 when they have TBR=someone in the commit description. So I'm going to slightly change the proposal:

Rather than removing all ability of folks to use "git cl upload --tbrs=someone" and put "TBR=someone" in the commit description, I'll simply unify the processing of those with the current processing of normal reviewers. If someone uses those to indicate that their CL should be TBRed, it will automatically be self-approved, but the TBR= line will be stripped from the commit message (just like the R= line already is) and the list of reviewers specified by TBR= and --tbrs will be merged with those specified as normal reviewers.

On Wed, Feb 28, 2018 at 7:10 AM Colin Blundell <blun...@chromium.org> wrote:

Hi Aaron,

From the POV of someone who exclusively uses the WebUI to TBR, am I correct that the workflow change you're proposing is the following?

CURRENT WORKFLOW: Add X to reviewers, add TBR=X to commit message and commit.

PROPOSED WORKFLOW: Add X to reviewers, self-approve the CL and commit, tool adds TBR=X to commit message via analysis.

If I'm correct, I personally feel that it's somewhat strange and dangerous to give this much weight to self-approval. No one (I assume) writes "TBR=foo" by accident, but it's totally plausible to me to imagine self-approving by accident. I also feel that the semantics of writing "TBR=foo" are crystal-clear, whereas the semantics of self-approval are kind of murky (kind of in the sense of "why do we even have that lever?"). 

You're almost correct, but not quite. The current workflow is "Add X to reviewers, add TBR=X (or anything really, doesn't have to correspond unfortunately) to commit message, self-approve the CL, and commit". As I mentioned above, Gerrit already requires self-approval in order for TBR changes to land. So this isn't adding any new requirements, it's just removing the double-bookeeping of self-approval and TBR= lines.

Aaron

 

--

--
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/CAH58R2fZxMyoN6%2BrbcA7wE6A-fnP7nfi2PC8Eg5xd0Gjf8LqjA%40mail.gmail.com.

[Aaron Gable]

On Wed, Feb 28, 2018 at 9:14 AM Sylvain Defresne <sdef...@chromium.org> wrote:

On Wed, Feb 28, 2018 at 6:02 PM, Aaron Gable <aga...@chromium.org> wrote: 

In Gerrit, it is already the case that someone TBRing a change must self-approve it. If they don't, the Submit to CQ button doesn't even show up, and is replaced by the quick-action Code-Review+1 button, which will self-approve the change. So this isn't adding another unintuitive step to their process, and folks who do TBR changes already know that they need to self-approve them.

This does not match my experience. AFAICT, I see "Submit to CQ" button as soon as my CL received any +1 comment. This does not need to be a self-approved, and does not need to cover all OWNERS.

This is why I find the proposal confusing. My most frequent use case for TBR-ing a CL is that I do some refactoring of a class, get approval from one OWNER of the refactored class, and then TBR OWNERS of the affected client code if the changes are mechanical/trivial. In that case, I would see the "Submit to CQ" button. Currently I would just add TBR=... and then send to CQ. I don't know what I would have to do after the proposal.

 Ah, I see. Yes, this is a case that this proposal handles differently, thanks for bringing it up explicitly.

Your description of what you would do under the current system is missing a step. Today, you would get approval from some reviewers, add TBR=... to the commit description, get a popup telling you that doing so is insufficient and you must also self-approve the change, do so, and then send to the CQ.

Under the proposed system, we remove that extra step in the middle. You would get approval from some owners, self-approve the change, and then submit the change to the CQ.

Independent of this proposal, we're already making changes to make this easier; in particular, we're ensuring that the quick-action "Code-Review+1" button shows up to everyone who hasn't approved the change yet, no matter whether or not other reviewers have approved it already.

Since you brought it up, I think it makes sense to make it even easier. When the change has been approved by some but not all reviewers, we could give change uploaders a "TBR and Submit to CQ" button next to the normal "Submit to CQ" button, which would self-approve and then submit the change. Would that assuage your concerns about not knowing what to do in this situation?

[Marijn Kruisselbrink]

On Wed, Feb 28, 2018 at 9:33 AM, Aaron Gable <aga...@chromium.org> wrote:

On Wed, Feb 28, 2018 at 9:14 AM Sylvain Defresne <sdef...@chromium.org> wrote:

On Wed, Feb 28, 2018 at 6:02 PM, Aaron Gable <aga...@chromium.org> wrote: 

In Gerrit, it is already the case that someone TBRing a change must self-approve it. If they don't, the Submit to CQ button doesn't even show up, and is replaced by the quick-action Code-Review+1 button, which will self-approve the change. So this isn't adding another unintuitive step to their process, and folks who do TBR changes already know that they need to self-approve them.

This does not match my experience. AFAICT, I see "Submit to CQ" button as soon as my CL received any +1 comment. This does not need to be a self-approved, and does not need to cover all OWNERS.

This is why I find the proposal confusing. My most frequent use case for TBR-ing a CL is that I do some refactoring of a class, get approval from one OWNER of the refactored class, and then TBR OWNERS of the affected client code if the changes are mechanical/trivial. In that case, I would see the "Submit to CQ" button. Currently I would just add TBR=... and then send to CQ. I don't know what I would have to do after the proposal.

 Ah, I see. Yes, this is a case that this proposal handles differently, thanks for bringing it up explicitly.

Your description of what you would do under the current system is missing a step. Today, you would get approval from some reviewers, add TBR=... to the commit description, get a popup telling you that doing so is insufficient and you must also self-approve the change, do so, and then send to the CQ.

I don't think that is true. There are plenty of TBR'd commits that don't have any kind of self-approval. Just take a look at git log and some random sampling of commits with TBR=, many of those don't have a Reviewed-by of the author/committer.

 

Under the proposed system, we remove that extra step in the middle. You would get approval from some owners, self-approve the change, and then submit the change to the CQ.

Independent of this proposal, we're already making changes to make this easier; in particular, we're ensuring that the quick-action "Code-Review+1" button shows up to everyone who hasn't approved the change yet, no matter whether or not other reviewers have approved it already.

Since you brought it up, I think it makes sense to make it even easier. When the change has been approved by some but not all reviewers, we could give change uploaders a "TBR and Submit to CQ" button next to the normal "Submit to CQ" button, which would self-approve and then submit the change. Would that assuage your concerns about not knowing what to do in this situation?

--

--
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/CAH58R2chdbvhpW%3D26m4c9jqzA5O48C-Pg7PFwWQT8PNN0XaPbw%40mail.gmail.com.

[Aaron Gable]

Augh, I'm sorry, you're totally right. I got myself thinking about two different scenarios and mixed myself up. The popup telling you that TBR is insufficient and you must self-approve only shows up when you don't already have at least one approval from someone else. My apologies.

That said, I think this highlights the ways in which the current system is confusing. The TBR= line doesn't actually keep track of who needs to approve a change, just that someone does. And whether or not it tells you that you need to self-approve depends on the order in which events happen, regardless of the overall state of the CL immediately before it is submitted.

This proposal simplifies that: if you want to land your change while missing approval from certain people, self-approve it. That's it.

[Scott Graham]

The one thing that I've noticed happens quite often is that people confuse CQ+1 with CR+1. To avoid accidental TBR, I think it would be good to keep some explicit, separate notation that indicates it's being TBRd.

(Alternatively, could we change the score buttons from two close-to-each-other-+1s and a +2 to "LGTM", "DRY RUN", and "SUBMIT TO CQ"?)

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

[Colin Blundell]

Hi Aaron,

As long as this proposal preserves the workflow that I can add "TBR=foo" to the commit message in the WebUI and then hit "CQ+2", my main concern is withdrawn.

Thanks,

Colin

[Aaron Gable]

Colin: That's explicitly the one workflow that would no longer be supported. I'd like to talk more about why that particular workflow is good for you, and what the workflow of "add people to the reviewers list and self-approve" would be lacking for you. Can we chat, or take this discussion offline?

Scott: Yep, I've added some updates to the doc about buttons and messages which would reduce confusion and makes clear that/when the change is being TBR'd.

[Colin Blundell]

Aaron and I synced up offline. For my workflow, the change proposed is as follows:

Current:

1. Add foo to reviewer list

2. Add TBR=foo to cl description in WebUI

3. Send message saying "TBR=foo for trivial changes to //chrome" or whatever

4. Hit CQ+2

In the new world (changes bolded):

1. Add foo to reviewer list

2. Self-approve

3. Send message saying "TBR=foo for trivial changes to //chrome" or whatever

4. Hit CQ+2

5. Tool adds TBR=foo to the CL description

I have a slight preference for the current as I find it to be more explicit, but it's certainly not a big deal.

Best,

Colin

[dmaz...@google.com]

There are three cases I use TBR for:
1. Urgent revert or fix to a build or test
2. Change to documentation or README files only
3. Large change got review from several people already,
but TBRing the owner of some directory with a trivial change

I think #1 and #2 are fine - self-review makes total sense for
these.

I'm not sure about #3. I think self-review is a more confusing
way to think about it than TBR, but that's just my opinion.

I'm most concerned about this case:

> The one thing that I've noticed happens quite often is that people confuse CQ+1 with CR+1

I'm seeing this happen all the time. Is there anything we could do to
make it less confusing?

If you're the owner, what if the CR+1 button was replaced with a
"Self-Approve" button? That'd be harder to press by accident!

[Aaron Gable]

On Fri, Mar 9, 2018 at 9:54 AM <dmaz...@google.com> wrote:

There are three cases I use TBR for:
1. Urgent revert or fix to a build or test
2. Change to documentation or README files only
3. Large change got review from several people already,
   but TBRing the owner of some directory with a trivial change

I think #1 and #2 are fine - self-review makes total sense for
these.

I'm not sure about #3. I think self-review is a more confusing
way to think about it than TBR, but that's just my opinion.

Yep, I acknowledge that this may be slightly less intuitive, but I think that is offset by two things:

1) I have plans (see also below) to push user education and have good affordances in the UI

2) I think that having a single mechanism for bypassing approval is good; we shouldn't have different mechanisms depending on why you're bypassing approval.

 

I'm most concerned about this case:

> The one thing that I've noticed happens quite often is that people confuse CQ+1 with CR+1

I'm seeing this happen all the time. Is there anything we could do to
make it less confusing?

If you're the owner, what if the CR+1 button was replaced with a
"Self-Approve" button? That'd be harder to press by accident!

Yep, I've already mentioned this a little bit upthread, and I've updated the doc to contain some of these ideas, but I definitely plan to change the buttons which are visible to folks. We can replace CR+1 with self-approve for change owners, we can replace "Submit to CQ" with "TBR and Submit to CQ" when someone already has self-approved, and more. 

[Dirk Pranke]

On Fri, Mar 9, 2018 at 11:19 AM, Aaron Gable <aga...@chromium.org> wrote:

On Fri, Mar 9, 2018 at 9:54 AM <dmaz...@google.com> wrote:

There are three cases I use TBR for:
1. Urgent revert or fix to a build or test
2. Change to documentation or README files only
3. Large change got review from several people already,
   but TBRing the owner of some directory with a trivial change

I think #1 and #2 are fine - self-review makes total sense for
these.

I'm not sure about #3. I think self-review is a more confusing
way to think about it than TBR, but that's just my opinion.

Yep, I acknowledge that this may be slightly less intuitive, but I think that is offset by two things:

1) I have plans (see also below) to push user education and have good affordances in the UI

2) I think that having a single mechanism for bypassing approval is good; we shouldn't have different mechanisms depending on why you're bypassing approval.

2) is not obviously true to me, but I think it depends on the user experience and details of what you mean. It is important to capture the intent (the why) somewhere, and to make sure the different intents can't be easily confused.

-- Dirk

 

I'm most concerned about this case:

> The one thing that I've noticed happens quite often is that people confuse CQ+1 with CR+1

I'm seeing this happen all the time. Is there anything we could do to
make it less confusing?

If you're the owner, what if the CR+1 button was replaced with a
"Self-Approve" button? That'd be harder to press by accident!

Yep, I've already mentioned this a little bit upthread, and I've updated the doc to contain some of these ideas, but I definitely plan to change the buttons which are visible to folks. We can replace CR+1 with self-approve for change owners, we can replace "Submit to CQ" with "TBR and Submit to CQ" when someone already has self-approved, and more. 

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.

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

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAH58R2fg-5AUUMKVRDAHWkYKZQinwSSPEHuRP6A8ttBm%3DJk3%3DQ%40mail.gmail.com.

[Michael Giuffrida]

On the subject of confusion: The "Reviewed-by" commit description tag is different from the "reviewedby" Gerrit field. Is there some glossary for terms like "Reviewed-by" that get appended to commit descriptions?

On Friday, March 9, 2018 at 11:20:34 AM UTC-8, agable wrote: