Summary

Add mercurial hook for posting review requests automatically

Review Request #7824 — Created Dec. 23, 2015 and discarded Nov. 26, 2016, 6:19 p.m.

Information

Owner

misery

Repository

RBTools

Branch

master

Bugs

Depends On

~~7823~~

Reviewers

Groups

rbtools

People

halvorlu

Description

A Mercurial hook to post to Review Board on push to a central server.

The hook only allows the push if all commits have been approved in a
review requests, otherwise it rejects it and creates a review request
for the (new) commits. This is similar to "rbt post", but

1. does not require the user to install RBTools locally
2. makes sure any changes pushed to the central server have been
   reviewed and approved
3. makes links of all references to tickets/bugs/issues, which
   rbt post doesn't do.
4. automatically finds the right review request to update if there
   are any new commits, based the commit ID and a date/author hash.
   This does not require the user to confirm anything, which rbt post
   (often) requires. This also allows the hook to recognize
   rebased/amended changesets, because the date/author hash
   is unchanged.

Patch is based on work of Halvor Lund!

Testing Done

Replacing this: https://reviews.reviewboard.org/r/7100/

We use this commit hook since february 2015 as a gatekeeper like gerrit codereview.

Issues

Description	From	Last Updated
We've been slowly transitioning to Google Python-style docstrings. Can you write this as: Run the hook. Args: node (unicode): The …	david	Dec. 28, 2015, 4:48 p.m.
Can you use the Args: format as described above?	david	Dec. 28, 2015, 4:48 p.m.
Can we wrap this so each condition is on its own line? You can also combine the second two: if …	david	Dec. 28, 2015, 5:04 p.m.
Args:	david	Dec. 28, 2015, 4:49 p.m.
r before s.	david	Dec. 28, 2015, 5:05 p.m.
ids is already a list here.	david	Dec. 28, 2015, 5:05 p.m.
We put full-package imports before from ... imports.	david	Dec. 28, 2015, 5:07 p.m.
Process return codes should be int, not bool.	david	Dec. 28, 2015, 5:08 p.m.
Please reformat as: revisions = { 'base': rev1, 'tip': rev2, 'parent_base': parent, }	david	Dec. 28, 2015, 5:09 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 23 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 25 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 5 E128 continuation line under-indented for visual indent	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 5 E122 continuation line missing indentation or outdented	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 22 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 24 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 5 E122 continuation line missing indentation or outdented	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 1 E122 continuation line missing indentation or outdented	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 15 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 17 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 20 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 22 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 15 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 17 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 11 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 13 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 21 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 23 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 15 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 17 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 26 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 28 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 18 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 20 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 19 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 21 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 20, 2016, 1:39 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Jan. 28, 2016, 2 a.m.
It's a little weird to call this LOGGER, since it's not a constant. How about logger?	david	Feb. 3, 2016, 1:31 a.m.
Can you surround the whole conditional in parentheses instead of using the continuation character?	david	Feb. 3, 2016, 1:38 a.m.
These should be at the top of the file.	david	Feb. 3, 2016, 1:37 a.m.
Can we swap the order of these?	david	Feb. 3, 2016, 1:42 a.m.
This should be grouped with the other stdlib imports.	david	March 31, 2016, 3:20 p.m.
Call this "logger" (since it's not really a constant)	david	March 31, 2016, 3:20 p.m.
Put this import at the top of the file.	david	March 31, 2016, 3:20 p.m.
Rather than implementing a new execute method, we should use rbtools.utils.process.execute	david	March 31, 2016, 3:20 p.m.
This should use the approval API. See https://www.reviewboard.org/docs/manual/2.5/webapi/2.0/resources/review-request/#fields	david	March 31, 2016, 3:20 p.m.
Can you wrap the whole conditional in parens instead of using the continuation character? Please also put the "and" on …	david	March 31, 2016, 3:20 p.m.
Why doesn't this use rbtools.utils.users.get_authenticated_session ?	david	March 31, 2016, 3:20 p.m.
Put this import at the top of the file.	david	March 31, 2016, 3:20 p.m.

contrib/tools/mercurial_push.py (Diff revision 1)

The issue has been resolved. Show all issues

We've been slowly transitioning to Google Python-style docstrings. Can you write this as:

Run the hook.

Args:
    node (unicode): The hex node ID of the first changeset.

contrib/tools/mercurial_push.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you use the Args: format as described above?
```

contrib/tools/mercurial_push.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we wrap this so each condition is on its own line? You can also combine the second two:

if (revreq.branch == branch and
    'diff_hash' in revreq.extra_data.get('diff_hash', '')):

misery Dec. 28, 2015, 5:12 p.m.

Are you sure? I changed it to this:

if (revreq.branch == branch and
    diff_hash == revreq.extra_data.get('diff_hash'):

david Dec. 28, 2015, 5:18 p.m.

You're right. I was confusing this with other code I was reading in another tab (that used in).

contrib/tools/mercurial_push.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Args:
```
rbtools/hooks/common.py (Diff revision 1)
The issue has been resolved. Show all issues
```
r before s.
```
rbtools/hooks/common.py (Diff revision 1)
The issue has been resolved. Show all issues
```
ids is already a list here.
```
rbtools/hooks/mercurial.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We put full-package imports before from ... imports.
```
rbtools/hooks/mercurial.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Process return codes should be int, not bool.
```

rbtools/hooks/mercurial.py (Diff revision 1)

The issue has been resolved. Show all issues

Please reformat as:

revisions = {
    'base': rev1,
    'tip': rev2,
    'parent_base': parent,
}

Diff:

Revision 2 (+924 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/common.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py


WARNING: Number of comments exceeded maximum, showing 30 of 69.

Tool: Pyflakes
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 9
 E128 continuation line under-indented for visual indent
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 23
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 25
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 5
 E128 continuation line under-indented for visual indent
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 5
 E122 continuation line missing indentation or outdented
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 22
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 24
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 5
 E122 continuation line missing indentation or outdented
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E122 continuation line missing indentation or outdented
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 15
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 17
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 20
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 22
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 15
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 17
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 11
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 13
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 21
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 23
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 15
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 17
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 26
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 28
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 18
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 20
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 19
 E251 unexpected spaces around keyword / parameter equals
```
contrib/tools/mercurial_push.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 21
 E251 unexpected spaces around keyword / parameter equals
```

Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

Thanks for taking the time to submit this again. What are the main differences from my original submission?

misery Jan. 4, 2016, 3:21 a.m.

I just fixed the issues in your review request. :-)
I added a new request because I cannot update your request. I will fix these issues here, too. Hopefully we get this merged!

Diff:

Revision 3 (+924 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/common.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: Pyflakes
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

Change Summary:

Compatible with 2.5.2 now and fix a "unchanged changeset" mistake

Diff:

Revision 4 (+924 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/common.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

rbtools/hooks/mercurial.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Diff:

Revision 5 (+925 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/common.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

Change Summary:

Save final changeset id in close description like "rbt land".

Could someone review this to get it finally merged? :-)

Diff:

Revision 6 (+929 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/common.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

Ship it!

```
Ship It!
```

contrib/tools/mercurial_push.py (Diff revision 6)

The issue has been resolved. Show all issues

It's a little weird to call this LOGGER, since it's not a constant. How about logger?

contrib/tools/mercurial_push.py (Diff revision 6)

The issue has been resolved. Show all issues

Can you surround the whole conditional in parentheses instead of using the continuation character?

contrib/tools/mercurial_push.py (Diff revision 6)
The issue has been resolved. Show all issues
```
These should be at the top of the file.
```
rbtools/hooks/mercurial.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can we swap the order of these?
```

Change Summary:

Fix issues

Diff:

Revision 7 (+928 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/common.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/common.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

Fix it!

```
Sorry for the long delay on this.
```
rbtools/hooks/mercurial.py (Diff revision 7)
An issue was opened. Show all issues
```
This should be grouped with the other stdlib imports.
```
rbtools/hooks/mercurial.py (Diff revision 7)
An issue was opened. Show all issues
```
Call this "logger" (since it's not really a constant)
```
rbtools/hooks/mercurial.py (Diff revision 7)
An issue was opened. Show all issues
```
Put this import at the top of the file.
```

rbtools/hooks/mercurial.py (Diff revision 7)

An issue was opened. Show all issues

Rather than implementing a new execute method, we should use rbtools.utils.process.execute

rbtools/hooks/mercurial.py (Diff revision 7)

An issue was opened. Show all issues

This should use the approval API. See https://www.reviewboard.org/docs/manual/2.5/webapi/2.0/resources/review-request/#fields

rbtools/hooks/mercurial.py (Diff revision 7)

An issue was opened. Show all issues

Can you wrap the whole conditional in parens instead of using the continuation character? Please also put the "and" on the previous line.

rbtools/hooks/mercurial.py (Diff revision 7)

An issue was opened. Show all issues

Why doesn't this use rbtools.utils.users.get_authenticated_session ?

rbtools/hooks/mercurial.py (Diff revision 7)
An issue was opened. Show all issues
```
Put this import at the top of the file.
```

Status:: Discarded
Change Summary:: This is superseded by a complete refactoring: https://reviews.reviewboard.org/r/8554/