UI to confirm rewritten commit message

RESOLVED FIXED

Status

MozReview
General
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcote, Assigned: glob)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 4 obsolete attachments)

40 bytes, text/x-review-board-request
dminor
: review+
Details | Review
40 bytes, text/x-review-board-request
gps
: review+
Details | Review
40 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
40 bytes, text/x-review-board-request
dminor
: review+
Details | Review
40 bytes, text/x-review-board-request
mdoglio
: review+
Details | Review
(Reporter)

Description

3 years ago
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

3 years ago
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."
(Reporter)

Updated

3 years ago
Blocks: 1220214
(Assignee)

Comment 2

3 years ago
(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.
(Assignee)

Comment 3

3 years ago
Created attachment 8688837 [details]
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.
Attachment #8688837 - Flags: review?(dminor)
(Assignee)

Comment 4

3 years ago
Created attachment 8688838 [details]
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.
Attachment #8688838 - Flags: review?(gps)
(Assignee)

Comment 5

3 years ago
Created attachment 8688839 [details]
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.
Attachment #8688839 - Flags: review?(smacleod)
(Assignee)

Comment 6

3 years ago
Created attachment 8688840 [details]
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.
Attachment #8688840 - Flags: review?(dminor)
(Assignee)

Comment 7

3 years ago
Created attachment 8688841 [details]
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.
Attachment #8688841 - Flags: review?(dminor)
(Assignee)

Comment 8

3 years ago
(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

3 years ago
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.
Attachment #8688837 - Flags: review?(dminor)

Comment 10

3 years ago
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.
Attachment #8688840 - Flags: review?(dminor)

Comment 11

3 years ago
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.
Attachment #8688841 - Flags: review?(dminor)
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.
Attachment #8688838 - Flags: review?(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.
(Assignee)

Comment 14

3 years ago
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.
Attachment #8688839 - Flags: review?(smacleod) → review+
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
(Assignee)

Comment 16

3 years ago
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/
Attachment #8688837 - Flags: review?(dminor)
(Assignee)

Updated

3 years ago
Attachment #8688838 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8688839 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8688840 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8688841 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
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'.
(Assignee)

Comment 18

3 years ago
Created attachment 8690679 [details]
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.
Attachment #8690679 - Flags: review?(gps)
(Assignee)

Comment 19

3 years ago
Created attachment 8690680 [details]
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.
Attachment #8690680 - Flags: review?(smacleod)
(Assignee)

Comment 20

3 years ago
Created attachment 8690681 [details]
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.
Attachment #8690681 - Flags: review?(dminor)
(Assignee)

Comment 21

3 years ago
Created attachment 8690682 [details]
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.
Attachment #8690682 - Flags: review?(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
Attachment #8690682 - Flags: review?(mdoglio) → review+

Comment 23

3 years ago
(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

3 years ago
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?
Attachment #8690681 - Flags: review?(dminor) → review+

Updated

3 years ago
Attachment #8688837 - Flags: review?(dminor) → review+

Comment 25

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

https://reviewboard.mozilla.org/r/25435/#review23329
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
Attachment #8690679 - Flags: review?(gps) → review+
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.
Attachment #8690680 - Flags: review?(smacleod) → review+
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
(Reporter)

Comment 30

3 years ago
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. :)
(Assignee)

Comment 31

3 years ago
(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).
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.