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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pehrsons, Assigned: gps)

Tracking

Details

Attachments

(3 attachments)

Reporter

Description

3 years ago
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
Reporter

Comment 1

3 years ago
The biggest issue is that it in these cases carries over any r+ in case the reviewers stay the same.
Assignee

Comment 2

3 years ago
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.
Reporter

Comment 3

3 years ago
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)
Assignee

Comment 4

3 years ago
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)
Assignee

Updated

3 years ago
Blocks: 1243483
Assignee

Updated

3 years ago
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)
Assignee

Comment 5

3 years ago
I think this is a low hanging fruit.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Assignee

Comment 6

3 years ago
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)
Assignee

Comment 7

3 years ago
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)
Assignee

Updated

3 years ago
Blocks: 1245589
Assignee

Comment 9

3 years ago
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/
Assignee

Comment 10

3 years ago
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/
Assignee

Comment 11

3 years ago
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/
Assignee

Comment 12

3 years ago
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+
Assignee

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.