Rietveld is posting "User@ deleted patchset X"

[Christopher Lam]

Hey,

So it seems that when a user deletes a patchset, Rietveld now automagically posts a message marking the deletion.

Is there any way to turn this off? I often upload to Rietveld just because it has a nicer diff that I would prefer to self-review in or to run a try job just to see what will happen without wanting it in the review history proper. Doing these things now puts me at risk of spamming people with deletion messages.

Thanks,

-calamity

[Matt Giuca]

+1

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

[Ilya Sherman]

FWIW, I don't think these messages are emailed out -- they're just noted on the codereview site -- so you're not spamming anyone.

[Brett Wilson]

On the other hand, I was really happy to see the "User X has added
reviewer foo" comment on the code review tool today. I've been
frustrated before trying to see who added me when I was added to a
code review.

Brett

[Ilya Sherman]

On Mon, Aug 25, 2014 at 8:28 PM, Brett Wilson <bre...@chromium.org> wrote:

On the other hand, I was really happy to see the "User X has added
reviewer foo" comment on the code review tool today. I've been
frustrated before trying to see who added me when I was added to a
code review.

+1.  I'm a fan of all the new annotations, as long as they continue not to send mail.

[Nico Weber]

On Mon, Aug 25, 2014 at 8:29 PM, Ilya Sherman <ishe...@chromium.org> wrote:

On Mon, Aug 25, 2014 at 8:28 PM, Brett Wilson <bre...@chromium.org> wrote:

On the other hand, I was really happy to see the "User X has added
reviewer foo" comment on the code review tool today. I've been
frustrated before trying to see who added me when I was added to a
code review.

+1.  I'm a fan of all the new annotations, as long as they continue not to send mail.

They do seem to cause every of review I send out to begin with "Re: " (apparently the mailer adds that if there's a comment on the bug already?)

[Matt Giuca]

I think that posting to tell you that reviewers have been added/removed is helpful. But the patch set deletion is not only spammy, but seems pointless and makes me feel bad for cleaning up.

Often I'll upload a patch set, then realise it should be slightly fixed or split into two and it's not worth the reviewer having to deal with a patch set correcting the previous patch set that he/she hasn't even looked at yet, so if it's within a few minutes I'll just delete the patch set and upload new ones. Now there's a record of my shenanigans (whereas previously I could just fix it and leave it in a clean state). (So yeah, mostly it's an OCD thing, but I don't like exposing this information.)

[Ryan Sleevi]

On Aug 25, 2014 9:37 PM, "Matt Giuca" <mgi...@chromium.org> wrote:
>
> I think that posting to tell you that reviewers have been added/removed is helpful. But the patch set deletion is not only spammy, but seems pointless and makes me feel bad for cleaning up.
>
> Often I'll upload a patch set, then realise it should be slightly fixed or split into two and it's not worth the reviewer having to deal with a patch set correcting the previous patch set that he/she hasn't even looked at yet, so if it's within a few minutes I'll just delete the patch set and upload new ones. Now there's a record of my shenanigans (whereas previously I could just fix it and leave it in a clean state). (So yeah, mostly it's an OCD thing, but I don't like exposing this information.)
>

As much as I hate +1 pile-ons, a huge +1 for the same neatness obsession.

[Primiano Tucci]

+2: one for the As much as I hate +1 pile-ons) and one for Matt's comment. I like to not expose trivial self-reviewed nits to the actual reviewer.

Can we make it so that no message is generated if there were no comments after the patchset being deleted?

--

[rmi...@chromium.org]

Apologizes for the lack of PSA, I was not sure when these features were going to become live in Rietveld.

Here is some information:

Rietveld now records the patchset# that is committed / reverted (announced here). As part of the effort of improving the auditability of Rietveld issues, the following two additions were made:

* Log when a patchset is deleted.

The owner of an issue can delete any patchset at any time. This can cause confusion when the CQ posts a message saying a patchset# has been submitted, and then the owner goes back and deletes and recreates it. Similarly, when a reverted patchset (using the one-click revert button) is deleted and recreated. I discussed this with chrome infra and it was agreed that having an easily accessible log of the sequence of events here will help improve auditability.

* Log when the reviewers list in Rietveld changes

The reviewers list can be easily changed in an issue, we should be able to find out what changed in the list and who changed it.

These new annotations do not send out email, they just append to the comments on the Rietveld issue.

[rmi...@chromium.org]

On Monday, August 25, 2014 11:31:49 PM UTC-4, Nico Weber wrote:

On Mon, Aug 25, 2014 at 8:29 PM, Ilya Sherman <ishe...@chromium.org> wrote:

On Mon, Aug 25, 2014 at 8:28 PM, Brett Wilson <bre...@chromium.org> wrote:

On the other hand, I was really happy to see the "User X has added
reviewer foo" comment on the code review tool today. I've been
frustrated before trying to see who added me when I was added to a
code review.

+1.  I'm a fan of all the new annotations, as long as they continue not to send mail.

They do seem to cause every of review I send out to begin with "Re: " (apparently the mailer adds that if there's a comment on the bug already?)

Thanks Nico.

Filed: https://code.google.com/p/chromium/issues/detail?id=407565

[Ryan Hamilton]

On Tue, Aug 26, 2014 at 5:34 AM, <rmi...@chromium.org> wrote:

Apologizes for the lack of PSA, I was not sure when these features were going to become live in Rietveld.

Here is some information:

Rietveld now records the patchset# that is committed / reverted (announced here). As part of the effort of improving the auditability of Rietveld issues, the following two additions were made:

* Log when a patchset is deleted.

The owner of an issue can delete any patchset at any time. This can cause confusion when the CQ posts a message saying a patchset# has been submitted, and then the owner goes back and deletes and recreates it. Similarly, when a reverted patchset (using the one-click revert button) is deleted and recreated. I discussed this with chrome infra and it was agreed that having an easily accessible log of the sequence of events here will help improve auditability.

​Then perhaps this activity log​ could be accessed via a link instead of visible by default on the main page. +1 to all the other comments about not cluttering the UI with information that is not relevant to the task of writing or reviewing the CL.

[Matt Giuca]

On 27 August 2014 00:23, Ryan Hamilton <r...@chromium.org> wrote:

On Tue, Aug 26, 2014 at 5:34 AM, <rmi...@chromium.org> wrote:

Apologizes for the lack of PSA, I was not sure when these features were going to become live in Rietveld.

Here is some information:

Rietveld now records the patchset# that is committed / reverted (announced here). As part of the effort of improving the auditability of Rietveld issues, the following two additions were made:

* Log when a patchset is deleted.

The owner of an issue can delete any patchset at any time. This can cause confusion when the CQ posts a message saying a patchset# has been submitted, and then the owner goes back and deletes and recreates it. Similarly, when a reverted patchset (using the one-click revert button) is deleted and recreated. I discussed this with chrome infra and it was agreed that having an easily accessible log of the sequence of events here will help improve auditability.

​Then perhaps this activity log​ could be accessed via a link instead of visible by default on the main page. +1 to all the other comments about not cluttering the UI with information that is not relevant to the task of writing or reviewing the CL.

Yeah, I'm happy for that information to be logged and publicly accessible. I just don't want it cluttering the main review UI.

Alternatively, if the specific thing you want to catch is people submitting a patch set then deleting it, you could only log when a patch set is deleted if it has been submitted (or any other auditable conditions like having had try jobs run on it, having the commit box ticked, etc).

[rmi...@chromium.org]

Discussed this in detail with jrobbins@ yesterday.

We came up with the following plan:

1) Add patchset ids to all messaging where it is important to identify which patchset id (not just the incremental patchset number) was used.

  AFAIK there are two missing places right now that only log the incremental number without mentioning the id:

** Manual commits. Eg: "Committed patchset #2 manually as 40e90b3 (presubmit successful)."

** Reverts. Eg: "A revert of this CL (patchset #2) has been created in..."

2) Remove the patchset deletion messages. If every commit and revert message contains the patchset id then during audits we should be able to determine which patchsets were different.

3) Add a new 'auto_generated' field to messages created by the system for bookkeeping. Display these 'auto_generated' messages in a yet to be determined UI (jrobbins@ is going to experiment with this). After this is done we may be able to bring back the patchset deletion messages.

[Aaron Gable]

I would propose using just #1 and #3. Then, once messages are annotated with "auto_generated", add a toggle to show or hide them. Default the toggle to "hide". There are other review systems and bug trackers that use this system (come talk to me if you want to see a great example), and it works really well.

Aaron

[Ryan Hamilton]

This sounds ideal to me.

[rmi...@chromium.org]

Aaron shared with me the toggling example he was referring to, and it looked good. It contained a switch that displays/hides all auto_generated messages.

Regarding the original proposal, I would still like to do step #2 (disable patchset deletion logging) because step #3 may take some time to completely implement. Also after step #3 I will re-enable patchset deletion logging.

Here are the step #1 updates:

* cl/513763002 added patchset ids to manual commits. This is live.

* I also added 'id:' prefix to commit messages posted by the CQ to make it consistent with the above manual commit messages.

* cl/131400043 added patchset ids to Rietveld reverts. This is submitted but not live yet.

For step #2:

* cl/129700043 stops the logging of patchset deletions. This is also submitted but not live yet.

I will next start working on step #3 to display a toggle.

[Dirk Pranke]

I like the toggle idea, but I would probably exclude cq-generated messages from being hidden. 

In other words, I would always display "CQ is trying ...", "Try jobs failed for ...", and "CQ committed ..." messages, *especially* the last one (the fact that something was committed).

But perhaps that's just me ...

-- Dirk

[rmi...@chromium.org]

On Wednesday, August 27, 2014 5:34:13 PM UTC-4, Dirk Pranke wrote:

I like the toggle idea, but I would probably exclude cq-generated messages from being hidden. 

In other words, I would always display "CQ is trying ...", "Try jobs failed for ...", and "CQ committed ..." messages, *especially* the last one (the fact that something was committed).

I agree with this, cq-generated messages should not be hidden.

The plan was to hide messages created by Rietveld for auditing like:

* 'CQ was (un)checked by..'

* 'xyz changed the reviewers...'

* 'xyz deleted patchset #1 (id:123)'

[Matt Menke]

There's a bug in this feature.  When a patch set in an unpublished CL is deleted, the CL is considered an "outgoing review", and the first actually published message doesn't include the standard review request text.

[rmi...@chromium.org]

On Thursday, August 28, 2014 10:47:51 AM UTC-4, Matt Menke wrote:

There's a bug in this feature.  When a patch set in an unpublished CL is deleted, the CL is considered an "outgoing review", and the first actually published message doesn't include the standard review request text.

Thank you for reporting the bug, I CC'ed you to https://code.google.com/p/chromium/issues/detail?id=407382

[rmi...@chromium.org]

The ability to hide/display auto generated messages is now live on Rietveld.

cl/131690043 added this functionality. From the CL description:

Adds ability to display/hide auto generated messages.

* Created a new counter for generated messages. The text will now look like: "Total messages: 3 (2 generated)".

* Added new 'Show Generated Messages' and 'Hide Generated Messages' links.

* Renamed 'Expand All Messages' to 'Expand Messages' and 'Collapse All Messages' to 'Collapse Messages'.

* Displayed generated messages with light grey background and default border.

* Generated messages are hidden by default. If the only messages that exist are generated messages then they are not hidden by default.

An example:

https://codereview.chromium.org/534653003/#msg5 (extreme example that displays all message colors that currently exist in Rietveld)

The patchset deletion logging has also been turned on.

Thanks!

On Wednesday, August 27, 2014 5:40:50 PM UTC-4, rmi...@chromium.org wrote:

[Elliott Sprehn]

I'm seeing tons of issue spam on Rietveld now, for example: https://codereview.chromium.org/498773007

Publishing my comments made me a reviewer which spammed the issue with a reviewer added comment, my code comments, and then an empty comment.

[rmi...@chromium.org]

On Tuesday, September 2, 2014 4:30:29 PM UTC-4, Elliott Sprehn wrote:

I'm seeing tons of issue spam on Rietveld now, for example: https://codereview.chromium.org/498773007

Publishing my comments made me a reviewer which spammed the issue with a reviewer added comment, my code comments, and then an empty comment.

When you publish comments the 'add as a reviewer' checkbox is checked by default (this behavior has not changed). This is why it says the reviewer list changed (this generated message is hidden by default), the code comments being published is the old behavior as well.

I cannot explain the empty comment which was sent a second later though, and have not been able to reproduce it in this test issue:

https://codereview.chromium.org/531923002/ 

[Jason Robbins]

The logs show two separate POST requests to /498773007/publish about 300ms apart.

I'm thinking that this might be another manifestation of issue 408228.

Thanks,

jason!

[Scott Graham]

On Tue, Sep 2, 2014 at 10:54 AM, <rmi...@chromium.org> wrote:

The ability to hide/display auto generated messages is now live on Rietveld.

Could it possibly remember what I last used? I find I mostly want to "Show generated messages" so I have to reclick it every time I reload. (I know that's the opposite of the complaints up-thread).

[rmi...@chromium.org]

On Monday, September 8, 2014 8:04:38 PM UTC-4, Scott Graham wrote:

On Tue, Sep 2, 2014 at 10:54 AM, <rmi...@chromium.org> wrote:

The ability to hide/display auto generated messages is now live on Rietveld.

Could it possibly remember what I last used? I find I mostly want to "Show generated messages" so I have to reclick it every time I reload. (I know that's the opposite of the complaints up-thread).

Could you please file a crbug and assign it to me, I will take a look.

Thanks!

[Scott Graham]

On Tue, Sep 9, 2014 at 5:11 AM, <rmi...@chromium.org> wrote:

On Monday, September 8, 2014 8:04:38 PM UTC-4, Scott Graham wrote:

On Tue, Sep 2, 2014 at 10:54 AM, <rmi...@chromium.org> wrote:

The ability to hide/display auto generated messages is now live on Rietveld.

Could it possibly remember what I last used? I find I mostly want to "Show generated messages" so I have to reclick it every time I reload. (I know that's the opposite of the complaints up-thread).

Could you please file a crbug and assign it to me, I will take a look.

https://code.google.com/p/chromium/issues/detail?id=412425

Thanks

[Ravi]

On Tuesday, September 9, 2014 1:32:35 PM UTC-4, Scott Graham wrote:

On Tue, Sep 9, 2014 at 5:11 AM, <rmi...@chromium.org> wrote:

On Monday, September 8, 2014 8:04:38 PM UTC-4, Scott Graham wrote:

On Tue, Sep 2, 2014 at 10:54 AM, <rmi...@chromium.org> wrote:

The ability to hide/display auto generated messages is now live on Rietveld.

Could it possibly remember what I last used? I find I mostly want to "Show generated messages" so I have to reclick it every time I reload. (I know that's the opposite of the complaints up-thread).

Could you please file a crbug and assign it to me, I will take a look.

https://code.google.com/p/chromium/issues/detail?id=412425

Added ability to opt-in to view generated messages by default. You can set this preference by going to your settings here: https://codereview.chromium.org/settings