1220232 - UI to confirm rewritten commit message

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mark Côté [:mcote]]

Since there is no universal standard for commit messages right now, we'll be using a best-effort approach to writing the reviewers in the commit message upon autolanding.  We'll need to confirm the rewritten message with the user for each commit in the series (ideally all in one dialog).  If the user rejects the rewritten message, autoland will not be performed, and the user will be instructed to either land manually or rewrite the commit message, push it back up to MozReview, and try autolanding again.

We'll just need to confirm the first line, since that's all that should be rewritten.

Eventually we can try doing something more interesting, like allowing the commit message to be edited directly, but for now we can use something simple.

Comment 1

[Gregory Szorc [:gps]]

Mercurial has facilities for opening an editor with content. We could rewrite the commit message and open the editor with that with some in-line comments telling them to fix any issues the auto rewriting may have incurred. This is how other Mercurial commands operate and should "just work."

Comment 2

[:glob ∗]

(In reply to Gregory Szorc [:gps] from comment #1)
> Mercurial has facilities for opening an editor with content. We could
> rewrite the commit message and open the editor with that with some in-line
> comments telling them to fix any issues the auto rewriting may have
> incurred. This is how other Mercurial commands operate and should "just
> work."

the problem with that approach is autoland is triggered from the mozreview ui, not from the command line, so the confirmation needs to live in review board.

Comment 3

[:glob ∗]

Attachment: MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor

mozautomation: add reviewer replacing (bug 1220232); r?dminor

Adds replace_reviewers, which will return the commit_description with all
existing r= and r? flags replaced with r=reviewers.

Comment 4

[:glob ∗]

Attachment: MozReview Request: mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps

mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps

Promote mozautomation from a raw library to an installable package.
Update mozreview's ansible and test scripts to deploy mozautomation.

Comment 5

[:glob ∗]

Attachment: MozReview Request: mozreview: move review helper methods out of hooks.py (bug 1220232); r?smacleod

mozreview: move review helper methods out of hooks.py (bug 1220232); r?smacleod

The review helper methods in hooks.py have utilitiy in other parts of review
board.  This patch creates a 'review_helper' package.

Comment 6

[:glob ∗]

Attachment: MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); r?dminor

mozreview: add commit_rewrite api resource (bug 1220232); r?dminor

Adds a webapi resource which returns the commit descriptions for the provided
parent review with r=reviewer indicators.  Existing r? and r= markers will be
rewritting, or an r= will be appended to the first line if none were provided.
This is used by autoland.

Comment 7

[:glob ∗]

Attachment: MozReview Request: mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?dminor

mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?dminor

Adds a confirmation dialog to the autoland menu item, which displays the
rewritten commit messages.  If all looks good the rewritten commit message is
passed to autoland.

Comment 8

[:glob ∗]

(In reply to Byron Jones ‹:glob› from comment #6)
> Created attachment 8688840 [details]
> MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232);
> r?dminor

this part is missing tests -- i'm still battling on that front.

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8688837 [details]
MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor

https://reviewboard.mozilla.org/r/25435/#review22955

::: pylib/mozautomation/mozautomation/commitparser.py:111
(Diff revision 1)
> +    reviewers = "r=" + ",".join(reviewers)

nit: we seem to be using ' rather than " in this file, please change this function to be consistent.

::: pylib/mozautomation/mozautomation/commitparser.py:125
(Diff revision 1)
> +                                                    commit_summary)

This leaves any whitespace after a reviewer's name, e.g.

replace_reviewers('bug 1 - blah r?dminor r?gps r?abc sr=abc', ['dminor', 'glob', 'gps', 'abc'])

gives:

'bug 1 - blah r=dminor,glob,gps,abc   sr=abc'

::: pylib/mozautomation/mozautomation/commitparser.py:129
(Diff revision 1)
> +        d = { 'first': True }

You don't seem to use d as dictionary anywhere else, so just:

first = True

should be fine here.

::: pylib/mozautomation/mozautomation/commitparser.py:139
(Diff revision 1)
> +        commit_summary = REVIEWERS_RE.sub(replace_first_reviewer, commit_summary)

Please trim any trailing whitespace here, just in case.

Comment 10

[Dan Minor [:dminor]]

Comment on attachment 8688840 [details]
MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); r?dminor

https://reviewboard.mozilla.org/r/25441/#review22965

::: pylib/mozreview/mozreview/extension.py:53
(Diff revision 1)
> +from mozreview.resources.commit_rewrite import (commit_rewrite_resource,)

nit: please make this alphabetical and remove the parentheses around commit_rewrite_resource.

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:35
(Diff revision 1)
> +        if not is_parent(parent_request):

This seems like a serious error to me. Please log something here.

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:38
(Diff revision 1)
> +            return DOES_NOT_EXIST

PERMISSION_DENIED seems more suitable here

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:39
(Diff revision 1)
> +        if COMMITS_KEY not in parent_request.extra_data:

This also seems like a serious problem, please log it as well.

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:52
(Diff revision 1)
> +            if child_request.approved:

I think this is an error condition - we should not be asking for a rewrite on an unapproved review request.

You can probably exit early with an error here.

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:63
(Diff revision 1)
> +                'summary': description,

This field should be called description rather than summary.

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8688841 [details]
MozReview Request: mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?dminor

https://reviewboard.mozilla.org/r/25443/#review22969

This lgtm, but you might want to flag mdoglio as well - I think he's more familiar with this code.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8688838 [details]
MozReview Request: mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps

https://reviewboard.mozilla.org/r/25437/#review23001

You shouldn't need to check in the egg-info files: Python packaging tools will generate these from setup.py at package build time. Follow https://packaging.python.org/en/latest/distributing/ for the packaging best practices.

::: ansible/roles/docker-rbweb/tasks/main.yml:26
(Diff revision 1)
> +- name: Install mozautomation library
> +  command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/mozautomation
> +
>  - name: Install mozreview extension
>    command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/mozreview
>  
>  - name: Install rbbz extension
>    command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/rbbz
>  
>  - name: Install rbmotd extension
>    command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/rbmotd

Should probably be using with_items: here to reduce boilerplate.

Comment 13

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/25439/#review23005

::: pylib/mozreview/mozreview/review_helpers.py:1
(Diff revision 1)
> +from __future__ import unicode_literals

Need license header.

Comment 14

[:glob ∗]

https://reviewboard.mozilla.org/r/25435/#review22955

> You don't seem to use d as dictionary anywhere else, so just:
> 
> first = True
> 
> should be fine here.

this doesn't work - you'll get "local variable 'first' referenced before assignment" because assignments to names always go to the innermost scope.

Comment 15

[Steven MacLeod [:smacleod]]

Comment on attachment 8688839 [details]
MozReview Request: mozreview: move review helper methods out of hooks.py (bug 1220232); r?smacleod

https://reviewboard.mozilla.org/r/25439/#review23083

Comment 16

[:glob ∗]

Comment on attachment 8688837 [details]
MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor

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

Comment 17

[:glob ∗]

unfortunately i managed to push a change that wiped out the issues from mozreview.

addressing issues here instead:

> ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:35
> (Diff revision 1)
> > +        if not is_parent(parent_request):
> 
> This seems like a serious error to me. Please log something here.

i don't agree that logging is required here.  the consumer of this API should be able to pass in whatever it wants, and we should return an appropriate error code when required.  logging of "hey, this isn't a parent request" belongs in the realm of the consumer, not the api.

> ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:63
> (Diff revision 1)
> > +                'summary': description,
> 
> This field should be called description rather than summary.

for consistency i followed our 'summary' api call from review_request_summary.py, which calls this field 'summary'.

Comment 18

[:glob ∗]

Attachment: MozReview Request: mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps

mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps

Promote mozautomation from a raw library to an installable package.
Update mozreview's ansible and test scripts to deploy mozautomation.

Comment 19

[:glob ∗]

Attachment: MozReview Request: mozreview: move review helper methods out of hooks.py (bug 1220232); r=smacleod

mozreview: move review helper methods out of hooks.py (bug 1220232); r=smacleod

The review helper methods in hooks.py have utilitiy in other parts of review
board.  This patch creates a 'review_helper' package.

Comment 20

[:glob ∗]

Attachment: MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); r?dminor

mozreview: add commit_rewrite api resource (bug 1220232); r?dminor

Adds a webapi resource which returns the commit descriptions for the provided
parent review with r=reviewer indicators.  Existing r? and r= markers will be
rewritting, or an r= will be appended to the first line if none were provided.
This is used by autoland.

Comment 21

[:glob ∗]

Attachment: MozReview Request: mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?mdoglio

mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?mdoglio

Adds a confirmation dialog to the autoland menu item, which displays the
rewritten commit messages.  If all looks good the rewritten commit message is
passed to autoland.

Comment 22

[Mauro Doglio [:mdoglio]]

Comment on attachment 8690682 [details]
MozReview Request: mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?mdoglio

https://reviewboard.mozilla.org/r/25911/#review23325

Comment 23

[Dan Minor [:dminor]]

(In reply to Byron Jones ‹:glob› from comment #17)
> unfortunately i managed to push a change that wiped out the issues from
> mozreview.
> 
> addressing issues here instead:
> 
> > ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:35
> > (Diff revision 1)
> > > +        if not is_parent(parent_request):
> > 
> > This seems like a serious error to me. Please log something here.
> 
> i don't agree that logging is required here.  the consumer of this API
> should be able to pass in whatever it wants, and we should return an
> appropriate error code when required.  logging of "hey, this isn't a parent
> request" belongs in the realm of the consumer, not the api.

Ah sorry, I thought you were looking up the parent of the review request passed to the API so if that didn't end up being a parent request it would indicate some kind of data integrity error.

Comment 24

[Dan Minor [:dminor]]

Comment on attachment 8690681 [details]
MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); r?dminor

https://reviewboard.mozilla.org/r/25909/#review23327

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:36
(Diff revision 1)
> +        if not is_parent(parent_request):

Since this API expects a parent review request, you could look up the parent review request rather than returning an error. You could also then only return the rewritten commit description for the child that is requested. I'm not sure if that is worth doing now, maybe add a TODO?

Comment 25

[Dan Minor [:dminor]]

Comment on attachment 8688837 [details]
MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor

https://reviewboard.mozilla.org/r/25435/#review23329

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8690679 [details]
MozReview Request: mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps

https://reviewboard.mozilla.org/r/25905/#review23437

Comment 27

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/25907/#review23439

Is this diff missing the changes to hooks.py?

::: pylib/mozreview/mozreview/review_helpers.py:5
(Diff revision 1)
> +from __future__ import unicode_literals

Please throw absolute_import in here as well.

Comment 28

[Steven MacLeod [:smacleod]]

Comment on attachment 8690680 [details]
MozReview Request: mozreview: move review helper methods out of hooks.py (bug 1220232); r=smacleod

https://reviewboard.mozilla.org/r/25907/#review23721

Comment 29

[:glob ∗]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/ff5286dd10b3
https://hg.mozilla.org/hgcustom/version-control-tools/rev/c94a76673d3c
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f61035162306
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5b0cc2971f25
https://hg.mozilla.org/hgcustom/version-control-tools/rev/c2620e01e208

Comment 30

[Mark Côté [:mcote]]

Was hgext/reviewboard/tests/test-review-commit-rewrite.t never reviewed?  I don't see it in the last revision of that commit in MozReview (https://reviewboard.mozilla.org/r/25909/).

I noticed because you were missing a few (glob)s. :)

Comment 31

[:glob ∗]

(In reply to Mark Côté [:mcote] from comment #30)
> Was hgext/reviewboard/tests/test-review-commit-rewrite.t never reviewed?  I
> don't see it in the last revision of that commit in MozReview
> (https://reviewboard.mozilla.org/r/25909/).

it wasn't.  it wasn't raised as an issue, and because i can't carry forward r+s i didn't want to waste anyone else's time on rubberstamping changes and further delay autoland.

> I noticed because you were missing a few (glob)s. :)

oops.  thanks for fixing that (http://hg.mozilla.org/hgcustom/version-control-tools/rev/14b4fd0e4dbc).