Add Git hooks to close review requests automatically and to check if each review request is approved.

Review Request #5403 — Created Feb. 5, 2014 and submitted

anselina
Review Board
master
5543
6d9cdcf...
reviewboard, students

There are two Git hook scripts in this review request.

The first is a Git post-receive hook script (git-hook-set-submitted) that will automatically close review requests as "submitted" after a push.

The second is a Git pre-receive hook script (git-hook-check-approval) that checks if each review request corresponding to a commit has been approved. A commit that has no corresponding review request is marked as being unapproved (optional). If any of the checked commits is unapproved, it declines the push (optional).

In both, to determine which review requests should be processed, it scans through each commit's commit message for the following strings (case-insensitive): "Reviewed at <reviewboard_url>/r/<id>" or "Review request #<id>".

Created a local Git repository, added the hook, and tested different pushes from a cloned repository:

  • Commits with and without a review request ID in the commit message
  • Merge commits (3-way merge, fast-forward merge)
  • Branch deletion (no "fatal" error shows up)
  • Branch creation (1 branch with 2 initial commits -- all review requests are closed properly)
  • Commits referencing the same review request ID (close_review_request is executed only once, and the change description displays all commits that referenced that particular review request ID)
  • Commit pushed to multiple branches (e.g., someone commits on branch1, merges branch1 into master, pushes both branch1 and master -- see attached screenshot. close_review_request is executed once.)
  • Branch creation and commit pushed to multiple branches (e.g., someone creates branch1, commits on branch1, merges branch1 into master, pushes both branch1 and master)

All test cases above showed the expected behaviour.

Additionally, for the pre-receive hook:
- Commit with review request ID that is approved (push is accepted)
- Commit with review request ID that is not approved, due to open issues or a lack of "Ship It!"s (push is declined)
- Commit without a review request ID in the commit message (push is declined)
- Multiple commits, where one of the review requests is not approved (push is declined)
- Multiple commits, where all of the review requests are approved (push is accepted)
- Set REQUIRE_REVIEW_REQUESTS to False, and pushed a commit without a review request ID in the commit message (push is accepted)
- Set DECLINE_UNAPPROVED_PUSH to False, and pushed commits that are not approved (push is accepted)


Description From Last Updated

We should expand this top comment to describe the format used to reference the review requdest ids in the commit ...

SM smacleod

Take a look into rbtools.utils.process, we include an execute command which has some extra helpers, including allowing you to specify ...

SM smacleod

You should try to cut the summary down to one line. You could probably add the information about revserse order ...

SM smacleod

Style: These doc strings should generally be a single line summary, with a blank line, and then additional information. So, ...

SM smacleod

I'd prefer if we explicitly return None rather than having it happen due to no call to return. Maybe go ...

SM smacleod

This would probably look cleaner formatted as a list on multiple lines. See my previous comment about using rbtools' execute ...

SM smacleod

Nit: "each commit's commit message" is duped here.

mike_conleymike_conley

I think we generally put the imports before any const declarations.

mike_conleymike_conley

The "Reviewed at /r/" is really "Reviewed at <URL>/r/", right?

chipx86chipx86

Maybe mention that this can be customized if a different format is required. Given that, the regex should used named ...

chipx86chipx86

Should have a 'r' prefix, like: REGEX = r'(....)'

chipx86chipx86

No blanke line here.

chipx86chipx86

No need for parens on the join string.

chipx86chipx86

The parameters should align. For logging calls, you don't need to use %. You can just pass the parameters as ...

chipx86chipx86

No blank line here.

chipx86chipx86

No blank line here either. You can just collapse these into one conditional.

chipx86chipx86

Same comment about parens.

chipx86chipx86

Same comment about arguments.

chipx86chipx86

Two blank lines between top-level functions/classes/etc.

chipx86chipx86

This is really "Reviewed at <URL>/r/"

chipx86chipx86

Same comments about the regex.

chipx86chipx86

No blank line.

chipx86chipx86

Does this show the branch names, or just the SHAs?

chipx86chipx86

No need for parens.

chipx86chipx86

There's an extra blank line here. I think maybe we miscommunicated when you asked if files should end in a ...

daviddavid

Extra blank line.

daviddavid
anselina
SM
  1. This is looking great so far!

  2. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     

    We should expand this top comment to describe the format used to reference the review requdest ids in the commit message.

    Speaking of which, maybe we should have the regex as a global up here so it can be customized easily.

  3. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     

    I wonder if we could pull this information from the repositories ".reviewboardrc" file?

    I don't think you should really spend a lot of time investigating this, or if it's even useful, just a thought.

  4. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     
     
     
     

    Take a look into rbtools.utils.process, we include an execute command which has some extra helpers, including allowing you to specify the command as a list.

    1. As discussed earlier, the execute command in rbtools.utils.process' cannot handle the command in get_excluded_branches, so we decided to keep this execute function.

  5. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     
     
     

    You should try to cut the summary down to one line. You could probably add the information about revserse order after a blank line.

  6. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     
     

    Style: These doc strings should generally be a single line summary, with a blank line, and then additional information.

    So, in this case:

    """Returns the review request ID referenced in the commit message.

    We assume there is only one review request to be closed per commit. If
    an ID cannot be found we return None.
    """

  7. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     
     
     

    I'd prefer if we explicitly return None rather than having it happen due to no call to return.

    Maybe go with something like this?

    review_request_id = (match and match.group(1)) or None
    return review_request_id
    
  8. contrib/tools/git-hook-set-submitted (Diff revision 1)
     
     
     
     
     

    This would probably look cleaner formatted as a list on multiple lines. See my previous comment about using rbtools' execute function.

    1. (See my above comment in the third issue)

  9. 
      
anselina
mike_conley
  1. This looks really good! Just some small nits here on a quick pass.

  2. contrib/tools/git-hook-set-submitted (Diff revision 2)
     
     
     

    Nit: "each commit's commit message" is duped here.

  3. contrib/tools/git-hook-set-submitted (Diff revision 2)
     
     
     
     
     
     

    I think we generally put the imports before any const declarations.

  4. This looks

anselina
chipx86
  1. 
      
  2. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     

    The "Reviewed at /r/" is really "Reviewed at <URL>/r/", right?

    1. Yup, I must have forgotten to update the blurb at the top when we decided we should include the URL in the regex!

  3. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     

    Maybe mention that this can be customized if a different format is required.

    Given that, the regex should used named groups for the results, instead of positional groups. You can do that with:

    r'.... (?<id>\d+)'
    

    And then later:

    m.group('id')
    
  4. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     

    Should have a 'r' prefix, like:

    REGEX = r'(....)'
    
  5. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     
     
     

    No blanke line here.

  6. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     

    No need for parens on the join string.

  7. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     
     

    The parameters should align.

    For logging calls, you don't need to use %. You can just pass the parameters as parameters to the function.

  8. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     
     
     

    No blank line here.

  9. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     
     
     

    No blank line here either.

    You can just collapse these into one conditional.

  10. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     

    Same comment about parens.

  11. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     
     

    Same comment about arguments.

  12. contrib/tools/git-hook-check-approval (Diff revision 3)
     
     
     
     

    Two blank lines between top-level functions/classes/etc.

  13. contrib/tools/git-hook-set-submitted (Diff revision 3)
     
     

    This is really "Reviewed at <URL>/r/"

  14. contrib/tools/git-hook-set-submitted (Diff revision 3)
     
     
     

    Same comments about the regex.

  15. contrib/tools/git-hook-set-submitted (Diff revision 3)
     
     
     
     

    No blank line.

  16. contrib/tools/git-hook-set-submitted (Diff revision 3)
     
     
     

    Does this show the branch names, or just the SHAs?

    1. This shows both branch names and SHAs, and will be something like "Pushed to master (6752b46), release-1.3 (6752b46)". Do we want to do something different?

  17. contrib/tools/git-hook-set-submitted (Diff revision 3)
     
     

    No need for parens.

  18. 
      
chipx86
  1. Oh, one more thing. I think this code would be better living in the RBTools tree. More accessible than pulling down the RB tree. It'll also help tie the hooks to RBTools versions more.

    I didn't see any change yet for the RBTools modifications for the new the modules you made.

    1. Thanks for the review! I actually uploaded two files (common.py, git.py) separately for the RBTools modifications because this review request is stuck under the Review Board repo. Should I just start another WIP review request for that code, and add a "Depends On" for this review request?

  2. 
      
anselina
anselina
anselina
anselina
anselina
anselina
anselina
david
  1. What do users see if they try to push but their push is denied because the review request isn't approved?

    1. Here's what the output looks like right now for a push with 3 commits (the 1st does not have any matching review request ID in the commit message, the 2nd has a review request that is approved, and the 3rd has a review request with open issues) with REQUIRE_REVIEW_REQUEST and DECLINE_UNAPPROVED_PUSH both set to True:

      (reviewboard)anselina@ubuntu:~/dev/git1$ git push origin test8
      Counting objects: 11, done.
      Compressing objects: 100% (9/9), done.
      Writing objects: 100% (9/9), 881 bytes, done.
      Total 9 (delta 6), reused 0 (delta 0)
      Unpacking objects: 100% (9/9), done.
      remote: WARNING:root:A review request has not been created for test8 (41201ce)
      remote: WARNING:root:Review request #34 for test8 (9329c83) is not approved: The review request has open issues.
      To file:///home/anselina/dev/gitrepos/git1/
       ! [remote rejected] test8 -> test8 (pre-receive hook declined)
      error: failed to push some refs to 'file:///home/anselina/dev/gitrepos/git1/'
      

      Maybe I should throw in a final warning before declining the push (i.e. "Declining the push - there are 1 or more unapproved commits.")?

    2. I agree that a final warning would be nice. How about also adding a log formatter to do better than "WARNING:root:<message>" ?

    3. It looks like this now (let me know if you have any thoughts on making it better?):

      (reviewboard)anselina@ubuntu:~/dev/git1$ git push origin test5
      Counting objects: 11, done.
      Compressing objects: 100% (9/9), done.
      Writing objects: 100% (9/9), 867 bytes, done.
      Total 9 (delta 6), reused 0 (delta 0)
      Unpacking objects: 100% (9/9), done.
      remote: WARNING: A review request has not been created for test5 (a826fe8)
      remote: WARNING: Review request #30 for test5 (ccd2c3e) is not approved: The review request has not been marked "Ship It!"
      remote: WARNING: Declining the push - there are 1 or more unapproved commits.
      To file:///home/anselina/dev/gitrepos/git1/
       ! [remote rejected] test5 -> test5 (pre-receive hook declined)
      error: failed to push some refs to 'file:///home/anselina/dev/gitrepos/git1/'
      
    4. Looks great!

  2. contrib/tools/git-hook-check-approval (Diff revision 7)
     
     

    There's an extra blank line here. I think maybe we miscommunicated when you asked if files should end in a newline. Some editors don't add a \n at the very end of the file, but most do. It looks here like your files end in \n\n instead of just \n.

  3. contrib/tools/git-hook-set-submitted (Diff revision 7)
     
     

    Extra blank line.

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

    1. Won't be making any changes - this is good to go!

  2. 
      
anselina
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (b48028e). Thanks!
Loading...