PSA: Planning to add new 'Commit after a reviewer gives LGTM' checkbox in Rietveld

[rmi...@chromium.org]

I have been working on a Rietveld feature that adds a new checkbox above the 'Commit' checkbox.

The new checkbox is called 'Commit after a reviewer gives LGTM'.

This is what it looks like:

​​

Staged here.

The flow and design has been improved from my original attempt after lots of feedback from reed@, jrobbins@ and iannucci@. Design discussions are in this CL.

Motivation

Chromium/Blink/Skia developers are geographically distributed and span multiple time zones.

A developer on the east coast might create a CL and send it to a developer in UK during the day EST time. The UK developer LGTMs late night EST time, then in the morning the east coast developer checks the CQ bit. After this it may take several hours to land because trybots do take a while and/or the tree could be closed.

For some CLs it would be useful to have a way of telling Rietveld to flip the CQ bit automatically after a LGTM.

The Flow

* Check the new auto LGTM checkbox.

* When there is an LGTM from a reviewer the CQ checkbox will be automatically checked. A 'not LGTM' will uncheck the CQ checkbox. The last approval/disapproval is the one that is used.

* When a new patchset is added or the issue is closed the new auto LGTM checkbox is unchecked (similar to the CQ checkbox).

* The email address of the developer is logged in the CL's msgs section when the new checkbox is flipped (similar to the CQ checkbox).

* The original CQ checkbox works as before and developers who do not like or want to use the new checkbox do not have to.

Future Improvements

* This feature currently checks the CQ checkbox after a single LGTM. Saying 'LGTM with nits' would still flip the CQ bit. We handle this somewhat by displaying a warning msg on the comments publish page.

Soon I would like to work on a way of specifying AND/OR reviewers in Rietveld (with the corresponding check in the CQ). You will then be able to say 'xyz OR abc', an LGTM from either one will flip the CQ bit and the CQ would also check for this. 'xyz AND abc' would check for an LGTM from both.

* dpranke@ has recommended using a flow similar to Webkit's model (dropdown/select with empty/cq-/cq?/cq+). This may become the next evolution of the two checkbox flow.

Any comments/feedback will be appreciated.

If this looks good, then I will submit and deploy it in a few days.

Thanks,

Ravi

[rmi...@chromium.org]

Looks like the checkbox image did not attach correctly.

Trying again:

[Nico Weber]

This sounds risky. Do you have any stats on which percentage of reviews have just a single lgtm?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Colin Blundell]

Interesting, thanks!

A couple questions:

- If I'm not misunderstanding, this won't take into account OWNERS in its initial version? It seems like it might result in a lot of spam in that case.

- The trying to commit when the reviewer has some comments to address behavior seems unfortunate. In fact, it seems like it has the potential to decrease the likelihood of reviewers giving "LGTM with nits", if they fear/know that the CL will get auto-committed without taking their comments into account. Is there no way to address that as part of V1?

Thanks,

Colin

--

[rmi...@chromium.org]

On Monday, March 17, 2014 8:20:28 AM UTC-4, Colin Blundell wrote:

Interesting, thanks!

A couple questions:

- If I'm not misunderstanding, this won't take into account OWNERS in its initial version? It seems like it might result in a lot of spam in that case.

Right, OWNERS are not taken into account. I am not sure it would result in more spam as compared to the current behavior with the single checkbox. Could you give an example of your concern?

 

- The trying to commit when the reviewer has some comments to address behavior seems unfortunate. In fact, it seems like it has the potential to decrease the likelihood of reviewers giving "LGTM with nits", if they fear/know that the CL will get auto-committed without taking their comments into account. Is there no way to address that as part of V1?

We tried to address this with the following:

* On the comment publish page the following msg is displayed (if the new checkbox is checked): "Note: The 'Commit after a reviewer gives LGTM' bit is turned on for this CL. Saying 'LGTM with nits' will send it to the CQ."

* If a reviewer wants to say "LGTM with nits" then he/she can first uncheck the new checkbox.

[rmi...@chromium.org]

On Monday, March 17, 2014 8:18:11 AM UTC-4, Nico Weber wrote:

This sounds risky. Do you have any stats on which percentage of reviews have just a single lgtm?

I do not have these stats, not sure how to go about getting it from Rietveld's datastore.

Could you explain why this might be risky? Also, the original 'Commit' checkbox's behavior is unchanged, the new checkbox can be used when a single LGTM is deemed sufficient.

[Anthony Berent]

As someone who frequently is looking for reviewers in other timezones (mainly looking for MTV reviewers from LON) this sounds great. I, at least, would be unlikely to use it for larger reviews, but there is a level where the CL is too complex for TBR, yet it is urgent and probably correct and only needs a single review.

This could also be useful in cases where the main review is complete, but there are just one or two files that need owner approval.

[Colin Blundell]

On Mon, Mar 17, 2014 at 1:35 PM, <rmi...@chromium.org> wrote:

On Monday, March 17, 2014 8:20:28 AM UTC-4, Colin Blundell wrote:

Interesting, thanks!

A couple questions:

- If I'm not misunderstanding, this won't take into account OWNERS in its initial version? It seems like it might result in a lot of spam in that case.

Right, OWNERS are not taken into account. I am not sure it would result in more spam as compared to the current behavior with the single checkbox. Could you give an example of your concern?

 

As long as engineers didn't use it until they were waiting for the final LGTM they need for a CL, you're right, it shouldn't result in more spam. Are you confident that engineers will understand that that would be the correct way to use this checkbox? My initial assumption on seeing such a checkbox would be that its semantics would be "Commit once all necessary LGTMs are obtained to pass presubmit checks." That might just be me though :).

 

- The trying to commit when the reviewer has some comments to address behavior seems unfortunate. In fact, it seems like it has the potential to decrease the likelihood of reviewers giving "LGTM with nits", if they fear/know that the CL will get auto-committed without taking their comments into account. Is there no way to address that as part of V1?

We tried to address this with the following:

* On the comment publish page the following msg is displayed (if the new checkbox is checked): "Note: The 'Commit after a reviewer gives LGTM' bit is turned on for this CL. Saying 'LGTM with nits' will send it to the CQ."

* If a reviewer wants to say "LGTM with nits" then he/she can first uncheck the new checkbox.

That sounds good. I'm afraid that that specific language will just incline reviewers to not say LGTM there, which seems counterproductive. Maybe something like "Please uncheck the box if you have comments you want addressed with your LGTM" or something like that?

[Bartosz Fabianowski]

I agree that for those of us working across multiple timezones, automatic commit after LGTM is a great feature. But I am worried about it ignoring nits. Why not make it so that only a straight "LGTM" with no further comments kicks off the CQ? If there are any nits, do *not* auto-commit and allow the CL author to fix them first. This would seem like the desired behavior in 99% of cases.

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

--

Google Germany GmbH

Dienerstr. 12

80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

[rmi...@chromium.org]

On Monday, March 17, 2014 8:52:31 AM UTC-4, Colin Blundell wrote:

On Mon, Mar 17, 2014 at 1:35 PM, <rmi...@chromium.org> wrote:

On Monday, March 17, 2014 8:20:28 AM UTC-4, Colin Blundell wrote:

Interesting, thanks!

A couple questions:

- If I'm not misunderstanding, this won't take into account OWNERS in its initial version? It seems like it might result in a lot of spam in that case.

Right, OWNERS are not taken into account. I am not sure it would result in more spam as compared to the current behavior with the single checkbox. Could you give an example of your concern?

 

As long as engineers didn't use it until they were waiting for the final LGTM they need for a CL, you're right, it shouldn't result in more spam. Are you confident that engineers will understand that that would be the correct way to use this checkbox? My initial assumption on seeing such a checkbox would be that its semantics would be "Commit once all necessary LGTMs are obtained to pass presubmit checks." That might just be me though :).

Maybe the text of the checkbox needs to be more descriptive.

How about something like "Commit after an LGTM from any reviewer" ?

 

 

- The trying to commit when the reviewer has some comments to address behavior seems unfortunate. In fact, it seems like it has the potential to decrease the likelihood of reviewers giving "LGTM with nits", if they fear/know that the CL will get auto-committed without taking their comments into account. Is there no way to address that as part of V1?

We tried to address this with the following:

* On the comment publish page the following msg is displayed (if the new checkbox is checked): "Note: The 'Commit after a reviewer gives LGTM' bit is turned on for this CL. Saying 'LGTM with nits' will send it to the CQ."

* If a reviewer wants to say "LGTM with nits" then he/she can first uncheck the new checkbox.

That sounds good. I'm afraid that that specific language will just incline reviewers to not say LGTM there, which seems counterproductive. Maybe something like "Please uncheck the box if you have comments you want addressed with your LGTM" or something like that?

That SGTM. I will update the text.

[rmi...@chromium.org]

On Monday, March 17, 2014 8:53:34 AM UTC-4, Bartosz Fabianowski wrote:

I agree that for those of us working across multiple timezones, automatic commit after LGTM is a great feature. But I am worried about it ignoring nits. Why not make it so that only a straight "LGTM" with no further comments kicks off the CQ? If there are any nits, do *not* auto-commit and allow the CL author to fix them first. This would seem like the desired behavior in 99% of cases.

I fear that might complicate things (both in the Rietveld code and the flow). Reviewers will not be able to say "LGTM thanks for working on this!". Hopefully Colin's suggestions will help make things more clear for reviewers when the new checkbox is checked.

[Bartosz Fabianowski]

On 03/17/2014 02:03 PM, rmi...@chromium.org wrote:
>
>
> On Monday, March 17, 2014 8:53:34 AM UTC-4, Bartosz Fabianowski wrote:
>>
>> I agree that for those of us working across multiple timezones, automatic
>> commit after LGTM is a great feature. But I am worried about it ignoring
>> nits. Why not make it so that only a straight "LGTM" with no further
>> comments kicks off the CQ? If there are any nits, do *not* auto-commit and
>> allow the CL author to fix them first. This would seem like the desired
>> behavior in 99% of cases.
>>
>
> I fear that might complicate things (both in the Rietveld code and the
> flow). Reviewers will not be able to say "LGTM thanks for working on
> this!". Hopefully Colin's suggestions will help make things more clear for
> reviewers when the new checkbox is checked.

I did mean that additional text in the reply body should stop the CQ.
Additional in-line comments (the ones you get when you double-click
anywhere in the code) should stop the CQ IMHO.

>
>
>>
>>
>> On 17 March 2014 13:47, Anthony Berent <abe...@chromium.org <javascript:>>wrote:
>>
>>> As someone who frequently is looking for reviewers in other timezones
>>> (mainly looking for MTV reviewers from LON) this sounds great. I, at least,
>>> would be unlikely to use it for larger reviews, but there is a level where
>>> the CL is too complex for TBR, yet it is urgent and probably correct and
>>> only needs a single review.
>>>
>>> This could also be useful in cases where the main review is complete, but
>>> there are just one or two files that need owner approval.
>>>
>>>
>>> On Mon, Mar 17, 2014 at 12:38 PM, <rmi...@chromium.org <javascript:>>wrote:
>>>
>>>>
>>>>
>>>> On Monday, March 17, 2014 8:18:11 AM UTC-4, Nico Weber wrote:
>>>>>
>>>>> This sounds risky. Do you have any stats on which percentage of reviews
>>>>> have just a single lgtm?
>>>>>
>>>>
>>>> I do not have these stats, not sure how to go about getting it from
>>>> Rietveld's datastore.
>>>> Could you explain why this might be risky? Also, the original 'Commit'
>>>> checkbox's behavior is unchanged, the new checkbox can be used when a
>>>> single LGTM is deemed sufficient.
>>>>
>>>>
>>>>>
>>>>>
>>>>> On Mon, Mar 17, 2014 at 12:05 PM, <rmi...@chromium.org> wrote:
>>>>>
>>>>>>
>>>>>> I have been working on a Rietveld feature that adds a new checkbox
>>>>>> above the 'Commit' checkbox.
>>>>>> The new checkbox is called 'Commit after a reviewer gives LGTM'.
>>>>>>
>>>>>> This is what it looks like:
>>>>>>
>>>>>> ​​
>>>>>>

>>>>>> Staged here <https://skia-codereview-staging2.appspot.com/490001/>.

>>>>>>
>>>>>> The flow and design has been improved from my original attempt after
>>>>>> lots of feedback from reed@, jrobbins@ and iannucci@. Design discussions

>>>>>> are in this CL <https://codereview.appspot.com/66340044/>.
>>>>>>
>>>>>>
>>>>>> *Motivation*

>>>>>>
>>>>>> Chromium/Blink/Skia developers are geographically distributed and span
>>>>>> multiple time zones.
>>>>>> A developer on the east coast might create a CL and send it to a
>>>>>> developer in UK during the day EST time. The UK developer LGTMs late night
>>>>>> EST time, then in the morning the east coast developer checks the CQ bit.
>>>>>> After this it may take several hours to land because trybots do take a
>>>>>> while and/or the tree could be closed.
>>>>>> For some CLs it would be useful to have a way of telling Rietveld to
>>>>>> flip the CQ bit automatically after a LGTM.
>>>>>>
>>>>>>

>>>>>> *The Flow*

>>>>>>
>>>>>> * Check the new auto LGTM checkbox.
>>>>>> * When there is an LGTM from a reviewer the CQ checkbox will be
>>>>>> automatically checked. A 'not LGTM' will uncheck the CQ checkbox. The last
>>>>>> approval/disapproval is the one that is used.
>>>>>> * When a new patchset is added or the issue is closed the new auto
>>>>>> LGTM checkbox is unchecked (similar to the CQ checkbox).
>>>>>> * The email address of the developer is logged in the CL's msgs
>>>>>> section when the new checkbox is flipped (similar to the CQ checkbox).
>>>>>> * The original CQ checkbox works as before and developers who do not
>>>>>> like or want to use the new checkbox do not have to.
>>>>>>
>>>>>>

>>>>>> *Future Improvements*

>>>>>>
>>>>>> * This feature currently checks the CQ checkbox after a single LGTM.
>>>>>> Saying 'LGTM with nits' would still flip the CQ bit. We handle this
>>>>>> somewhat by displaying a warning msg on the comments publish page.
>>>>>> Soon I would like to work on a way of specifying AND/OR reviewers in
>>>>>> Rietveld (with the corresponding check in the CQ). You will then be able to
>>>>>> say 'xyz OR abc', an LGTM from either one will flip the CQ bit and the CQ
>>>>>> would also check for this. 'xyz AND abc' would check for an LGTM from both.
>>>>>> * dpranke@ has recommended using a flow similar to Webkit's model
>>>>>> (dropdown/select with empty/cq-/cq?/cq+). This may become the next
>>>>>> evolution of the two checkbox flow.
>>>>>>
>>>>>>
>>>>>> Any comments/feedback will be appreciated.
>>>>>> If this looks good, then I will submit and deploy it in a few days.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Ravi
>>>>>>
>>>>>> --
>>>>>> --
>>>>>> Chromium Developers mailing list: chromi...@chromium.org
>>>>>>
>>>>>> View archives, change email options, or unsubscribe:
>>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>>
>>>>>
>>>>> --
>>>> --

>>>> Chromium Developers mailing list: chromi...@chromium.org <javascript:>

>>>> View archives, change email options, or unsubscribe:
>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>
>>>
>>> --
>>> --

>>> Chromium Developers mailing list: chromi...@chromium.org <javascript:>

>>> View archives, change email options, or unsubscribe:
>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>
>>> To unsubscribe from this group and stop receiving emails from it, send an

>>> email to chromium-dev...@chromium.org <javascript:>.

[rmi...@chromium.org]

Also, how about a new button on the publish page:

"LGTM with nits (do not send to CQ yet)"

This new button would automatically uncheck the new checkbox.

(this idea came from a discussion with reed@ a few minutes ago).

[Philipp Neubeck]

Isn't it already possible that reviewers with commit-rights can tick the commit checkbox?

If so, isn't a comment by the author like "Please commit after reviewing." sufficient or could you somehow consider this in your design?

This would at least work up to two or three reviewers.

Google Germany GmbH

Dienerstraße 12

80331 München

Deutschland

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Graham Law, Katherine Stephens

[rmi...@chromium.org]

On Monday, March 17, 2014 9:10:48 AM UTC-4, Bartosz Fabianowski wrote:

On 03/17/2014 02:03 PM, rmi...@chromium.org wrote:
>
>
> On Monday, March 17, 2014 8:53:34 AM UTC-4, Bartosz Fabianowski wrote:
>>
>> I agree that for those of us working across multiple timezones, automatic
>> commit after LGTM is a great feature. But I am worried about it ignoring
>> nits. Why not make it so that only a straight "LGTM" with no further
>> comments kicks off the CQ? If there are any nits, do *not* auto-commit and
>> allow the CL author to fix them first. This would seem like the desired
>> behavior in 99% of cases.
>>
>
> I fear that might complicate things (both in the Rietveld code and the
> flow). Reviewers will not be able to say "LGTM thanks for working on
> this!". Hopefully Colin's suggestions will help make things more clear for
> reviewers when the new checkbox is checked.

I did mean that additional text in the reply body should stop the CQ.
Additional in-line comments (the ones you get when you double-click
anywhere in the code) should stop the CQ IMHO.

I have similar concerns about this behavior as well (about complicating the flow). Comments in the code that say 'Agreed with your previous comments here' or 'We should use this same pattern in the xyz framework', maybe this is not commonly done but I know I have done this before.

[Bartosz Fabianowski]

On 03/17/2014 02:28 PM, rmi...@chromium.org wrote:
>
>
> On Monday, March 17, 2014 9:10:48 AM UTC-4, Bartosz Fabianowski wrote:
>>

>> On 03/17/2014 02:03 PM, rmi...@chromium.org <javascript:> wrote:
>>>
>>>
>>> On Monday, March 17, 2014 8:53:34 AM UTC-4, Bartosz Fabianowski wrote:
>>>>
>>>> I agree that for those of us working across multiple timezones,
>> automatic
>>>> commit after LGTM is a great feature. But I am worried about it
>> ignoring
>>>> nits. Why not make it so that only a straight "LGTM" with no further
>>>> comments kicks off the CQ? If there are any nits, do *not* auto-commit
>> and
>>>> allow the CL author to fix them first. This would seem like the desired
>>>> behavior in 99% of cases.
>>>>
>>>
>>> I fear that might complicate things (both in the Rietveld code and the
>>> flow). Reviewers will not be able to say "LGTM thanks for working on
>>> this!". Hopefully Colin's suggestions will help make things more clear
>> for
>>> reviewers when the new checkbox is checked.
>>
>> I did mean that additional text in the reply body should stop the CQ.
>> Additional in-line comments (the ones you get when you double-click
>> anywhere in the code) should stop the CQ IMHO.

s/I did mean/I did not mean/

>>
>
> I have similar concerns about this behavior as well (about complicating the
> flow). Comments in the code that say 'Agreed with your previous comments
> here' or 'We should use this same pattern in the xyz framework', maybe this
> is not commonly done but I know I have done this before.

The default behavior should depend on what is done more commonly then. I
know that when I add in-line comments, in 90%+ of the cases, I am
pointing out things that should be fixed before landing the CL.

[rmi...@chromium.org]

On Monday, March 17, 2014 9:20:42 AM UTC-4, Philipp Neubeck wrote:

Isn't it already possible that reviewers with commit-rights can tick the commit checkbox?

If so, isn't a comment by the author like "Please commit after reviewing." sufficient or could you somehow consider this in your design?

This would at least work up to two or three reviewers.

What we have seen with this is that it does not guarantee that the reviewer will check the CQ box (could happen if the reviewer missed the author's comment or forgot about it).

The AND/OR reviewer logic in my email, mentioned under "Future improvements", should be able to eventually handle the case of two or more reviewers.

[rmi...@chromium.org]

+iannucci

+jrobbins

Robbie/Jason, thoughts about this?

[Gabriel Charette]

FWIW, as someone that works in another timezone, whenever I need this feature today I simply send my reviewer a "PTAL (please CQ if it LGTYs)".

This is typically either for a single reviewer small change or a bigger review for which I ask this of the owner closing off on the review after LGTMs from others.

I've yet to see a reviewer fail to check the box when it looked good to them as-is and faced with this CQ request and I'm not convinced that we need more than this verbal contract where both parties currently understand all the subtleties which would be hard to put in an automated system (as discussed above).

Cheers!
Gab

[Mikhail Naganov]

I agree with Gabriel. Instead of introducing an automatic feature with unclear logic, it's much better to use human judgement here.

I also had experience with MTV reviewers, when, after doing an owner's review, the reviewer simply ticked the CQ checkbox himself, if he was comfortable with the change.

[Mike Reed]

I agree that human judgement is always needed. This proposal is a way to formalize requests and decisions a little better, not replace them.

Certainly a formal GUI for this should not be required, and so the verbal-contract approach can always be done. But esp. when several reviewers are listed (as when looking for owners), I think there is value in formalizing the request in the CL. It better documents the transaction, and makes it clearer to other reviewers. Also, at times the author does *not* want to place the change in the CQ, but is looking for revewers' approvals. Some version of this new UI makes that situation clearer as well.

I am in favor of this proposed mechanism. Authors that are comfortable with informal means can of course continue to use them.

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

[John Abd-El-Malek]

This sounds like it would clutter the UI more.. It seems that if we go with this checkbox approach, a reviewer with comments could uncheck it.

[Philippe Gervais]

Another idea:

since auto-committing seems risky in some cases, couldn't we replace this feature by a reminder for the reviewer(s). This way the original problem raised by rmistry (that reviewers may miss the unformal request of checking the box) would be avoided. 

Sending a reminder would increase the level of spam though, and displaying it on Rietveld itself is not enough.

     Philippe

[Mike Reed]

Hmmm. What is the risk of the auto-adding-to-CQ, over what we have today? Perhaps we can fix that risk, and still allow some formalization of this pattern.

[Dominic Mazzoni]

This would be my preference, too - allow the author to set a "please cq if ready" flag.

At the very least, I suggest not enabling this feature as proposed until it could at least do owners checks.

[Philippe Gervais]

I was talking about the 'lgtm with nits' pattern, which is awkward. If the auto-CQ box is checked the reviewer has to remind themselves to uncheck the box, which can lead to CL being committed unexpectedly (and a extra mental load). So the question boils down to what is the most risky: committing or not committing?

For my part, I think committing something unintended is the most risky, others may disagree.

[Robert Iannucci]

The other thing we could do is we could explicitly recognize 'lgtm.{1,10}nits' (catching "lgtm%nits", "lgtm w/ nits", "lgtm with nits", "lgtm+nits", etc.) and count it as 'lgtm but uncheck the auto-cq box if it's checked'... A bit icky, but given the 'interface' of the current lgtm system, this would fit.

[Steven Bennetts]

Gerrit has a nice feature where the patch owner has to check a 'Ready' box for the CQ to pick up the patch. I am not suggesting implementing Gerrit's +1/+2 system, but maybe we could just add a 'Ready' checkbox so that a reviewer can evaluate and check the 'Commit' box if the owner has checked 'Ready' (and maybe prevent a non-owner from checking 'Commit' unless the owner has checked 'Ready'?

[Thiago Farina]

On Mon, Mar 17, 2014 at 2:41 PM, Steven Bennetts <stev...@google.com> wrote:

Gerrit has a nice feature where the patch owner has to check a 'Ready' box for the CQ to pick up the patch.

I like this idea more.

 

I am not suggesting implementing Gerrit's +1/+2 system,

Interesting. It seems another discussion for improving rietveld though.

 

but maybe we could just add a 'Ready' checkbox so that a reviewer can evaluate and check the 'Commit' box if the owner has checked 'Ready' (and maybe prevent a non-owner from checking 'Commit' unless the owner has checked 'Ready'?

This is basically to say to reviewer that he can check the commit box? Isn't the owner responsible for checking that button himself?

In my case I never had any problems with reviewers in other timezones, well I think they are all in different timezones anyway. What happens in my case is that, some reviewers when happy with my patch they 'LGTM" the change and right after they check the commit box for me (probably while I'm asleep).

IMO, this feature "Commit after a reviewer gives LGTM' is already possible, the reviewer just needs to make a judgment, as I exemplified above, and check the commit box himself if he is already satifies with the current shape of the latest patch.

--

Thiago Farina

[John Abd-El-Malek]

It seems that a number of people have commented that they informally use the "please CQ if it looks good to you". I've gotten that as well at different times and I always do it.

Does anyone here use this informal request and have it fail? i.e. trying to figure out if we need to add extra UI/state/heuristics to Rietveld; it seems there are higher priority things to improve there than this.

[David Michael]

On Mon, Mar 17, 2014 at 12:03 PM, John Abd-El-Malek <j...@chromium.org> wrote:

It seems that a number of people have commented that they informally use the "please CQ if it looks good to you". I've gotten that as well at different times and I always do it.

Does anyone here use this informal request and have it fail? i.e. trying to figure out if we need to add extra UI/state/heuristics to Rietveld; it seems there are higher priority things to improve there than this.

+1; I think it should be in the reviewer's hands to decide if it's ready for CQ, and the submitter can just say if they want their reviewer to CQ. There's even a "Quick LGTM & CQ" button already for the reviewer.

On Mon, Mar 17, 2014 at 11:00 AM, Thiago Farina <tfa...@chromium.org> wrote:

On Mon, Mar 17, 2014 at 2:41 PM, Steven Bennetts <stev...@google.com> wrote:

Gerrit has a nice feature where the patch owner has to check a 'Ready' box for the CQ to pick up the patch.

I like this idea more.

 

I am not suggesting implementing Gerrit's +1/+2 system,

Interesting. It seems another discussion for improving rietveld though.

 

but maybe we could just add a 'Ready' checkbox so that a reviewer can evaluate and check the 'Commit' box if the owner has checked 'Ready' (and maybe prevent a non-owner from checking 'Commit' unless the owner has checked 'Ready'?

This is basically to say to reviewer that he can check the commit box? Isn't the owner responsible for checking that button himself?

In my case I never had any problems with reviewers in other timezones, well I think they are all in different timezones anyway. What happens in my case is that, some reviewers when happy with my patch they 'LGTM" the change and right after they check the commit box for me (probably while I'm asleep).

IMO, this feature "Commit after a reviewer gives LGTM' is already possible, the reviewer just needs to make a judgment, as I exemplified above, and check the commit box himself if he is already satifies with the current shape of the latest patch.

--

Thiago Farina

[Mike Reed]

I guess I have the opposite reaction:

- multiple people have chimed in with a similar use-pattern

- Gerrit has formalized this pattern (to good effect I suppose)

- the code is tested to implement this

- adding the option will not negate other informal techniques

However, I can survive w/o this, so I will not continue to advocate for it.

[Ilya Sherman]

On Mon, Mar 17, 2014 at 5:05 AM, <rmi...@chromium.org> wrote:

Future Improvements

* dpranke@ has recommended using a flow similar to Webkit's model (dropdown/select with empty/cq-/cq?/cq+). This may become the next evolution of the two checkbox flow.

IMO this UI worked pretty well for WebKit reviews, and avoids most of the problems that people are bringing up on this thread.  Can you make this improvement prior to launching the feature, rather than after?

[rmi...@chromium.org]

Sounds like there is no consensus on this feature. I am happy to abandon this change and help Jason and Robbie explore alternate approaches in Rietveld v2 (making big changes in Rietveld v1 right now will make transitioning it to v2 more difficult).

[Paweł Hajdan, Jr.]

FYI I've seen https://code.google.com/p/chromium/issues/detail?id=162819 filed for this.

Feel free to track that bug, and deduplicate any other bugs related to this.

Paweł

--

[Jochen Eisinger]

The feature I requested in that bug is different from "commit after lgtm". It's similar to WebKit's cq? - the reviewer still has to click the lgtm+cq button.

best

-jochen