CODEWITHSUNDEEP
rubygitgithooks
Devon Ville
Devon Ville

Reputation: 2091

Enforce git branch policies

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

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:

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:

Upvotes: 2

Views: 593

Answers (1)

David Ongaro
David Ongaro

Reputation: 3946

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.

Upvotes: 1

Related Questions