MozReview mismatches new/old patches & rewrites bugzilla coments without warning, if you remove a patch from the middle of a stack & tweak later MozReview-Commit-ID by accident

NEW
Unassigned

Status

2 years ago
2 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

Details

(Reporter)

Description

2 years ago
[I'm filing this bug at gps's request, though I'm not quite sure there's anything actionable here, except maybe to warn if MozReview-Commit-ID is unrecognized]

In bug 1216843 we got into a pretty bad state, reviewability-wise, by accident. Specifically:
 1) Hiro had an original patch stack with ~16 patches.
 2) At my request, hiro spun off part 6 into its own bug.
 3) By accident / without noticing, hiro (or really mercurial-on-hiro's-behalf) modified the MozReview-Commit-ID for his patches.
 4) So, when hiro re-pushed to review...
    - The patch labeled "part 7" was actually now 6th in the patch series (since part 6 was removed). This should be fine; numbering is arbitrary.
    - MozReview couldn't figure out that this new "part 7" (6th in the new series) was the same as the old "part 7" (7th in the old series).  It simply matched it against the old "part 6".  Therefore, the interdiff is useless.
    - This happened for multiple (all?) of the patches after that point.

See for example:
 * the "Review Request Changed" chunk for Revision 5 on https://reviewboard.mozilla.org/r/69424/ (currently at the bottom of that page) -- that's where Part 7 got accidentally mapped onto the (obsolete) review page for Part 6. 
 * the "Review Request Changed" chunk for Revision 6 on https://reviewboard.mozilla.org/r/69426/ (currently at the bottom of that pgae) -- that's where part 8 got accidentally mapped onto the review page for the previous version of Part 7.

The first page there shows the new version of Part 7; the second page shows the old version of Part 7. As you can see by comparing them, the MozReview-Commit-ID changed (from Ic7dIrZWvih to A7E1TzjvMYm ), which is why MozReview couldn't figure out the correct mapping.

This caused various forms of badness:
 - Some review+ state got clobbered (or mis-applied?) apparently -- see bug 1216843 comment 140.
 - Interdiffing is currently useless (e.g. I can't compare current Part 7 against old Part 7, or current part 8 against old part 8, since they're all mismatched).

 - Some old comments **got rewritten** with the updated commit message from the mismatched patch!!!  This makes it look like the old comments are about different patches than they were actually about. For example, bug 1216843 comment 112 I effectively said "This patch should be removed".  When I posted that comment, it started out by saying "Comment on [....] Extend
eCSSUnit_PercentageRGBAColor/eCSSUnit_PercentageRGBColor to store values
greater than 1.0."  I verified this by checking my bugmail. But now, MozReview has changed that comment to start out with "Comment on [...] Part 7: Implement color accumulation."  Basically, it sounds like my old comment was asking for a *completely different* patch to be removed, because MozReview rewrote the intro to that comment with the mismatched patch commit message.


I don't know precisely what the EXPECTED RESULTS should be here, except perhaps that we should try to detect & surface this sort of programmer mistake (hg footgun) sooner.  Maybe whenever MozReview matches a new patch to and old patch based on something other than the MozReview-Commit-ID (e.g. just based on patch ordering), it should surface a warning (either via the command line "push" results, or via the review-draft page) to alert the user that it's guessing about patch-matching and it might very well be wrong & about to mess up interdiff/review-granted-state/etc?

(I don't know precisely how/why the commit IDs changed here -- my current guess is that maybe it was part of an unbitrotting process, and maybe hiro regenerated the patch files "from scratch" at the end of that process, not realizing that this would give him a new MozReview-Commit-ID? Or something like that -- that's one way this *could* happen, at least. And we should consider that sort of case & try to make it less of a footgun, if we can.)
(Reporter)

Updated

2 years ago
Summary: If you remove a patch from the middle of a stack, MozReview can get confused → MozReview mismatches new/old patches & rewrites bugzilla coments without warning, if you remove a patch from the middle of a stack & tweak later MozReview-Commit-ID by accident
(In reply to Daniel Holbert [:dholbert] from comment #0)

> (I don't know precisely how/why the commit IDs changed here -- my current
> guess is that maybe it was part of an unbitrotting process, and maybe hiro
> regenerated the patch files "from scratch" at the end of that process, not
> realizing that this would give him a new MozReview-Commit-ID? Or something
> like that -- that's one way this *could* happen, at least. And we should
> consider that sort of case & try to make it less of a footgun, if we can.)

To be precise, I did not touch MozReview-Commit-ID.  Actually, I did not realized it, all of my MQ files don't have MozReview-Commit-ID at all.  I confirmed now that the IDs were generated when I use hg qpush.
Though I don't know it's related to MozReview-Commit-ID mismatches, there is an interaction issue between review board and bugzilla. Please see bug 1216843 comment 161.
After I did run ./mach mercurial-setup, MozReview-Commit-ID has come to be written in each patch files.

Comment 4

2 years ago
There was another instance a month back of someone with an older version-control-tools checkout whose MQ didn't produce/preserve MozReview-Commit-ID. Pulling down the latest version-control-tools fixed it. I went through v-c-t's history and couldn't find an obvious commit that would have fixed things. There were no bugs or commits related to MQ and MozReview-Commit-ID not working :/

Comment 5

2 years ago
Addressing bug 1295338 will fix some of the issues in MozReview. But there is sufficient complexity in the reported scenario that bug 1295338 won't address unless it is scope bloated. Specifically the ability to store MozReview-Commit-ID from discarded review requests and revive them if an old MozReview-Commit-ID re-appears.
(Reporter)

Comment 6

2 years ago
(In reply to Gregory Szorc [:gps] from comment #4)
> There was another instance a month back of someone with an older
> version-control-tools checkout whose MQ didn't produce/preserve
> MozReview-Commit-ID.

That was jyc, I think (rendering intern).

I just discovered today that Fariskhi (another rendering intern) had the exact same problem as jyc & hiro -- his patches in MQ had no MozReview-Commit-ID hardcoded in each patch. They fixed themselves (gaining hardcoded MozReview-Commit-IDs) after he ran ./mach mercurial-setup and then "hg qref -e" on each patch.  So indeed, there must've been something that was fixed between ~June and now, which ./mach mercurial-setup pulls in & which gets these commit IDs into patches.
Today I realized that MozReview-Commit-ID is still missing when I did use hg qnew without -m option.
(Reporter)

Comment 8

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Today I realized that MozReview-Commit-ID is still missing when I did use hg
> qnew without -m option.

(Yeah, I think that's expected.  MozReview-Commit-ID is part of the commit message, so it's reasonable that it only gets autogenerated when you edit [or create] the commit message.  It shouldn't be too much of a problem that it doesn't get generated before that, because people should be adding commit messages before they push to mozreview.)
You need to log in before you can comment on or make changes to this bug.