Closed Bug 1242246 Opened 8 years ago Closed 8 years ago

Review request from deleted commit is recycled for new commit (even with evolve)

Categories

(MozReview Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pehrsons, Assigned: gps)

References

Details

Attachments

(3 files)

STR:

1. Push commits 1, 2 and 3 to mozreview.
2. remove commit 2 and insert a new unrelated commit 2' between commits 1 and 3.
3. push commits 1, 2' and 3 to mozreview.

Expected result: commit 2 made obsolete, commit 2' appears as a new commit.
Actual result: mozreview thinks commit 2' is a new revision of commit 2 and renames the bugzilla entry, creates an interdiff and keeps comments and history from commit 2 (to my limited understanding based on ordering since no evolve information was available).

For examples of this, see the following comments or just below them:
bug 1208371 comment 446
bug 1208371 comment 447
bug 1208371 comment 450
bug 1208371 comment 455
bug 1208371 comment 458
bug 1208371 comment 463
The biggest issue is that it in these cases carries over any r+ in case the reviewers stay the same.
Yes, this is a known issue. Pretty sure we already have a bug on file somewhere. The problem goes away if you use Mercurial's evolution extension. We have plans to address this for non-evolve users.
My ~/.hgrc has this:

> [extensions]
> (...)
> evolve = /Users/pehrsons/Comoyo/mutable-history/hgext/evolve.py

And that mutable-history repo is at rev ed63bf62ff02. Is that not enough?
Flags: needinfo?(gps)
Even with evolve, we're not smart enough to drop the old review request and create a new one for the new logical commit. We'll recycle the review request from the dropped commit for use with a new commit. This is wrong and can lead to issues. It is a less crappy issue than the non-evolve use case, where mapping is screwed up everywhere.
Flags: needinfo?(gps)
Blocks: 1243483
Summary: MozReview sometimes screws up patch mapping if you both remove and add patches in a new revision → Review request from deleted commit is recycled for new commit (even with evolve)
I think this is a low hanging fruit.
Assignee: nobody → gps
Status: NEW → ASSIGNED
The bug report indicated a failure to map a commit when obsolescence was
enabled and a commit is deleted and a new one added. Our current
behavior is to recycle leftover review requests, which is wrong if we
know for sure the incoming commit is not logically related to the old
one.

We add a test demonstrating the buggy behavior.

MozReview-Commit-ID: LcZTmoxt6j9

Review commit: https://reviewboard.mozilla.org/r/33409/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33409/
Attachment #8715411 - Flags: review?(dminor)
When Mercurial's obsolescence feature is enabled, the algorithm mapping
commits to review requests can make stronger assertions about how
commits changed over time.

Currently, clients with obsolescence enabled may report precursor nodes
for changesets submitted for review. However, there is no way to
distinguish a submission with obsolescence disabled from one where
obsolescence is enabled but there are no precursor nodes.

This commit adds a parameter to the review request submission requests
to inform the batch review request API whether the client has
obsolescence enabled. This will be used in a subsequent commit to change
commit mapping behavior.

MozReview-Commit-ID: J2jsV1XFbpL

Review commit: https://reviewboard.mozilla.org/r/33411/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33411/
Attachment #8715412 - Flags: review?(dminor)
Blocks: 1245589
Comment on attachment 8715411 [details]
MozReview Request: reviewboard: add test for review request recycling (bug 1242246); r?dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33409/diff/1-2/
Comment on attachment 8715412 [details]
MozReview Request: reviewboard: report whether obsolescence is enabled (bug 1242246); r?dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33411/diff/1-2/
Comment on attachment 8715413 [details]
MozReview Request: mozreview: don't reuse review requests if commit is new (bug 1242246); r?dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33413/diff/1-2/
Comment on attachment 8715413 [details]
MozReview Request: mozreview: don't reuse review requests if commit is new (bug 1242246); r?dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33413/diff/2-3/
Comment on attachment 8715412 [details]
MozReview Request: reviewboard: report whether obsolescence is enabled (bug 1242246); r?dminor

https://reviewboard.mozilla.org/r/33411/#review30575
Attachment #8715412 - Flags: review?(dminor) → review+
Comment on attachment 8715411 [details]
MozReview Request: reviewboard: add test for review request recycling (bug 1242246); r?dminor

https://reviewboard.mozilla.org/r/33409/#review30577
Attachment #8715411 - Flags: review?(dminor) → review+
Comment on attachment 8715413 [details]
MozReview Request: mozreview: don't reuse review requests if commit is new (bug 1242246); r?dminor

https://reviewboard.mozilla.org/r/33413/#review30579
Attachment #8715413 - Flags: review?(dminor) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: