Closed Bug 1128977 Opened 7 years ago Closed 3 years ago

Annotate mozreview commits with review metadata automatically

Categories

(MozReview Graveyard :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: dminor, Unassigned)

References

Details

Attachments

(1 obsolete file)

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.
I'm interested in picking this one up (assuming it's not the visible portion of another insurmountable iceberg.)
Assignee: nobody → dminor
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.
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.
Oooo - I forgot about that unclaimed_rid foo. I see where you are going and I think it could work!
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1128977/dminor (obsolete) —
/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
Attachment #8562164 - Flags: review?(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 on attachment 8562164 [details]
MozReview Request: bz://1128977/dminor

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

Ship It!
Attachment #8562164 - Flags: review?(gps) → review+
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.
(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.
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'admin@example.com', 0, 1)
+  (2, u'!', 0, u'user1+5', u'Dummy User1', u'', u'user1@example.com', 0, 1)
+  (3, u'!', 0, u'nick', u'Mozila User [:nick]', u'', u'user2@example.com', 0, 1)
   $ rbmanage dump-user $HGPORT1 'user1+5'
   2:
     avatar_url: http://www.gravatar.com/avatar/* (glob)
-    email: user1@example.com
-    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: user2@example.com
-    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
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.
(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
Depends on: 1132636
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
Attachment #8562164 - Flags: review+ → review?(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 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
Blocks: 1087462
Blocks: 1096768
Priority: P2 → P1
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
Looks like gps is planning on doing an expanded version of this work over in bug 1160266.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1160266
Attachment #8562164 - Attachment is obsolete: true
Attachment #8562164 - Flags: review?(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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Priority: P1 → P2
Assignee: dminor → nobody
Product: Developer Services → MozReview
MozReview is now obsolete. Please use Phabricator instead. Closing this bug.
Status: REOPENED → RESOLVED
Closed: 6 years ago3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.