mozreview: map commits to review requests based on commit ID (
bug 1245589); r?dminor
Now that clients report a commit identifier in commit messages, we can
now use that identifier to map commits to existing review requests!
Our new code simply iterates over available review requests and maps
incoming commits to a review request if both share the same commit ID.
There is minimal test fallout, as apparently we don't have much test
coverage over the impact of history rewriting on review request mapping.
The docs have been updated to reflect that commit mapping is no longer
extremely dumb.
Now that this is implemented, I'm very tempted to change the review
request recycling logic further. I can argue we don't need to re-use
unclaimed review requests any more because the mapping is good enough.
However, I'll need to think about this a bit more before I commit to
that. Baby steps.
MozReview-Commit-ID: IF9JojPZBmS
--- a/docs/mozreview/commits.rst
+++ b/docs/mozreview/commits.rst
@@ -403,23 +403,16 @@ Nobody is perfect. Reviewers will inevit
something in your code and re-submit the review request. How should
you do this?
The process for updating review requests with new commits is exactly
the same as submitting new commits. If all goes according to plan,
your rewritten commits map up to their previous versions and the
reviewer sees the new diffs!
-.. danger::
-
- There are some scenarios where updates to existing commits don't
- map cleanly to existing review requests. This can result in a
- horrible experience that will make you contemplate not using
- MozReview. Read on for details.
-
Understanding How Commits are Mapped to Review Requests
-------------------------------------------------------
A review request in MozReview tracks the evolution of a single logical
commit in version control. For example, say you have 3 commits:
1. Implement foo
2. Implement bar
@@ -441,70 +434,32 @@ Distributed version control tools (excep
evolution - more on this later) don't really track logical commits -
they track content. In reality, commits are represented as SHA-1
hashes of their content. In effect, commit identifiers are random.
This means that tracking the same logical commit (e.g. *Implement foo*)
through history rewriting is a non-exact science. It must be based on
heuristics (such as commit message or diff similarity) or some other
tracking mechanism.
-Mercurial's experimental changeset evolution feature is one such
-tracking mechanism. Unlike vanilla Mercurial or Git, this feature
-records that commit Y is a logical successor to commit X. Basically,
-when commit X is seen, Mercurial knows that Y is a newer version of it
-and should be used instead.
-
-.. important::
-
- MozReview doesn't yet make an attempt to intelligently map old
- commits to their new versions using heuristics.
+This can be problematic for MozReview because it is important for the
+same logical commit to consistently get mapped to the same review
+request. If this doesn't work, a review request could shift its focus
+to completely different commits during its lifetime!
- This means that reordering, inserting, or dropping commits can
- result in review requests getting mapped to different logical
- commits.
-
-Unless you have Mercurial's changeset evolution feature enabled,
-the behavior of MozReview to map commits to review requests is
-very simple: review requests and commits are paired in their existing
-topological order.
-
-For example, you have 2 commits, X and Y. These are allocated
-review requests 20 and 21 when first submitted. You make updates to both
-and re-submit. X' is allocated to 20 and Y' to 21. Nothing unexpected there.
-
-Now you add a new commit, Z. When you submit the series of X', Y', Z,
-X' is allocated to 20, Y' to 21, and a new review request - 22 - is created
-for Z. This also *just works*.
+MozReview has a mechanism for mapping incoming commits to existing review
+requests. It looks something like the following:
-But what happens when you remove X'? We now have Y'' and Z'. Since 20 is
-the first available review request, Y'' goes to 20 and Z' goes to 21. 22
-is left unused and is discarded. Comments about X and X' linger on 20,
-which is now tracking Y''. This is horribly sub-optimal.
-
-Avoiding Pitfall When Rewriting History
----------------------------------------
-
-As the above section demonstrates, the lack of heuristics for
-mapping logical commits to existing review requests can result in
-badness.
-
-While this sounds like a massive deficiency in MozReview, in practice
-people tend to learn to work around it by not practicing workflows that
-result in things getting in a wonky state. (Again, yes, we need to
-implement the heuristics so the tracking is better.)
+1. Obtain all review requests currently associated with the group of commits
+ being altered.
+2. Map exact commit SHA-1 matches to existing review requests (i.e. if
+ the commit didn't change and a review request is already on file, use it)
+3. Map commits using Mercurial's obsolescence data. If a Mercurial client
+ says it rewrote commit ``X`` to ``Y`` and a review request for ``X``
+ exists, map ``Y`` to that existing review request.
+4. Map commits according to matching ``MozReview-Commit-ID`` annotations.
+ The Mercurial and Git MozReview clients automatically add unique
+ *commit ID* annotations to commit messages. If an incoming commit has the
+ same *commit ID* as an existing review request, use that review request.
+5. Recycle existing review requests or create new review requests as needed.
-The easiest way to avoid issues with history rewriting is to use
-Mercurial's changeset evolution feature by installing the
-`evolve extension <https://bitbucket.org/marmoute/mutable-history>`_.
-The tracking it performs *just works*. It will change your Mercurial
-workflow significantly and will likely be a bit confusing initially.
-But people tend to love it once they get over the steep learning
-curve (it's no worse than Git's learning curve).
-
-If you don't want to use Mercurial + changeset evolution, you can work
-around the limitation by not inserting or removing commits at the
-beginning or middle of a commit series. As long as the offsets of commits
-within a series remain constant, MozReview will keep mapping commits
-to the same review request. If you keep adding commits to the end, these
-will be dealt with properly as well.
-
-Of course, commit series consisting of a single commit don't have any
-issues with offset remapping, so there is nothing to worry about there.
+In most scenarios, the commit mapping mechanism should *just work*. If
+you find a scenario where commits are being mapped to incorrect
+review requests, please file a bug.
--- a/hgext/reviewboard/tests/test-commits-deleted-no-obsolescence.t
+++ b/hgext/reviewboard/tests/test-commits-deleted-no-obsolescence.t
@@ -264,50 +264,49 @@ Review 6 should be marked as discarded
- '--- /dev/null'
- +++ b/foo5
- '@@ -0,0 +1,1 @@'
- +foo5
- ''
approved: false
approval_failure: A suitable reviewer has not given a "Ship It!"
-Dropping the first commit should shuffle all the reviews down the line.
-NOTE: If we ever employ heuristic matching on the server, this test
-likely gets invalidated.
+Dropping the first commit should have no effect since commit ids map to
+appropriate review requests
$ hg -q rebase -s 2 -d 0
$ hg strip -r 1 --no-backup
$ hg push
pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 0 changes to 3 files (+1 heads)
remote: recorded push in pushlog
submitting 3 changesets for review
changeset: 1:eeb6d49dcb09
summary: Bug 1 - Foo 2
- review: http://$DOCKER_HOSTNAME:$HGPORT1/r/2 (draft)
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/3 (draft)
changeset: 2:607f375f35c0
summary: Bug 1 - Foo 3
- review: http://$DOCKER_HOSTNAME:$HGPORT1/r/3 (draft)
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/4 (draft)
changeset: 3:81ee86efd38f
summary: Bug 1 - Foo 4
- review: http://$DOCKER_HOSTNAME:$HGPORT1/r/4 (draft)
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/5 (draft)
review id: bz://1/mynick
review url: http://$DOCKER_HOSTNAME:$HGPORT1/r/1 (draft)
(review requests lack reviewers; visit review url to assign reviewers)
(visit review url to publish these review requests so others can see them)
-The first commit was rewritten (we assume all subsequent were as well).
+The review request corresponding to the dropped commit has no draft
$ rbmanage dumpreview 2
id: 2
status: pending
public: true
bugs:
- '1'
commit: null
@@ -337,52 +336,18 @@ The first commit was rewritten (we assum
- new file mode 100644
- '--- /dev/null'
- +++ b/foo1
- '@@ -0,0 +1,1 @@'
- +foo1
- ''
approved: false
approval_failure: A suitable reviewer has not given a "Ship It!"
- draft:
- bugs:
- - '1'
- commit: null
- summary: Bug 1 - Foo 2
- description:
- - Bug 1 - Foo 2
- - ''
- - 'MozReview-Commit-ID: 5ijR9k'
- target_people: []
- extra:
- calculated_trophies: true
- commit_extra_data:
- p2rb: true
- p2rb.commit_id: eeb6d49dcb0950d771959358f662cf2e5ddc9dc1
- p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
- p2rb.identifier: bz://1/mynick
- p2rb.is_squashed: false
- diffs:
- - id: 9
- revision: 2
- base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
- name: diff
- extra: {}
- patch:
- - diff --git a/foo2 b/foo2
- - new file mode 100644
- - '--- /dev/null'
- - +++ b/foo2
- - '@@ -0,0 +1,1 @@'
- - +foo2
- - ''
-The last review request that got invalidated in the shuffle should
-be in the list of review requests to discard when the squashed review
-request is published.
+Review request 2 should be marked as discard on publish
$ rbmanage dumpreview 1
id: 1
status: pending
public: true
bugs:
- '1'
commit: bz://1/mynick
@@ -394,17 +359,17 @@ request is published.
calculated_trophies: true
p2rb.reviewer_map: '{}'
commit_extra_data:
p2rb: true
p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
p2rb.commits: '[["0b3e14fe3ff19019110705e72dcf563c0ef551f6", 2], ["bce658a3f6d6aa04bf5c449e0e779e839de4690e",
3], ["713878e22d952d478e88bfdef897fdfc73060351", 4], ["4d0f846364eb509a1b6ae3294f05439101f6e7d3",
5]]'
- p2rb.discard_on_publish_rids: '[5]'
+ p2rb.discard_on_publish_rids: '[2]'
p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
p2rb.identifier: bz://1/mynick
p2rb.is_squashed: true
p2rb.unpublished_rids: '[]'
diffs:
- id: 1
revision: 1
base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
@@ -483,18 +448,18 @@ request is published.
description: This is the parent review request
target_people: []
extra:
calculated_trophies: true
p2rb.reviewer_map: '{"3": [], "2": [], "5": [], "4": []}'
commit_extra_data:
p2rb: true
p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
- p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 2], ["607f375f35c0866a8e08bc1d6aaecc6ad259ed6e",
- 3], ["81ee86efd38ff60717aeeeff153292e84e58be0b", 4]]'
+ p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["607f375f35c0866a8e08bc1d6aaecc6ad259ed6e",
+ 4], ["81ee86efd38ff60717aeeeff153292e84e58be0b", 5]]'
p2rb.discard_on_publish_rids: '[]'
p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
p2rb.identifier: bz://1/mynick
p2rb.is_squashed: true
p2rb.unpublished_rids: '[]'
diffs:
- id: 8
revision: 3
@@ -539,21 +504,377 @@ Try removing a commit in the middle.
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 0 changes to 1 files (+1 heads)
remote: recorded push in pushlog
submitting 2 changesets for review
changeset: 1:eeb6d49dcb09
summary: Bug 1 - Foo 2
- review: http://$DOCKER_HOSTNAME:$HGPORT1/r/2
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/3
changeset: 2:a27a94c54524
summary: Bug 1 - Foo 4
- review: http://$DOCKER_HOSTNAME:$HGPORT1/r/3 (draft)
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/5 (draft)
review id: bz://1/mynick
review url: http://$DOCKER_HOSTNAME:$HGPORT1/r/1 (draft)
(review requests lack reviewers; visit review url to assign reviewers)
(visit review url to publish these review requests so others can see them)
+The parent review should have been updated accordingly
+
+ $ rbmanage dumpreview 1
+ id: 1
+ status: pending
+ public: true
+ bugs:
+ - '1'
+ commit: bz://1/mynick
+ submitter: default+5
+ summary: bz://1/mynick
+ description: This is the parent review request
+ target_people: []
+ extra_data:
+ calculated_trophies: true
+ p2rb.reviewer_map: '{"3": [], "2": [], "5": [], "4": []}'
+ commit_extra_data:
+ p2rb: true
+ p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["607f375f35c0866a8e08bc1d6aaecc6ad259ed6e",
+ 4], ["81ee86efd38ff60717aeeeff153292e84e58be0b", 5]]'
+ p2rb.discard_on_publish_rids: '[4]'
+ p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.identifier: bz://1/mynick
+ p2rb.is_squashed: true
+ p2rb.unpublished_rids: '[]'
+ diffs:
+ - id: 1
+ revision: 1
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo1 b/foo1
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo1
+ - '@@ -0,0 +1,1 @@'
+ - +foo1
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo3 b/foo3
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo3
+ - '@@ -0,0 +1,1 @@'
+ - +foo3
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - diff --git a/foo5 b/foo5
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo5
+ - '@@ -0,0 +1,1 @@'
+ - +foo5
+ - ''
+ - id: 7
+ revision: 2
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo1 b/foo1
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo1
+ - '@@ -0,0 +1,1 @@'
+ - +foo1
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo3 b/foo3
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo3
+ - '@@ -0,0 +1,1 @@'
+ - +foo3
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - ''
+ - id: 8
+ revision: 3
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo3 b/foo3
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo3
+ - '@@ -0,0 +1,1 @@'
+ - +foo3
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - ''
+ approved: false
+ approval_failure: Commit eeb6d49dcb0950d771959358f662cf2e5ddc9dc1 is not approved.
+ draft:
+ bugs:
+ - '1'
+ commit: bz://1/mynick
+ summary: bz://1/mynick
+ description: This is the parent review request
+ target_people: []
+ extra:
+ calculated_trophies: true
+ p2rb.reviewer_map: '{"3": [], "5": [], "4": []}'
+ commit_extra_data:
+ p2rb: true
+ p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["a27a94c54524d4331dec2f92f647067bfd6dfbd4",
+ 5]]'
+ p2rb.discard_on_publish_rids: '[]'
+ p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.identifier: bz://1/mynick
+ p2rb.is_squashed: true
+ p2rb.unpublished_rids: '[]'
+ diffs:
+ - id: 12
+ revision: 4
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - ''
+
+Deleting a commit and adding a commit will recycle a review request
+because of an unclaimed review request
+TODO this is sub-optimal. We should consider changing the review request
+recycling behavior when commit IDs are present.
+
+ $ hg -q up eeb6d49dcb09
+
+ $ echo foo6 > foo6
+ $ hg -q commit -A -m 'Bug 1 - Foo 6'
+ $ hg --config mozreview.autopublish=true push
+ pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
+ searching for changes
+ remote: adding changesets
+ remote: adding manifests
+ remote: adding file changes
+ remote: added 1 changesets with 1 changes to 1 files (+1 heads)
+ remote: recorded push in pushlog
+ submitting 2 changesets for review
+
+ changeset: 1:eeb6d49dcb09
+ summary: Bug 1 - Foo 2
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/3
+
+ changeset: 3:3b99865d1bab
+ summary: Bug 1 - Foo 6
+ review: http://$DOCKER_HOSTNAME:$HGPORT1/r/4 (draft)
+
+ review id: bz://1/mynick
+ review url: http://$DOCKER_HOSTNAME:$HGPORT1/r/1 (draft)
+ (review requests lack reviewers; visit review url to assign reviewers)
+ (visit review url to publish these review requests so others can see them)
+
+ $ rbmanage dumpreview 1
+ id: 1
+ status: pending
+ public: true
+ bugs:
+ - '1'
+ commit: bz://1/mynick
+ submitter: default+5
+ summary: bz://1/mynick
+ description: This is the parent review request
+ target_people: []
+ extra_data:
+ calculated_trophies: true
+ p2rb.reviewer_map: '{"3": [], "2": [], "5": [], "4": []}'
+ commit_extra_data:
+ p2rb: true
+ p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["607f375f35c0866a8e08bc1d6aaecc6ad259ed6e",
+ 4], ["81ee86efd38ff60717aeeeff153292e84e58be0b", 5]]'
+ p2rb.discard_on_publish_rids: '[5]'
+ p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.identifier: bz://1/mynick
+ p2rb.is_squashed: true
+ p2rb.unpublished_rids: '[]'
+ diffs:
+ - id: 1
+ revision: 1
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo1 b/foo1
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo1
+ - '@@ -0,0 +1,1 @@'
+ - +foo1
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo3 b/foo3
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo3
+ - '@@ -0,0 +1,1 @@'
+ - +foo3
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - diff --git a/foo5 b/foo5
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo5
+ - '@@ -0,0 +1,1 @@'
+ - +foo5
+ - ''
+ - id: 7
+ revision: 2
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo1 b/foo1
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo1
+ - '@@ -0,0 +1,1 @@'
+ - +foo1
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo3 b/foo3
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo3
+ - '@@ -0,0 +1,1 @@'
+ - +foo3
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - ''
+ - id: 8
+ revision: 3
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo3 b/foo3
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo3
+ - '@@ -0,0 +1,1 @@'
+ - +foo3
+ - diff --git a/foo4 b/foo4
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo4
+ - '@@ -0,0 +1,1 @@'
+ - +foo4
+ - ''
+ approved: false
+ approval_failure: Commit eeb6d49dcb0950d771959358f662cf2e5ddc9dc1 is not approved.
+ draft:
+ bugs:
+ - '1'
+ commit: bz://1/mynick
+ summary: bz://1/mynick
+ description: This is the parent review request
+ target_people: []
+ extra:
+ calculated_trophies: true
+ p2rb.reviewer_map: '{"3": [], "5": []}'
+ commit_extra_data:
+ p2rb: true
+ p2rb.base_commit: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.commits: '[["eeb6d49dcb0950d771959358f662cf2e5ddc9dc1", 3], ["3b99865d1bab8480235d913f4bcfc951fd9e3032",
+ 4]]'
+ p2rb.discard_on_publish_rids: '[]'
+ p2rb.first_public_ancestor: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ p2rb.identifier: bz://1/mynick
+ p2rb.is_squashed: true
+ p2rb.unpublished_rids: '[]'
+ diffs:
+ - id: 14
+ revision: 4
+ base_commit_id: 93d9429b41ecf0d2ad8c62b6ea26686dd20330f4
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo2 b/foo2
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo2
+ - '@@ -0,0 +1,1 @@'
+ - +foo2
+ - diff --git a/foo6 b/foo6
+ - new file mode 100644
+ - '--- /dev/null'
+ - +++ b/foo6
+ - '@@ -0,0 +1,1 @@'
+ - +foo6
+ - ''
+
$ mozreview stop
stopped 9 containers
--- a/pylib/mozreview/mozreview/resources/batch_review_request.py
+++ b/pylib/mozreview/mozreview/resources/batch_review_request.py
@@ -13,16 +13,19 @@ from djblets.webapi.decorators import (
)
from djblets.webapi.errors import (
INVALID_FORM_DATA,
LOGIN_FAILED,
NOT_LOGGED_IN,
PERMISSION_DENIED,
SERVICE_NOT_CONFIGURED,
)
+from mozautomation.commitparser import (
+ parse_commit_id,
+)
from reviewboard.accounts.backends import (
get_enabled_auth_backends,
)
from reviewboard.diffviewer.models import (
DiffSet,
)
from reviewboard.reviews.models import (
ReviewRequest,
@@ -535,16 +538,73 @@ class BatchReviewRequestResource(WebAPIR
pass
break
logger.info('%s: %d/%d mapped exactly or to precursors' % (
identifier, len(processed_nodes),
len(commits['individual'])))
+ # Clients should add "MozReview-Commit-ID" unique identifiers to
+ # commit messages. Search for them and match up accordingly.
+
+ unclaimed_rrs = [ReviewRequest.objects.get(pk=rid)
+ for rid in unclaimed_rids]
+
+ for commit in commits['individual']:
+ node = commit['id']
+ if node in processed_nodes:
+ continue
+
+ commit_id = parse_commit_id(commit['message'])
+ if not commit_id:
+ logger.warn('%s: commit %s does not have commit id' % (
+ identifier, node))
+ continue
+
+ for rr in unclaimed_rrs:
+ rr_commit_id = parse_commit_id(rr.description)
+ if commit_id != rr_commit_id:
+ continue
+
+ # commit ID in commit found in existing review request. Map
+ # it up.
+ logger.info('%s: commit ID %s for %s found in review request %d' % (
+ identifier, commit_id, node, rr.id))
+
+ try:
+ del remaining_nodes[node]
+ except KeyError:
+ pass
+
+ unclaimed_rids.remove(rr.id)
+ unclaimed_rrs.remove(rr)
+ draft, warns = update_review_request(local_site, request,
+ privileged_user,
+ reviewer_cache, rr,
+ commit)
+ squashed_reviewers.update(u for u in draft.target_people.all())
+ warnings.extend(warns)
+ processed_nodes.add(node)
+ node_to_rid[node] = rr.id
+ review_requests[rr.id] = rr
+ review_data[rr.id] = get_review_request_data(rr)
+ try:
+ discard_on_publish_rids.remove(rr.id)
+ except ValueError:
+ pass
+
+ break
+
+ logger.info('%s: %d/%d mapped after commit ID matching' % (
+ identifier, len(processed_nodes),
+ len(commits['individual'])))
+
+ logger.info('%s: %d unclaimed review requests' % (identifier, len(unclaimed_rids)))
+
# Now do a pass over the commits that didn't map cleanly.
for commit in commits['individual']:
node = commit['id']
if node in processed_nodes:
continue
# We haven't seen this commit before *and* our mapping above didn't
# do anything useful with it.