Incorrect "Note: The patchset sent to CQ was uploaded after this CL was approved." |
|||||
Issue descriptionAffected Version: 2.15-3037-gb931317fa0 What steps will reproduce the problem? 1. Review a Chromium CL, and mark it as "+1" to sticky-approve it, with nits. 2. Have the author upload a fixed version. 3. Re-review the CL, still be happy with it, and mark it as CR+1,CQ+2. What is the expected output? No message about changes after the CL was approved, since I've approved both the original and the revised version. What do you see instead? A spurious note about post-approval changes. In particular, this came up here: https://chromium-review.googlesource.com/c/chromium/src/+/1005791
,
Apr 16 2018
,
Jan 3
Based on the log of non-comment changes in https://chromium-review.googlesource.com/c/chromium/src/+/1005791 the warning makes some sense. If I view all changes (toggling off "comment-only") I see that CR+1 was attached to Patch Set #3, and CQ+2 was applied to Patch Set #5 (two uploads later). That said we copy min/max scores for Code-Review label for chromium/src (https://chromium.googlesource.com/chromium/+/meta/config/project.config), if we're going to make CR+1 sticky does this warning make sense?
,
Jan 10
Issue moved from a project with a different set of status labels. "New" is not a supported status in /p/chromium, so these ended up in a black-hole.
,
Yesterday
(34 hours ago)
Agree with ajp@ analysis above. This warning is really a hack in CQ as it was the easiest way to warn reviwers of potentially unreviewed code due sticky CR+1. Such potentially unreviewed patchsets are effectively TBRs, and as I and others argued elsewhere, TBRs should be tracked and accounted for in Gerrit itself (or its plugin). Hence, adding Gerrit tracker. +agable@ Anyways, so long as this warning is in CQ, it'd be nice to treat situation where CQ+2 is given by reviewer who gave CR+1 before specially and avoid sending notification, which I think is Pri-2. Also, I think this is a bit tricky to do right given potentially sticky CQ votes, more than one reviewer, and more than one triggering CQ vote.
,
Today
(13 hours ago)
Agreed -- I think there is one situation in which it makes sense to elide the warning: when there's only one reviewer (a.k.a. non-uploader) who has voted CR+1 on any patchset except the most recent, and that person is also the person who set CQ+2 on the most recent patchset. In all other situations (e.g. multiple people voted on earlier patchsets, or someone different is setting CQ+2) it makes sense to keep the warning. Since that's a pretty specific case, I'd say that addressing it is Pri-3 at most. Probably a quick and easy change, so if someone wants to pick it up, cool, but not something to take precedence over other bugs. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by logan@google.com
, Apr 16 2018