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

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Andreas Pehrson [:pehrsons]]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

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

Comment 2

[Gregory Szorc [:gps]]

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.

Comment 3

[Andreas Pehrson [:pehrsons]]

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?

Comment 4

[Gregory Szorc [: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.

Comment 5

[Gregory Szorc [:gps]]

I think this is a low hanging fruit.

Comment 6

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: add test for review request recycling (bug 1242246); r?dminor

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/

Comment 7

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: report whether obsolescence is enabled (bug 1242246); r?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/

Comment 8

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: don't reuse review requests if commit is new (bug 1242246); r?dminor

MozReview-Commit-ID: LRi48ckMsWq

Review commit: https://reviewboard.mozilla.org/r/33413/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33413/

Comment 9

[Gregory Szorc [:gps]]

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 10

[Gregory Szorc [:gps]]

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 11

[Gregory Szorc [:gps]]

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 12

[Gregory Szorc [:gps]]

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

[Dan Minor [:dminor]]

Comment on attachment 8715412 [details]
MozReview Request: reviewboard: report whether obsolescence is enabled (bug 1242246); r?dminor

https://reviewboard.mozilla.org/r/33411/#review30575

Comment 14

[Dan Minor [:dminor]]

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

Comment 15

[Dan Minor [:dminor]]

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

Comment 16

[Gregory Szorc [:gps]]

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