1160479 - Autoland should rewrite reviewers in the commit summary based upon data from MozReview when landing

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

[Dan Minor [:dminor]]

When landing to inbound, Autoland should automatically add reviewers to the commit summary if none are specified by the user. This information is readily available from mozreview and could be passed in as part of the autoland request.

Comment 1

[Gregory Szorc [:gps]]

How about Autoland strips r= lines and re-adds the annotation based on what's in MozReview? That way, we can "trust" that r= annotations traversing through Autoland are valid.

I'm sure there are workflows where this breaks (e.g. IRL review). But are those workflows going through MozReview today or tomorrow? Could we get away with the "radical" behavior for now?

I side-effect of this is that reviewer annotations will be consistently formatted. As it stands, parsing the N variations of review annotation is more complicated than it needs to be.

Comment 3

[Blake Wu [:bwu][:blakewu]]

Dan, 
Thanks for this bug!

Comment 4

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: accept commit_descriptions as part of autoland request (bug 1160479) r=glob

autoland: accept commit_descriptions as part of autoland request (bug 1160479) r?glob

This adds a new field for commit_descriptions to the json autoland request.
Since it is an optional field this only updates the documentation to indicate
that the field exists.

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r=gps

autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r?gps

This adds an mercurial extension that uses pylib/hgext/mozhg/mozhg/rewrite.py
to rewrite commit descriptions based upon a mercurial file. We use an
extension for now rather than importing it directly to avoid having to change
the Autoland license. At some point we might want to GPL Autoland and rewrite
all of the transplant functionality to call into mercurial directly.

Comment 6

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: rewrite commit descriptions (bug 1160479) r=glob

autoland: rewrite commit descriptions (bug 1160479) r?glob

If given commit descriptions in the autoland request, this will use the
rewritecommitdescription extension to rewrite the descriptions as requested.

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8681340 [details]
MozReview Request: autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r=gps

autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r?gps

This adds an mercurial extension that uses pylib/hgext/mozhg/mozhg/rewrite.py
to rewrite commit descriptions based upon a mercurial file. We use an
extension for now rather than importing it directly to avoid having to change
the Autoland license. At some point we might want to GPL Autoland and rewrite
all of the transplant functionality to call into mercurial directly.

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8681341 [details]
MozReview Request: autoland: rewrite commit descriptions (bug 1160479) r=glob

autoland: rewrite commit descriptions (bug 1160479) r?glob

If given commit descriptions in the autoland request, this will use the
rewritecommitdescription extension to rewrite the descriptions as requested.

Comment 9

[:glob ✱]

Comment on attachment 8681339 [details]
MozReview Request: autoland: accept commit_descriptions as part of autoland request (bug 1160479) r=glob

https://reviewboard.mozilla.org/r/23801/#review21453

lgtm

Comment 10

[:glob ✱]

Comment on attachment 8681341 [details]
MozReview Request: autoland: rewrite commit descriptions (bug 1160479) r=glob

https://reviewboard.mozilla.org/r/23805/#review21461

lgtm

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8681340 [details]
MozReview Request: autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r=gps

https://reviewboard.mozilla.org/r/23803/#review21885

::: autoland/hgext/rewritecommitdescriptions.py:10
(Diff revision 2)
> +# sigh, more path hackery
> +sys.path.insert(0, os.path.normpath(os.path.join(os.path.normpath(
> +    os.path.abspath(os.path.dirname(__file__))), '..', '..',
> +    'pylib', 'mozhg')))

https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard/client.py#l49 is typically used to configure the Python environment from extensions running directly from version-control-tools checkouts.

::: autoland/hgext/rewritecommitdescriptions.py:17
(Diff revision 2)
> +def rewrite_commit_descriptions(ui, repo, node, **opts):

Use `json=None` and remove the `**opts`: we know what the arguments are going to be, there is no need to use a wildcard.

::: autoland/hgext/rewritecommitdescriptions.py:36
(Diff revision 2)
> +        revised_description = descriptions.get(sha1, description)

I'm pretty sure there is a Unicode bug lingering here.

Mercurially internally uses a class `mercurial.encoding.localstr`. This is a child of `str`. It uses as its value the passed in value converted to the locally-defined encoding. So if the local encoding is ASCII and it receives a str with non-ASCII bytes, you could get a value like "My Nam?" with literal question marks where the local encoding could not represent a code point.

The `localstr` class also stores a copy of the unmodified passed in value. You can access it by calling `mercurial.encoding.fromlocal()`.

Anyway, IIRC json.load() returns `unicode` (not `str`) instances. Mercurial doesn't use `unicode` anywhere internally. So this is a bug.

It is also likely a bug that we're using a `str`/`unicode` instead of `localstr` when representing the `description` variable.

So, this probably needs to be something like:

if sha1 in descriptions:
    description = encoding.tolocal(descriptions[sha1].encode('utf-8'))
else:
    description = ctx.description()
    
There should definitely be an automated test around Unicode handling to verify it works as expected. There have certainly been a number of Unicode bugs in MozReview due to improper user or description handling.

::: autoland/hgext/rewritecommitdescriptions.py:45
(Diff revision 2)
> +        description = descriptions.get(sha1, ctx.description)

This should be `ctx.description()`.

::: autoland/hgext/rewritecommitdescriptions.py:59
(Diff revision 2)
> +cmdtable = {
> +    'rewritecommitdescriptions': (rewrite_commit_descriptions,
> +                                  [('', 'json', '',
> +                                    'path to json file with new commit descriptions',
> +                                    'string')],
> +                                  '[options] REV'),
> +}

If you cargo culted this from version-control-tools, there is some cleanup that needs to be performed :)

The proper way to declare commands is to use a decorator. See https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard/client.py#l79 and https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard/client.py#l988 for an example.

::: autoland/hgext/rewritecommitdescriptions.py:64
(Diff revision 2)
> +                                  '[options] REV'),

Pretty sure you don't need `[options]` here.

::: autoland/hgext/rewritecommitdescriptions.py:67
(Diff revision 2)
> +testedwith = '3.5'

This should be the first thing after import statements.

Comment 12

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/23803/#review21885

> If you cargo culted this from version-control-tools, there is some cleanup that needs to be performed :)
> 
> The proper way to declare commands is to use a decorator. See https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard/client.py#l79 and https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard/client.py#l988 for an example.

Heh, cargo culted from the mercurial wiki: https://www.mercurial-scm.org/wiki/WritingExtensions. Should have known better than to read a wiki :)

Comment 13

[Dan Minor [:dminor]]

Comment on attachment 8681339 [details]
MozReview Request: autoland: accept commit_descriptions as part of autoland request (bug 1160479) r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23801/diff/1-2/

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8681340 [details]
MozReview Request: autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23803/diff/2-3/

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8681341 [details]
MozReview Request: autoland: rewrite commit descriptions (bug 1160479) r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23805/diff/2-3/

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8681340 [details]
MozReview Request: autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r=gps

https://reviewboard.mozilla.org/r/23803/#review22261

::: autoland/hgext/rewritecommitdescriptions.py:10
(Diff revision 3)
> +import json
> +import os
> +import sys

Please put stdlib imports before mercurial imports.

::: autoland/hgext/rewritecommitdescriptions.py:43
(Diff revision 3)
> +    with open(descriptions) as f:

Probably best to throw an explicit 'rb' in here.

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 8681340 [details]
MozReview Request: autoland: add hg extension to rewrite commit descriptions based on json file (bug 1160479) r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23803/diff/3-4/

Comment 18

[Dan Minor [:dminor]]

Comment on attachment 8681341 [details]
MozReview Request: autoland: rewrite commit descriptions (bug 1160479) r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23805/diff/3-4/