Enforce git branch policies

Question

I'm trying to enforce a company policy, taking these assumptions:

There are only 3 available upstream branches: master, version/* and hotfix/*.
Master branch accepts only non-forwarded merge commits.
Version and Hotfix branches accept only fast-forward/rebased commits.
Master branch must only be merged into from Version or Hotfix branches.
Version and Hotfix branches must diverge from Master branch directly.

So far this is what I come up with:

#!/usr/bin/env ruby
# Encoding: utf-8

$oldrev, $newrev, $refname = STDIN.read.split(" ")
$real_refname = `git rev-parse --abbrev-ref #{$refname} 2> /dev/null`.strip
$merge_commits = `git rev-list --merges #{$oldrev}..#{$newrev} 2> /dev/null`.strip
$parent_commit = `git rev-parse #{$newrev}\^1`
$ancestor_branch = `git show-branch | grep '*' | grep -v '#{$real_refname}' | head -n1 | sed 's/.*\[\(.*\)\].*/\1/' | sed 's/[\^~].*//'`

puts "Enforcing Policies... \n(#{$real_refname}) (#{$oldrev[0,6]}) (#{$newrev[0,6]})"

$errors = []
def check_branch_policy()
  $errors.push "Branch #{$real_refname}: Only Version, Hotfix and Master branches are allowed to be pushed upstream." if !$real_refname.match(/^(version\/[1-9.]+|hotfix\/[1-9.]+|master)/)
  $errors.push "Branch #{$real_refname}: Master branch accepts only non-forwarded merge commits." if $real_refname.match('master') && (!$merge_commits.match($newrev) || !$parent_commit.match($oldrev))
  $errors.push "Branch #{$real_refname}: Version and Hotfix branches accept only fast-forward/rebased commits." if !$real_refname.match('master') && !$merge_commits.empty?
  $errors.push "Branch #{$real_refname}: Version and Hotfix branches must diverge from Master branch directly." if !$real_refname.match('master') && !$ancestor_branch[4,6].match('master')
  false
end
check_branch_policy

unless $errors.empty?
  puts '[POLICY] Invalid git branch rules.'
  $errors.each { |error| puts "#    #{error}" }
  exit 1
end

A few issues though:

First, I'd be glad for a general code review. I'm not a rubyist whatsoever, and I just patched around things I found on the web. So the code is probably pretty bad.
Is there an easier way to enforce the "Master branch accepts only non-forwarded merge commits."?
sed and grep doesn't seem to play well with git hooks, so I basically need an alternative to the current $ancestor_branch command. Didn't come up with anything yet.
When first pushing a branch, $real_refname doesn't work - it can't seem to abbrev-ref properly.
I can't seem to find a way to enforce "Master branch must only be merged into from Version or Hotfix branches." yet. Any ideas?

EDIT #1 - 25.05.14

After tinkering around a little bit I got to this:

#!/usr/bin/env ruby
# Encoding: utf-8

oldrev, newrev, refname = STDIN.read.split(" ")
short_refname = refname[11..-1]
merge_commits = `git rev-list --merges #{oldrev}..#{newrev}`.strip
unique_revs = `git rev-list --all --not $(git rev-list --all ^#{newrev})`
missed_revs = `git rev-list #{oldrev}..#{newrev}`

puts "Enforcing Policies... \n(#{short_refname}) (#{oldrev[0,6]}) (#{newrev[0,6]})"

def check_branch_policy(oldrev,newrev,short_refname,merge_commits,unique_revs,missed_revs)
  errors = []
  errors << "Only Version, Hotfix and Master branches are allowed to be pushed upstream." if
    !short_refname[/^(version\/[1-9.]+|hotfix\/[1-9.]+|master)/]
  if short_refname['master']
    # Master should have only one unique commit each time - the merge commit (newrev).
    errors << "Master branch accepts only non-forwarded merge commits, one at a time." if
      !merge_commits[newrev] && missed_revs.count > 2
  else
    # If not empty, it means there's a merge commit - whereas there shouldn't be.
    errors << "Version and Hotfix branches accept only fast-forward/rebased commits." if
      !merge_commits.empty?
    # If not equal, it means at least one commit is reachable from another ref - meaning it was diverged.
    errors << "Version and Hotfix branches must diverge from Master branch directly." if
      !unique_revs[missed_revs]
  end
  errors
end
errors = check_branch_policy(oldrev,newrev,short_refname,unique_revs,missed_revs)

unless errors.empty?
  puts '[POLICY] Invalid git branch rules.'
  errors.each { |error| puts "#    Branch #{short_refname}: #{error}" }
  exit 1
end

More questions arose though:

Is there a way to serve the local variables without calling them in the method? Otherwise the script throws an error.
I managed to find a way to retrieve the short_refname, but it's not so elegant. I read somewhere I can use short_refname = refname.chomp("refs/heads/") but it doesn't seem to work. Help?
I found a way (clever? too complex? go figure) to find if a branch has diverged where it shouldn't have but this brings two issues - I can't get all the refs from the hook. --stdin flag doesn't seem to cut it. Further, the exclude flag (^some_ref) doesn't work inside the hook, whereas in the terminal it works fine. Ideas?
Assuming I move this script to update hook, how can I get the refnames? The web sources weren't so clear so far...

Sorry I forgot about the variable scope (probably made too much js recently...). In this case you should declare your variables outside the method as instance variables (`@` prefix). Not a big improvement over globals, but I consider it a better habit. The `#chomp` method cuts the given parameter at the *end* of the string off. I still consider my regex given below better (`[/[^\/]*$/]`). If you use the `update` hook the refnames are given as commandline parameters (`$1`, `$2`, `$3`). — David Ongaro, May 25 '14 at 05:46
Using `[/[^\/]*$/]` actually didn't seem to work...Any idea why? — Devon Ville, May 26 '14 at 07:52
On second thought you're better of with `refname[11..-1]` (or `refname[/^refs\/heads\/(.*)/, 1]` to make sure you're chopping off the right prefix) because a branchname might include slashes. — David Ongaro, May 29 '14 at 06:20
Uh, that might explain it. Our branches do include slashes. Thanks! — Devon Ville, May 29 '14 at 14:57

David Ongaro · Accepted Answer · 2014-05-24T16:34:39.803

Lets first focus on the ruby part:

There is hardly ever a reason to use global variables in ruby. And in a script they are in a "global" scope anyway => get rid of the preceding $ in variable names

In this code:

$errors = []
def check_branch_policy()
  $errors.push "Branch #{$real_refname}: Only Version, Hotfix and Master branches are allowed to be pushed upstream." if !$real_refname.match(/^(version\/[1-9.]+|hotfix\/[1-9.]+|master)/)
  $errors.push "Branch #{$real_refname}: Master branch accepts only non-forwarded merge commits." if $real_refname.match('master') && (!$merge_commits.match($newrev) || !$parent_commit.match($oldrev))
  $errors.push "Branch #{$real_refname}: Version and Hotfix branches accept only fast-forward/rebased commits." if !$real_refname.match('master') && !$merge_commits.empty?
  $errors.push "Branch #{$real_refname}: Version and Hotfix branches must diverge from Master branch directly." if !$real_refname.match('master') && !$ancestor_branch[4,6].match('master')
  false
end
check_branch_policy

It's bad style to write a method (or a function) which just works on a global object created only for this purpose. You might as well just remove the method definition, because it does nothing here. This is not particular "ruby style" thing but applies to programming in general. The better solution is to just create the object inside the method and return it. I also don't like these long unreadable lines. So in total would probably structure it more like this:

def check_branch_policy
  errors = []
  errors << "Only Version, Hotfix and Master branches are allowed to be pushed upstream." if 
    !real_refname[/^(version\/[1-9.]+|hotfix\/[1-9.]+|master)/]
  if real_refname['master']
    errors << "Master branch accepts only non-forwarded merge commits." if
      !merge_commits[newrev] || !parent_commit[oldrev]
  else
    errors << "Version and Hotfix branches accept only fast-forward/rebased commits." if
      merge_commits.empty?
    errors << "Version and Hotfix branches must diverge from Master branch directly." if
      !ancestor_branch[4, 6]['master']
  end
  errors
end

Even though the messages may be less neatly aligned here, I think it's an improvement that one can better see the conditions which should hold in each case. Note that I used the ruby idoms << instead of .push and [] instead of .match. I also left the Branch #{real_refname}: prefix out, it can be just as well in your error output loop if its always the same.

Also there is hardly a reason to rely on grep and sed when you have the power of ruby at hand.

As for the git part:

What you're trying to do is certainly possible, but I guess some try and error is needed. So I can't give you a working solution out of the hand. Some remarks though:

I think a better way to get a short symbolic ref in ruby is
```
`git symbolic-ref #{refname}\`[/[^\/]*$/].chomp
```
or even
```
`git symbolic-ref --short #{refname}`
```
you can try if that works more reliable than git rev-parse --abbrev-ref. Furthermore your variable real_refname is badly named. The 'real' ref name sounds like it would actually be the SHA1 hash. Probably short_refname would be better.
Since you're reading the refs from stdin I guess that you use a pre-receive git hook? But in this case you've clearly a bug, because there might be several branches updated in one push. You should either iterate over stdin or use the update hook
git show-branch is a porcelain command, i.e. it shouldn't be used for scripting because the output is meant for users. I think Junio did some pretty neat stuff in his pre-rebase.sample. Maybe you can get some ideas from there how to do it with plumbing commands.
I used to write even simple hooks in ruby, but I learned over the years that bash is also quite capable. So unless your hook gets really complex you might just start with bash.

Thanks for your comment. I updated my question based on your remarks. — Devon Ville, May 24 '14 at 23:36

Enforce git branch policies

EDIT #1 - 25.05.14

1 Answers1

Linked