Summary

Add hooks module, containing utility functions common to hook scripts.

Review Request #5543 — Created Feb. 24, 2014 and submitted March 27, 2014, 2:01 p.m.

Information

Owner

anselina

Repository

RBTools

Branch

master

Bugs

Depends On

Blocks

5403

Commit

0c6bd5c...

Reviewers

Groups

rbtools, students

People

Description

This is a new 'hooks' module in RBTools. rbtools/hooks/common.py contains functions common to all hook scripts, and rbtools/hooks/git.py contains functions for Git hook scripts.

Testing Done


Tested the functions individually, and got the expected results.
More testing was done in https://reviews.reviewboard.org/r/5403/ for the Git post-receive and pre-receive hook scripts that use this new module.

Issues

Description	From	Last Updated
There should be two blank lines between items at the lowest indentation level. Here and below.	david	Feb. 28, 2014, 12:14 p.m.
This should probably propagate up the message in the exception.	david	Feb. 28, 2014, 12:14 p.m.
This should probably check that the command was sucessful.	david	Feb. 28, 2014, 1:49 p.m.
Should it be up to the caller to pass in a compiled regex with the options that they care to …	david	Feb. 28, 2014, 2:23 p.m.
This can just return directly instead of assigning to a variable and returning that.	david	Feb. 28, 2014, 12:15 p.m.
Two blank lines between top-level blocks. Here and below.	david	Feb. 28, 2014, 12:15 p.m.
It might be nice to pass in arguments as a list instead of relying on shell=True to split it.	david	Feb. 28, 2014, 7:39 p.m.
Relying on the pipe here makes it so this only works on unix-type systems (and even then relies a lot …	david	Feb. 28, 2014, 7:39 p.m.
You could use '0' * 40 here.	david	Feb. 28, 2014, 12:15 p.m.
No need for this line.	david	Feb. 28, 2014, 12:15 p.m.
No need for this line.	david	Feb. 28, 2014, 12:15 p.m.
"URL"	chipx86	March 3, 2014, 7:37 p.m.
So I believe that Christian's comment about _make_api_client was that it's only used in this one place, so instead of …	david	March 25, 2014, 10:30 p.m.

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

The issue has been resolved. Show all issues

There should be two blank lines between items at the lowest indentation level. Here and below.

rbtools/hooks/common.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably propagate up the message in the exception.
```
rbtools/hooks/common.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably check that the command was sucessful.
```

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

The issue has been resolved. Show all issues

Should it be up to the caller to pass in a compiled regex with the options that they care to use?

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

The issue has been resolved. Show all issues

This can just return directly instead of assigning to a variable and returning that.

rbtools/hooks/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Two blank lines between top-level blocks. Here and below.
```

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

The issue has been resolved. Show all issues

It might be nice to pass in arguments as a list instead of relying on shell=True to split it.

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

The issue has been resolved. Show all issues

Relying on the pipe here makes it so this only works on unix-type systems (and even then relies a lot on shell=True parsing this). You could do this filtering pretty easily in python instead.

rbtools/hooks/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You could use '0' * 40 here.
```
rbtools/hooks/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for this line.
```
rbtools/hooks/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for this line.
```

Change Summary:

Fixed some style issues, and modified execute statements so we no longer rely on shell=True.

Commit:

8ba9719ac1ea10d602df56eac278c29c19a52e48

84097a2a25dadee9bd8542ba0b95c7feeddb3d95

Diff:

Revision 2 (+214)

Show changes

	rbtools/hooks/common.py
	rbtools/hooks/git.py

Guess I never published this. Meant to :/

rbtools/hooks/common.py (Diff revision 2)

Just curious. Why add this function instead of just instantiating RBClient where needed?

anselina March 3, 2014, 7:38 p.m.

Hm, I actually copied this function and get_api() from rbtools/commands/init.py. Can this be improved?

rbtools/hooks/common.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"URL"
```

Change Summary:

Fixed capitalization of 'URL'.

Commit:

84097a2a25dadee9bd8542ba0b95c7feeddb3d95

642042a16f918fe310c6710be8266a745b4c2a41

Diff:

Revision 3 (+214)

Show changes

	rbtools/hooks/common.py
	rbtools/hooks/git.py

Change Summary:

Fixed a docstring.

Commit:

642042a16f918fe310c6710be8266a745b4c2a41

39f6b85e99c0a0a02a00e566b0209219a5f4a3aa

Diff:

Revision 4 (+214)

Show changes

	rbtools/hooks/common.py
	rbtools/hooks/git.py

Commit:

39f6b85e99c0a0a02a00e566b0209219a5f4a3aa

aa00a988cde63eda40a6ae7e810bbc82dc078f3b

Diff:

Revision 5 (+216)

Show changes

	rbtools/hooks/common.py
	rbtools/hooks/git.py

rbtools/hooks/common.py (Diff revision 5)

The issue has been resolved. Show all issues

So I believe that Christian's comment about _make_api_client was that it's only used in this one place, so instead of wrapping it in a function you can just replace this line with:

api_client = RBClient(server_url, username=username,
                      password=password)

Change Summary:

Changed all review request ID variables to ints, and added a log formatter.

Commit:

aa00a988cde63eda40a6ae7e810bbc82dc078f3b

55d245cb8bdc52272b26a2dfeda1d1d9650c4d91

Diff:

Revision 6 (+216)

Show changes

	rbtools/hooks/common.py
	rbtools/hooks/git.py

Ship it!

This looks good to me. Are there any other changes you want to make before I push it?

anselina March 27, 2014, 1:12 a.m.

I gave this and /r/5403 a final round of testing, and found one last bug I must've missed while adapting the code for the pre-receive approval hook. In any case, the latest revision fixes it. I won't be making any further changes, so this is ready to be pushed. :)

Change Summary:

Fixed a bug that caused the pre-receive hook to crash for a push with a newly created branch. (We need to run git rev-list using new_rev, not ref_name. The branch ref_name will not yet exist before the push, which is when the pre-receive hook is executed.)

Commit:

55d245cb8bdc52272b26a2dfeda1d1d9650c4d91

0c6bd5cc8c66b1fde9861a457968ce9167039f5a

Diff:

Revision 7 (+216)

Show changes

	rbtools/hooks/common.py
	rbtools/hooks/git.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (468379e). Thanks!