Git pre-commit hook to check that changes to certain files only happen on the master branch

Question

As a beginner in Ruby, I am trying to optimize this script that is a client side pre-commit hook for git.

Was hoping I could get a review to ensure I am following all ruby idioms, and to ensure I have the most elegant solution.

#!/usr/bin/env ruby
#
# Pre-commit hook created to help ensure dns changes are being 
# committed to the correct branch.
#

# Hash of zone files to look for when a commit is attempted.
zone_files = { 
  development: ‘some_zone_file1’,
  prod: ‘some_zone_file2’
}

# Execute git diff to determine files includes as part of commit.
out_data = %x{git diff --cached --name-only}
pri_branch = "master"

# Iterate through the zone files, and check if any are found in the output. 
# Only one needs to be found. No need to find all.
zone_files.keys.each do |key| 
  if out_data.include?zone_files[key]
    current_branch = %x{git branch}.match(/\* (.+?)\n/m)
    if current_branch[1].include?pri_branch
      break
    else
      puts "Warning: You have triggered a git pre-commit hook."
      puts "Warning: Your current branch is: #{current_branch[1]}."
      puts "Warning: For dns, ensure you commit on branch: #{pri_branch}"
      puts "Warning: Include '-n' to bypass this check."
      puts "Exiting..."
      exit 1
    end
  end
end

Answer 1

1. Use fewer abbreviations. It's certainly not bad in this case, but small stuff like production instead of prod (unless perhaps it's a naming convention for your production environment/server), and primary_branch (I guess?) instead of pri_branch. Or, at any rate, be consistent: You have development spelled out, but you shorten production?

2. Use parenthenses or at the very least spaces in method calls - and include? is just a method call. So this expression out_data.include?zone_files[key] looks weird right now.

3. When iterating a hash, the block will be passed the elements as [key, value] arrays. If you add a bit of array destructuring, you get key and value separately.

   zone_files.each do |environment, file|
     # ...
   end
   

4. But zone_files doesn't actually need to be a hash. Of course you can use one, and it may be worth it to label to two zone files. But it could just be an array. And you might want to make it constant while you're at it.

5. Get the current branch once. Right now it's being parsed for each of the zone files, since it's inside the loop.

6. And check the branch once - if it's the pri_branch then you can skip everything else.

7. You can use a heredoc for the warning

Here's my take

# Array of zone files to look for when a commit is attempted.
ZONE_FILES = %w(some_zone_file1 some_zone_file2).freeze

# The branch we're allowed to commit zone files on
PRIMARY_BRANCH = "master".freeze

# get the current branch
current_branch = %x{git branch}.match(/\* (.+?)\n/m)[1]

# stop here if we're committing to the primary branch
exit(0) if current_branch == PRIMARY_BRANCH

# execute git diff to determine files includes as part of commit.
commit_files = %x{git diff --cached --name-only}

# find out if any of the zone files are among the files
# being committed
committing_zone_file = ZONE_FILES.detect { |zone_file| commit_files.include?(zone_file) }

# and if we're committing a zone file, complain to the user
if committing_zone_file
  puts <<-EOT
Error: You have triggered a git pre-commit hook.
Your current branch is: #{current_branch}.
For DNS, ensure you commit on branch: #{PRIMARY_BRANCH}
Include '-n' to bypass this check.
Exiting...
  EOT

  exit(1)
end

Answer 2

Some notes:

• zone_files.keys.each and then zone_files[key]. You should iterate the pair: zone_files.each do |key, zone|

• current_branch = %x{git branch}.match(/\* (.+?)\n/m). Isn't this inmutable? then better move it before the loop.

• current_branch[1].include?pri_branch. That's pretty unreadable, put those parentheses: current_branch[1].include?(pri_branch)

Otherwise, it looks pretty good to me.

Answer 3

For readability, I suggest using parentheses when calling .include?(…).

Saying "Warning… Warning… Warning" on every line is a bit excessive, I think. A warning that prevents an operation from completing is not a warning — it's an error. The last line of the message would be more clearly phrased as "Run 'git commit -n' to bypass this check."

In general, though, I think the script looks fine.