1261942 - Mozreview gets review status confused after I insert a "part 0" patch at the beginning of a patch-stack (and change the commit ID of some later patch in the stack)

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect)

Description

[Daniel Holbert [:dholbert]]

I just got Mozreview into a confusing state on bug 1257688, and I think it's a bug.

WHAT HAPPENED:
 (1) I posted patches for parts 1 through 4 (via "hg push review").
 (2) Those all received r+.
 (3) I realized that I needed a "part 0", and I wrote patches for "part 0" and "part 5" locally. Those include "r?" in their commit message, and I updated all of my other commit messages (for part 1 through 4) to have r+ for explicitness.
 (4) I pushed my new patch stack (parts 0 through 5) to mozreview, with "hg push review"

EXPECTED RESULTS:
The mozreview page ( https://reviewboard.mozilla.org/r/43951/ ) should show parts 1 through 4 as still having status "r+". It should show parts 0 and 5 as having status "r?".

ACTUAL RESULTS:
- The mozreview page shows parts 0 through 3 as having status "r+", despite the fact that part 0 is brand-new.
- It shows parts 4 and 5 as having status "r?" despite the fact that part 4 was already reviewed.
- In other words, it seems to simply think "the first four patches have been reviewed", despite the fact that another first patch has been inserted.

To its credit, MozReview did correctly seem to track changes between the patches across my new review push -- at least, it kept some review comments on Part 1 associated with thew new Part 1 (instead of e.g. bumping them to Part 0).  So this sort of worked, but review statuses are the key thing that I'm noticing were incorrectly transferred to the new push.

Comment 1

[Daniel Holbert [:dholbert]]

(Side note: I'm not sure what the "right" way is to get out of this confusing state.  I tried tweaking the review flags on bugzilla to fix the incorrect r+ and r? that I mentioned in my ACTUAL RESULTS, but those changes weren't reflected in Mozreview. I don't see any way to force Mozreview to change a patch's status from "r+" to "r?".)

Comment 2

[Steven MacLeod [:smacleod]]

It appears to me that the MozReview-Commit-ID was changed for Part 3 before the push that messed things up, which caused Part 0 to recycle that review request (which we should be smarter about, but that's a different issue).

:dholbert, did you intentionally, or accidentally, remove or change the MozReview-Commit-ID in the commit message for Part 3?

Comment 3

[Daniel Holbert [:dholbert]]

I did not intentionally change it. It's possible that it changed accidentally, though.

I think I qfolded another patch (with a small amount of "scratch" work) into that Part 3 before re-pushing, which left me with two concatenated commit messages, and I deleted everything below the "***" in the concatenated commit-message section.  That should have left the original commit message intact (including its MozReview-Commit-ID) intact, though.

Comment 4

[Daniel Holbert [:dholbert]]

Yeah, I'll bet it was the qfold that messed me up. From some local testing, it looks like qfolding produces a commit message like so:
==========
Bug NNN Part 1: Blah blah. This patch's original commit ID is D53jxmw2wQb. r?whoever

MozReview-Commit-ID: D53jxmw2wQb
* * *
Patch with some scratch work. This will be qfolded.

MozReview-Commit-ID: 3dEFK3DUVuq
==========

And I'm willing to believe that I just deleted the middle 3 lines, accidentally leaving the commit ID from the later qfolded-away patch.  So I was left with a patch whose MozReview-Commit-ID had changed.

Comment 5

[Daniel Holbert [:dholbert]]

So, I guess the modified MozReview-Commit-ID meant that MozReview was unable to match all of the old patches with all of the new patches, and hence threw up its hands and assumed the first 4 old patches must match the first 4 new patches, and carried forward the r+ statuses based on that assumption?

I think that makes sense. It seems like it could be smarter here, but I can also imagine arbitrarily-more-tricky versions of this scenario (where N patches of an M-patch stack have their commit IDs changed, with X new patches inserted at the beginning of the stack), and it does seem somewhat reasonable that it'd throw up its hands and do simple matching based on queue-position if it can't match all of the old commit IDs.

Comment 6

[Daniel Holbert [:dholbert]]

er, s/and it does seem/so it does seem/

Comment 7

[Daniel Holbert [:dholbert]]

(Feel free to resolve this as WONTFIX if this is a scenario that's not intended to be supported.)

Comment 8

[Steven MacLeod [:smacleod]]

We are definitely going to improve the matching at some point, and have some bugs on file to track such things.

I'm going to leave this bug around though so that we keep this specific case in mind and improve what happens here.

Comment 9

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.