1128977 - Annotate mozreview commits with review metadata automatically

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect, P2)

Description

[Dan Minor [:dminor]]

From Bug 1086645 comment 3:

Something that would make the mapping problem easier is if we had the review request ID in every commit. We probably should start annotating commit messages with the MozReview review request ID or URL. We probably should have started doing this yesterday. A good place to start would be to have the Mercurial extension rewrite commit messages automatically. Then, when pushing, we can extract the review request out of the commit and target a special review request for closing.

My understanding of the intention here is to append the review request id/url to the commit on the hg server during the interaction with reviewboard.

I've heard an upcoming feature of mercurial is to be able to rewrite commits and push these back to the client, I'm not sure if this is something we want here. I'm assuming given the number of older version hg users and git client users we have this is a non-starter.

Comment 1

[Dan Minor [:dminor]]

I'm interested in picking this one up (assuming it's not the visible portion of another insurmountable iceberg.)

Comment 2

[Gregory Szorc [:gps]]

Yes, we should be doing this yesterday.

We really should aim to have the rewrite exposed to the client. This makes it much, much easier to associate changesets to review requests. It also allows clients not using evolve to get nice history rewriting support.

Exposing the rewrite to the client could be "fun." We can do this with bundle2 and the magical server-side rewrite, but that's not until Mercurial 3.4 in 3 months. In the interim, we may want to do some kind of 2 phase push: have the client reserve review request IDs, rewrite locally, then actually push to the server. This could be an iceberg.

Comment 3

[Dan Minor [:dminor]]

Since we already have a concept of unclaimed_rid in the postreviews code, my plan of attack is to add a 'reservereviews' protocol that ensures there are sufficient unclaimed_rids by creating new draft reviews as necessary and then returning a list of unclaimed review ids to use.

I have a WIP (with a hideous amount of code duplication) with this working, but I wanted to get feedback on the approach before proceeding.

Comment 4

[Gregory Szorc [:gps]]

Oooo - I forgot about that unclaimed_rid foo. I see where you are going and I think it could work!

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: bz://1128977/dminor

/r/3653 - hgrb: Create functions for code that is shared between pushing reviews and reserving review ids (bug 1128977); r=gps

Pull down this commit:

hg pull review -r 033a74796e7933c7f300e82c96f50edb1bf215f9

Comment 6

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3653/#review2927

Review Board pretty much reviewed this for me with its automagical "moved from" rendering. This is seriously one of the nicest review aids I've ever seen. I almost didn't have to think.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +    # A mapping from previously pushed node, which has not been processed
> +    # yet, to the review request id associated with that node.

These comments should probably be docstrings now. But it's fine to leave as is if you are feeling lazy :)

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> -    # but have not been published. If this list contains an item, it should be
> +    remaining_nodes = get_remaining_nodes(previous_commits) 

Nit: trailing whitespace

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8562164 [details]
MozReview Request: bz://1128977/dminor

https://reviewboard.mozilla.org/r/3651/#review2929

Ship It!

Comment 8

[Dan Minor [:dminor]]

I'm hitting a few tests that fail due to changes in the diff id, like the following:

--- /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-added.t
+++ /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-added.t.err
@@ -108,7 +108,7 @@
       p2rb.is_squashed: true
       p2rb.unpublished_rids: '[]'
     diffs:
-    - id: 4
+    - id: 6
       revision: 1
       base_commit_id: null
       patch:

I'm having trouble determining if this is a significant change or not. I've tried adding asserts in various places and I'm not seeing extra diffs being created. Because the review ids match the expected results, I don't think this is due to extra reviews being created.

Comment 9

[Dan Minor [:dminor]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/6170be263a0d

Comment 10

[Dan Minor [:dminor]]

(In reply to Dan Minor [:dminor] from comment #8)
> I'm hitting a few tests that fail due to changes in the diff id, like the
> following:
> 
> ---
> /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-
> added.t
> +++
> /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-commits-
> added.t.err
> @@ -108,7 +108,7 @@
>        p2rb.is_squashed: true
>        p2rb.unpublished_rids: '[]'
>      diffs:
> -    - id: 4
> +    - id: 6
>        revision: 1
>        base_commit_id: null
>        patch:
> 
> I'm having trouble determining if this is a significant change or not. I've
> tried adding asserts in various places and I'm not seeing extra diffs being
> created. Because the review ids match the expected results, I don't think
> this is due to extra reviews being created.

I was uploading a unnecessary draft which was causing the diff ids to be off by one. Later code depended upon this to ensure a draft review was available or it would error out. I've changed that spot from get_draft to get_or_create_draft and removed the unnecessary draft upload.

Comment 11

[Dan Minor [:dminor]]

Final test failure to investigate - in test-auth.t I'm missing some fields in dump-user. I've added a dump-auth command which dumps the auth table and everything seems to be fine there. I'll investigate further tomorrow.

--- /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-auth.t
+++ /home/dminor/src/version-control-tools/hgext/reviewboard/tests/test-auth.t.err
@@ -127,6 +127,9 @@
 Usernames for users without the IRC nick syntax are based on email fragment and BZ user id
 
   $ rbmanage dump-auth
+  (1, u'!', 0, u'admin+1', u'Admin', u'', u'[e-mail]', 0, 1)
+  (2, u'!', 0, u'user1+5', u'Dummy User1', u'', u'[e-mail]', 0, 1)
+  (3, u'!', 0, u'nick', u'Mozila User [:nick]', u'', u'[e-mail]', 0, 1)
   $ rbmanage dump-user $HGPORT1 'user1+5'
   2:
     avatar_url: http://www.gravatar.com/avatar/* (glob)
-    email: [e-mail]
-    first_name: Dummy User1
-    fullname: Dummy User1
     id: 2
-    last_name: ''
     url: /users/user1%2B5/
     username: user1+5
 
@@ -143,11 +142,7 @@
   $ rbmanage dump-user $HGPORT1 nick
   3:
     avatar_url: http://www.gravatar.com/avatar/* (glob)
-    email: [e-mail]
-    first_name: Mozila User [:nick]
-    fullname: Mozila User [:nick]
-    id: 3
-    last_name: ''
+    id: 3
     url: /users/nick/
     username: nick
 

ERROR: test-auth.t output changed

Comment 12

[Dan Minor [:dminor]]

It looks like the users are being created with is_private set when my patch is applied which causes the email and name fields to be redacted from the webapi. Need to investigate further to determine why is_private is being set.

Comment 13

[Dan Minor [:dminor]]

(In reply to Dan Minor [:dminor] from comment #12)
> It looks like the users are being created with is_private set when my patch
> is applied which causes the email and name fields to be redacted from the
> webapi. Need to investigate further to determine why is_private is being set.

On closer examination, it appears that we always want is_private to be set when a user is created from bugzilla [1], but because this is on the else clause of a try/except it is only set the second time the user is accessed. My patch must cause the user to be accessed twice, so I see different results. I'll file a separate bug.

[1] https://dxr.mozilla.org/hgcustom:version-control-tools/source/pylib/rbbz/rbbz/models.py#88

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8562164 [details]
MozReview Request: bz://1128977/dminor

/r/3653 - hgrb: Create functions for code that is shared between pushing reviews and reserving review ids (bug 1128977); r=gps
/r/4101 - hgext: add reservereviews protocol to reviewboard hg extension (bug 1128977); r=gps

Pull down these commits:

hg pull review -r bac25a98881ce6c503f1a082fe6610a35379b723

Comment 15

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/4101/#review3269

I'm still wrapping my head around this feature. Here is a partial review.

::: hgext/reviewboard/hgrb/proto.py
(Diff revision 1)
> +        return identifier 

Nit: trailing whitespace

::: hgext/reviewboard/hgrb/proto.py
(Diff revision 1)
> +    for i, node in enumerate(nodes):

You don't use i here, so no need to enumerate().

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +    # Do a pass and find all commits that map cleanly to old review requests.
> +    for commit in commits['individual']:
> +        node = commit['id']
> +
> +        if node not in remaining_nodes:
> +            continue
> +
> +        # If the commit appears in an old review request, by definition of
> +        # commits deriving from content, the commit has not changed and there
> +        # is nothing to update. Update our accounting and move on.
> +        rid = remaining_nodes[node]
> +        del remaining_nodes[node]
> +        unclaimed_rids.remove(rid)
> +
> +        commit_to_rid[node] = rid
> +
> +        try:
> +            discard_on_publish_rids.remove(rid)
> +        except ValueError:
> +            pass
> +
> +    # Find commits that map to a previous version.
> +    for commit in commits['individual']:
> +        node = commit['id']
> +        if node in commit_to_rid:
> +            continue
> +
> +        # The client may have sent obsolescence data saying which commit this
> +        # commit has derived from. Use that data (if available) to try to find
> +        # a mapping to an old review request.
> +        for precursor in commit['precursors']:
> +            rid = remaining_nodes.get(precursor)
> +            if not rid:
> +                continue
> +
> +            del remaining_nodes[precursor]
> +            unclaimed_rids.remove(rid)
> +
> +            commit_to_rid[node] = rid
> +
> +            try:
> +                discard_on_publish_rids.remove(rid)
> +            except ValueError:
> +                pass
> +
> +            break
> +
> +
> +    # Finally do a pass creating new rids as necessary

I *really* don't like duplicating this code from above. This code is needlessly complex and having it defined multiple times is a recipe for disaster.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        m = review_url_rexp.search(commit['message'])

I think we should use multiline regexps and match `^Review-URL:` so we look for `Review-URL:` at the beginning of lines.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        unclaimed_rids.remove(rid)

This assumes the rid is in unclaimed_rids. I bet I can find ways to break this.

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8562164 [details]
MozReview Request: bz://1128977/dminor

/r/3653 - hgrb: Create functions for code that is shared between pushing reviews and reserving review ids (bug 1128977); r=gps
/r/4101 - hgext: add reservereviews protocol to reviewboard hg extension (bug 1128977); r=gps
/r/5035 - Address review feedback

Pull down these commits:

hg pull review -r 96def7c628d8be20f73315d00869ee4d1ca99816

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 8562164 [details]
MozReview Request: bz://1128977/dminor

/r/3653 - hgext: create review and series unique ids on the client (bug 1128977); r=gps

Pull down this commit:

hg pull review -r ca8412ac5dd481d857381d3e0a57ebe642186399

Comment 18

[Dan Minor [:dminor]]

Looks like gps is planning on doing an expanded version of this work over in bug 1160266.

Comment 19

[Gregory Szorc [:gps]]

Oh, I forgot this bug was a thing. We should probably keep this bug open to track the larger feature. Bug 1160266 is just about implementing an identifier in commits. We'll need follow-up work to integrate that into MozReview.

Comment 20

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.