| Umherirrender | |
| Jun 29 2020, 1:54 PM |
When looking at merged https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/603493
It has lost the last vote.
Having merged patch sets without the ✓ looks bad.
Please have a look why there are lost and may restore if possible
A list:
https://gerrit.wikimedia.org/r/q/status:merged+-label:Code-Review%252B2,50
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | QChris | T186406 Polygerrit Outgoing reviews, Incoming reviews and Recently closed headings should be more different from commits | |||
| Resolved | None | T220801 PolyGerrit 2.15 UI does not support custom dashboards (supported from in 2.16+) | |||
| Resolved | QChris | T206453 Big empty space under patch description | |||
| Restricted Task | |||||
| Resolved | None | T210515 context links in diff don't use pointer cursor | |||
| Resolved | Paladox | T223278 "Server error: line 1:33 no viable alternative at character '['" when visiting particular patch | |||
| Restricted Task | |||||
| Resolved | hashar | T103990 Gerrit upload-pack send ALL references causing massive network I/O on common operations | |||
| Resolved | None | T195280 New Gerrit UI does not contain copyable Change-Id or parent commit id | |||
| Declined | Feature | None | T201246 Make a PolyGerrit plugin to welcome new users | ||
| Resolved | QChris | T254158 Gerrit 3.2 upgrade | |||
| Declined | BUG REPORT | None | T256633 Some merged patch sets does not show the last Code-Review+2 vote in Gerrit3 |
@QChris and I chatted about it last night and we could not find any obvious reason for the lack of CR+2. The change metadata or review notes do show the change got submitted with a code-review +2, although it was carried over from a previous patchset since we reapply scores for trivial rebases/patchsets ( https://gerrit.wikimedia.org/r/c/All-Projects/+/124891 ):
[label "Code-Review"] function = MaxWithBlock copyMinScore = true copyAllScoresOnTrivialRebase = true copyAllScoresIfNoCodeChange = true
That might confuses Gerrit at some point but we haven't found a way to reproduce it and lack clear data.
@Umherirrender: What a great find. Thanks!
I went through the first 50 patch sets of merged changes without CR+2.
For most of them, some user simply by-passed code review and pushed the change directly onto the branch. Example change 604556 Note the "Change has been successfully pushed." comment ("pushed" instead of "merged"). These cases seem on purpose.
(For a few of these, it's no so clear to see that they got pushed through as they say "Change has been merged by [...]". But note that you get that message when you have push permission and push with the submit option.)
For 5 (which is a surprisingly high number from my point of view) the CR+2 got retracted after the merge. Example change 604992 Note the "Removed Code-Review+2 by [...]" in the last but 2 comments.
For a few, a behaviour change of Gerrit became effective (I did not know or notice that before-hand, and this required quite some digging. However, once one knows what's going on, it's also easy to find the relevant commit: da70bea56d776ac7f71fa0192d2053b387427da2 ).
Assume you upload PS1 and get a CR+2.
Then you rebase to arrive at PS2, and (since it's a simple rebase) CR+2 gets copied over.
Then you rework your change and arrive at PS3. The CR+2 gets removed, as there are code changes.
Then you decide that the code changes are not worth it, and you upload a rebase of PS2 as PS4.
In the old Gerrit v2.15, PS4 reclaimed the CR+2 from PS2. Hence, PS4 had CR+2 and Jenkins could merge.
In the new Gerrit v3.2.2, a lost vote never gets reclaimed (due to performance reasons. Meh.). So the code change in PS3 removed the CR+2 for good, and PS4 does no longer reclaim the CR+2 of PS2.
Since votes are not stored in Gerrit's backend, but recomputed from its NoteDB, we no longer see that the change once had a CR+2 when it got submitted. To be honest, I think this sucks. However, these changes had a CR+2 when they got merged.
An example is the change you called out: change 603493. PS9 received CR+2. PS12 is a rework and takes away the CR+2 for good. PS16 (the merged one) is again a rebase of PS9.
The only one of the 50 that I could not figure out is change 604347. It looks like a straight rebase. But the parent changes along. And if I try to replay what happened on a local Gerrit v2.15.14, PS2 does not get recognized as rebase and has CR+2 revoked. However, on WMF's gerrit, the message for PS2 is "Uploaded patch set 2: Patch Set 1 was rebased." So maybe Gerrit retained the CR+2 here? There are a few things you can do with edits and drafts. I tried hard, but I cannot get my local Gerrit v2.15.14 to think PS2 is a rebase, so I fail to fully figure out what's going on.
So we started with the latest 50 merged changes without CR+2. The oldest of the 50 got merged 2019-08-14, so those 50 cover almost a year. Most of them are false positives (direct pushes, revoked CR+2). A few are not satisfactory (Gerrit behaviour change). And for, one we don't have a good explanation.
It's not perfect.
But after this analysis, I think there is no real bug that's at play here. It's more many false positives, and a Gerrit behaviour change. Hence, marking declined, because it would of course be great to set these few straight that really had a CR+2 when getting merged and are missing CR+2 now. But there is no easy way (without messing with brittle NoteDB) to achieve that. And the impact is really little.
@QChris that is awesome! Thank you for the very detailed explanation as to why some CR+2 are no more showing when in Gerrit 2.15 they definitely had it. Well done!
(thank you @Umherirrender for the bug report, that was definitely worth investigating)