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)
MozReview Graveyard
General
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
Reporter | ||
Comment 1•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
I think this is a low hanging fruit.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 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•8 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 | ||
Comment 8•8 years ago
|
||
MozReview-Commit-ID: LRi48ckMsWq Review commit: https://reviewboard.mozilla.org/r/33413/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33413/
Attachment #8715413 -
Flags: review?(dminor)
Assignee | ||
Comment 9•8 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•8 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•8 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•8 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 13•8 years ago
|
||
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 14•8 years ago
|
||
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 15•8 years ago
|
||
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 | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4cc663863fcb00870bc217e4d9d5c9ee61ea19b7 reviewboard: add test for review request recycling (bug 1242246); r=dminor https://hg.mozilla.org/hgcustom/version-control-tools/rev/f67021092ae3b8fcc868f5883f181f649853f86e reviewboard: report whether obsolescence is enabled (bug 1242246); r=dminor https://hg.mozilla.org/hgcustom/version-control-tools/rev/8dc282df5d249bab7e115b44dad1cc54bcca68ad mozreview: don't reuse review requests if commit is new (bug 1242246); r=dminor
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Developer Services → MozReview
You need to log in
before you can comment on or make changes to this bug.
Description
•