PSA: Rietveld now has post-approval patchset notifications

91 views
Skip to first unread message

Ravi

unread,
Feb 4, 2015, 8:02:48 AM2/4/15
to chromi...@chromium.org

When reviewers LGTM an issue, they assume that the code that is reviewed matches the code that is eventually submitted. Small deviations make sense (merges or lgtm with nits), but it would be useful to inform approvers when they occur.

If a new patchset is uploaded after atleast one reviewer has approved the issue then the approvers are notified with "New patchsets have been uploaded after l-g-t-m from xyz,abc".
If the approvers have already been notified then they are not notified again for subsequent new patchset uploads. They will be notified again only if they LGTM again.

Original feature request was in chromium:454356.

Thanks,
Ravi

Philippe Gervais

unread,
Feb 4, 2015, 12:40:00 PM2/4/15
to rmi...@chromium.org, chromium-dev
Thanks a lot Ravi for this feature! Rietveld is getting safer.

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

Lei Zhang

unread,
Feb 4, 2015, 2:10:49 PM2/4/15
to rmi...@chromium.org, Chromium-dev
BTW, it looks like the emails do not CC chromium-reviews, so my normal
email filter for Chromium code reviews does not catch these and they
go into my inbox. So it's time to add a new filter for "New patchsets
have been uploaded after l-g-t-m".

On Wed, Feb 4, 2015 at 5:02 AM, Ravi <rmi...@chromium.org> wrote:
>

Scott Graham

unread,
Feb 5, 2015, 1:26:20 PM2/5/15
to Lei Zhang, rmi...@chromium.org, Chromium-dev
I find this one a bit noisy. I often use Rietveld as a visual diff tool because I can't read the output of `git diff` very effectively.

So when I upload to self-review, my reviewer gets spammed right away (before I publish that I've addressed nits, etc.) presumably opening up the CL to look when it's not necessarily ready yet.

If the motivation is safety (i.e. to avoid changes being snuck in), would it be possible to instead notify when the CL lands that what landed was different, rather than when a new patchset is uploaded?

Nico Weber

unread,
Feb 5, 2015, 1:27:47 PM2/5/15
to Scott Graham, Lei Zhang, rmi...@chromium.org, Chromium-dev
thakis started writing a reply to this thread

Nico Weber

unread,
Feb 5, 2015, 1:27:56 PM2/5/15
to Scott Graham, Lei Zhang, rmi...@chromium.org, Chromium-dev
On Thu, Feb 5, 2015 at 10:25 AM, Scott Graham <sco...@chromium.org> wrote:
I find this one a bit noisy. I often use Rietveld as a visual diff tool because I can't read the output of `git diff` very effectively.

So when I upload to self-review, my reviewer gets spammed right away (before I publish that I've addressed nits, etc.) presumably opening up the CL to look when it's not necessarily ready yet.

If the motivation is safety (i.e. to avoid changes being snuck in), would it be possible to instead notify when the CL lands that what landed was different, rather than when a new patchset is uploaded?

+1, I agree with all this.

Ravi

unread,
Feb 5, 2015, 2:59:08 PM2/5/15
to chromi...@chromium.org, sco...@chromium.org, the...@chromium.org, rmi...@chromium.org, Jason Robbins


On Thursday, February 5, 2015 at 1:27:56 PM UTC-5, Nico Weber wrote:
On Thu, Feb 5, 2015 at 10:25 AM, Scott Graham <sco...@chromium.org> wrote:
I find this one a bit noisy. I often use Rietveld as a visual diff tool because I can't read the output of `git diff` very effectively.

So when I upload to self-review, my reviewer gets spammed right away (before I publish that I've addressed nits, etc.) presumably opening up the CL to look when it's not necessarily ready yet.

If the motivation is safety (i.e. to avoid changes being snuck in), would it be possible to instead notify when the CL lands that what landed was different, rather than when a new patchset is uploaded?

+1, I agree with all this.

I expected lgtm w/nits to be resolved in the next uploaded patchset but, as you mentioned, this is not always the cause. I am though not sure how to work around this. Rietveld does not really know when a CL has landed. It could look for messages like "Committed patchset #10 (id:380001)..." from the CQ and manual commits, but that does not seem very reliable.
Personally as a reviewer, if I did 'lgtm w/nits' I would not jump on the notifications until the author replied with 'Done' to my comments. I would be more curious to check the CL's latest patchset if I was not expecting any further changes. Thoughts?

Nico Weber

unread,
Feb 5, 2015, 3:02:22 PM2/5/15
to rmi...@chromium.org, Chromium-dev, Scott Graham, Lei Zhang, Jason Robbins
On Thu, Feb 5, 2015 at 11:59 AM, Ravi <rmi...@chromium.org> wrote:


On Thursday, February 5, 2015 at 1:27:56 PM UTC-5, Nico Weber wrote:
On Thu, Feb 5, 2015 at 10:25 AM, Scott Graham <sco...@chromium.org> wrote:
I find this one a bit noisy. I often use Rietveld as a visual diff tool because I can't read the output of `git diff` very effectively.

So when I upload to self-review, my reviewer gets spammed right away (before I publish that I've addressed nits, etc.) presumably opening up the CL to look when it's not necessarily ready yet.

If the motivation is safety (i.e. to avoid changes being snuck in), would it be possible to instead notify when the CL lands that what landed was different, rather than when a new patchset is uploaded?

+1, I agree with all this.

I expected lgtm w/nits to be resolved in the next uploaded patchset but, as you mentioned, this is not always the cause. I am though not sure how to work around this.

What about notifying if a new patch set has been uploaded when cq is pressed?

Scott Graham

unread,
Feb 5, 2015, 3:02:50 PM2/5/15
to Nico Weber, rmi...@chromium.org, Chromium-dev, Lei Zhang, Jason Robbins
Or when the issue is closed?

Ravi

unread,
Feb 5, 2015, 3:13:04 PM2/5/15
to chromi...@chromium.org, tha...@chromium.org, rmi...@chromium.org, the...@chromium.org, jrob...@chromium.org


On Thursday, February 5, 2015 at 3:02:50 PM UTC-5, Scott Graham wrote:
Or when the issue is closed?

On Thu, Feb 5, 2015 at 12:01 PM, Nico Weber <tha...@chromium.org> wrote:
On Thu, Feb 5, 2015 at 11:59 AM, Ravi <rmi...@chromium.org> wrote:


On Thursday, February 5, 2015 at 1:27:56 PM UTC-5, Nico Weber wrote:
On Thu, Feb 5, 2015 at 10:25 AM, Scott Graham <sco...@chromium.org> wrote:
I find this one a bit noisy. I often use Rietveld as a visual diff tool because I can't read the output of `git diff` very effectively.

So when I upload to self-review, my reviewer gets spammed right away (before I publish that I've addressed nits, etc.) presumably opening up the CL to look when it's not necessarily ready yet.

If the motivation is safety (i.e. to avoid changes being snuck in), would it be possible to instead notify when the CL lands that what landed was different, rather than when a new patchset is uploaded?

+1, I agree with all this.

I expected lgtm w/nits to be resolved in the next uploaded patchset but, as you mentioned, this is not always the cause. I am though not sure how to work around this.

What about notifying if a new patch set has been uploaded when cq is pressed?

Checking for new patches when the CQ checkbox is clicked sgtm, since the intent is to now commit the patch. This will not however cover the 'git cl land' case unless doing that uploads the patch to Rietveld to check if it changed or not (jochen@ has spoken about this recently).
I prefer this more than checking when the issue is closed because I know I have closed many issues that have not been committed and abandoned for various reasons.

How do others feel about performing this check when CQ checkbox is clicked?

Scott Graham

unread,
Feb 5, 2015, 3:20:53 PM2/5/15
to rmi...@chromium.org, chromium-dev, Nico Weber, Lei Zhang, Jason Robbins
On Thu, Feb 5, 2015 at 12:13 PM, Ravi <rmi...@chromium.org> wrote:


On Thursday, February 5, 2015 at 3:02:50 PM UTC-5, Scott Graham wrote:
Or when the issue is closed?

On Thu, Feb 5, 2015 at 12:01 PM, Nico Weber <tha...@chromium.org> wrote:
On Thu, Feb 5, 2015 at 11:59 AM, Ravi <rmi...@chromium.org> wrote:


On Thursday, February 5, 2015 at 1:27:56 PM UTC-5, Nico Weber wrote:
On Thu, Feb 5, 2015 at 10:25 AM, Scott Graham <sco...@chromium.org> wrote:
I find this one a bit noisy. I often use Rietveld as a visual diff tool because I can't read the output of `git diff` very effectively.

So when I upload to self-review, my reviewer gets spammed right away (before I publish that I've addressed nits, etc.) presumably opening up the CL to look when it's not necessarily ready yet.

If the motivation is safety (i.e. to avoid changes being snuck in), would it be possible to instead notify when the CL lands that what landed was different, rather than when a new patchset is uploaded?

+1, I agree with all this.

I expected lgtm w/nits to be resolved in the next uploaded patchset but, as you mentioned, this is not always the cause. I am though not sure how to work around this.

What about notifying if a new patch set has been uploaded when cq is pressed?

Checking for new patches when the CQ checkbox is clicked sgtm, since the intent is to now commit the patch. This will not however cover the 'git cl land' case unless doing that uploads the patch to Rietveld to check if it changed or not (jochen@ has spoken about this recently).
I prefer this more than checking when the issue is closed because I know I have closed many issues that have not been committed and abandoned for various reasons.

How do others feel about performing this check when CQ checkbox is clicked?

That seems better to me.

Presumably the `git cl land` case needs to be handled via a different auditing mechanism, as anyone with committer rights can surely commit "whatever" without Rietveld knowing.

James Cook

unread,
Feb 6, 2015, 12:14:22 PM2/6/15
to Scott Graham, rmi...@chromium.org, chromium-dev, Nico Weber, Lei Zhang, Jason Robbins
Can the "added after l-g-t-m" include the patchset title?  I'm much more concerned about patches titled "fix crash" or "fix test failure" than "rebase" or "fix gn".

James

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

Ravi

unread,
Feb 9, 2015, 8:10:03 AM2/9/15
to chromi...@chromium.org, sco...@chromium.org, rmi...@chromium.org, tha...@chromium.org, the...@chromium.org, jrob...@chromium.org, jame...@chromium.org


On Friday, February 6, 2015 at 12:14:22 PM UTC-5, James Cook wrote:
Can the "added after l-g-t-m" include the patchset title?  I'm much more concerned about patches titled "fix crash" or "fix test failure" than "rebase" or "fix gn".

Including the patchset title (if it exists) in the message does sound useful. I will add it, thank you for the suggestion.
 

James

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

Thiago Farina

unread,
Feb 9, 2015, 1:47:46 PM2/9/15
to Ravi Mistry, Chromium-dev, Scott Graham, Nico Weber, Lei Zhang, Jason Robbins, James Cook
On Mon, Feb 9, 2015 at 11:10 AM, Ravi <rmi...@chromium.org> wrote:


On Friday, February 6, 2015 at 12:14:22 PM UTC-5, James Cook wrote:
Can the "added after l-g-t-m" include the patchset title?  I'm much more concerned about patches titled "fix crash" or "fix test failure" than "rebase" or "fix gn".

Including the patchset title (if it exists) in the message does sound useful. I will add it, thank you for the suggestion.
 
Also the message is saying "patchsets", but I think it should say "patchset" as we usually only upload one patch per time, specially in the rebase case. Which I think will be most of the cases where someone will upload a new patch set after a lgtm.

-- 
Thiago Farina

Ricky Zhou

unread,
Feb 9, 2015, 9:11:15 PM2/9/15
to tfa...@chromium.org, Ravi Mistry, Chromium-dev, Scott Graham, Nico Weber, Lei Zhang, Jason Robbins, James Cook
Nice to have this feature! Would it also be worth also sending
notifications when somebody deletes the latest patch set? Unless
reviewers look at all previous patch sets, it seems like it might
still be possible for an evil person to sneak a bad change in the
middle of a long series of patch sets, then delete the last couple of
patch sets post-LGTM.

Ricky

Ravi

unread,
Feb 10, 2015, 7:10:57 AM2/10/15
to chromi...@chromium.org, tfa...@chromium.org, rmi...@chromium.org, sco...@chromium.org, tha...@chromium.org, the...@chromium.org, jrob...@chromium.org, jame...@chromium.org
Thiago, it says "patchsets" because it stops notifying after the 1st unapproved patchset (to reduce spam). So there could be one or more new patchsets uploaded.

Ricky, Nico's proposal of only notifying when the CQ button is pressed would cover deleted patchsets because it would notify only when the change is ready to the submitted.

Dana Jansens

unread,
Feb 10, 2015, 12:46:51 PM2/10/15
to ric...@chromium.org, Thiago Farina, Ravi Mistry, Chromium-dev, Scott Graham, Nico Weber, Lei Zhang, Jason Robbins, James Cook
On Mon, Feb 9, 2015 at 6:10 PM, Ricky Zhou <ric...@chromium.org> wrote:
Nice to have this feature! Would it also be worth also sending
notifications when somebody deletes the latest patch set? Unless
reviewers look at all previous patch sets, it seems like it might
still be possible for an evil person to sneak a bad change in the
middle of a long series of patch sets, then delete the last couple of
patch sets post-LGTM.

May I ask why we're so worried about this? Wouldn't we revoke their right to hit the CQ button if they are evil?

I also find the current notification noisy and look forward to just hearing about it on commit if I have to hear about it.

Raymond Toy

unread,
Feb 12, 2015, 5:21:18 PM2/12/15
to rmi...@chromium.org, chromium-dev
Can these emails be sent out only if the lgtm is from an owner (or committer)? I have most of my CLs reviewed initially by a non-committer and any fixes I upload get the email. 

On Wed, Feb 4, 2015 at 5:02 AM, Ravi <rmi...@chromium.org> wrote:

--
--
Chromium Developers mailing list: chromi...@chromium.org
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.

Ravi

unread,
Feb 13, 2015, 7:27:01 AM2/13/15
to chromi...@chromium.org, rmi...@chromium.org

Rietveld does not currently know who committers are, but this will change after chromium:241879 is fixed.
Also, having the notifications happen only when the CQ is clicked (I am working on this) should hopefully address your comment.

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

Raymond Toy

unread,
Feb 13, 2015, 12:34:17 PM2/13/15
to chromium-dev
That works for me.  Thanks!

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

Ravi

unread,
Feb 24, 2015, 7:25:11 AM2/24/15
to chromi...@chromium.org

Rietveld now notifies approvers of new patchsets only when the CQ is clicked. Fixed in chromium:458547.

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

Reply all
Reply to author
Forward
0 new messages