Gracefully handle .git submodule files

It turns out that the `.git` directory can also be a file whose contents
point to another directory.

If users used this kind of `.git` file, Overcommit would fail
spectacularly. Add support for detecting this case and handling it
gracefully.

In the process, I noticed a number of tests depended on the `repo_root`
and `git_dir` helpers of the `Utils` module, so I extracted some code to
clear their internal caches before each example.

Change-Id: I18941d9c5ae72d240f1fef44593330592d2398ca
Reviewed-on: http://gerrit.causes.com/44223
Tested-by: jenkins <jenkins@brigade.com>
Reviewed-by: Shane da Silva <shane.dasilva@brigade.com>

Author: sds

CHANGELOG.md

@@ -10,6 +10,7 @@
   [JSXCS](https://github.com/orktes/node-jsxcs)
 * Add pre-commit Ruby code smell checking with
   [Reek](https://github.com/troessner/reek)
+* Gracefully handle `.git` files that point to an external git directory
 
 ## 0.18.0
 

lib/overcommit/git_repo.rb

@@ -68,7 +68,7 @@ def store_merge_state
         @merge_head = merge_head
       end
 
-      merge_msg_file = File.expand_path('.git/MERGE_MSG', Overcommit::Utils.repo_root)
+      merge_msg_file = File.expand_path('MERGE_MSG', Overcommit::Utils.git_dir)
       @merge_msg = File.open(merge_msg_file).read if File.exist?(merge_msg_file)
     end
 
@@ -90,16 +90,16 @@ def store_cherry_pick_state
     # of a merge.
     def restore_merge_state
       if @merge_head
-        FileUtils.touch(File.expand_path('.git/MERGE_MODE', Overcommit::Utils.repo_root))
+        FileUtils.touch(File.expand_path('MERGE_MODE', Overcommit::Utils.git_dir))
 
-        File.open(File.expand_path('.git/MERGE_HEAD', Overcommit::Utils.repo_root), 'w') do |f|
+        File.open(File.expand_path('MERGE_HEAD', Overcommit::Utils.git_dir), 'w') do |f|
           f.write("#{@merge_head}\n")
         end
         @merge_head = nil
       end
 
       if @merge_msg
-        File.open(File.expand_path('.git/MERGE_MSG', Overcommit::Utils.repo_root), 'w') do |f|
+        File.open(File.expand_path('MERGE_MSG', Overcommit::Utils.git_dir), 'w') do |f|
           f.write("#{@merge_msg}\n")
         end
         @merge_msg = nil
@@ -110,8 +110,8 @@ def restore_merge_state
     # of a cherry-pick.
     def restore_cherry_pick_state
       if @cherry_head
-        File.open(File.expand_path('.git/CHERRY_PICK_HEAD',
-                                   Overcommit::Utils.repo_root), 'w') do |f|
+        File.open(File.expand_path('CHERRY_PICK_HEAD',
+                                   Overcommit::Utils.git_dir), 'w') do |f|
           f.write("#{@cherry_head}\n")
         end
         @cherry_head = nil

lib/overcommit/installer.rb

@@ -60,7 +60,7 @@ def update
 
     def hooks_path
       absolute_target = File.expand_path(@target)
-      File.join(absolute_target, '.git', 'hooks')
+      File.join(Overcommit::Utils.git_dir(absolute_target), 'hooks')
     end
 
     def master_hook_install_path

lib/overcommit/utils.rb

@@ -9,11 +9,47 @@ def script_path(script)
       end
 
       # Returns an absolute path to the root of the repository.
+      #
+      # We do this ourselves rather than call `git rev-parse --show-toplevel` to
+      # solve an issue where the .git directory might not actually be valid in
+      # tests.
+      #
+      # @return [String]
       def repo_root
         @repo_root ||=
           begin
-            result = `git rev-parse --show-toplevel`.chomp
-            result if $?.success?
+            git_dir = Pathname.new(File.expand_path('.')).enum_for(:ascend).find do |path|
+              File.exist?(File.join(path, '.git'))
+            end
+
+            unless git_dir
+              raise Overcommit::Exceptions::InvalidGitRepo, 'no .git directory found'
+            end
+
+            git_dir.to_s
+          end
+      end
+
+      # Returns an absolute path to the .git directory for a repo.
+      #
+      # @param repo_dir [String] root directory of git repo
+      # @return [String]
+      def git_dir(repo_dir = repo_root)
+        @git_dir ||=
+          begin
+            git_dir = File.expand_path('.git', repo_dir)
+
+            # .git could also be a file that contains the location of the git directory
+            unless File.directory?(git_dir)
+              git_dir = File.read(git_dir)[/^gitdir: (.*)$/, 1]
+
+              # Resolve relative paths
+              unless git_dir.start_with?('/')
+                git_dir = File.expand_path(git_dir, repo_dir)
+              end
+            end
+
+            git_dir
           end
       end
 

libexec/index-tags

@@ -6,10 +6,10 @@
 
 set -e
 
-trap "rm -f .git/tags.$$" EXIT
-err_file=.git/ctags.err
-if ctags --tag-relative -Rf.git/tags.$$ --exclude=.git "$@" 2>${err_file}; then
-  mv .git/tags.$$ .git/tags
+trap "rm -f $GIT_DIR/tags.$$" EXIT
+err_file=$GIT_DIR/ctags.err
+if ctags --tag-relative -Rf$GIT_DIR/tags.$$ --exclude=.git "$@" 2>${err_file}; then
+  mv $GIT_DIR/tags.$$ $GIT_DIR/tags
   [ -e ${err_file} ] && rm -f ${err_file}
 else
   # Ignore STDERR unless `ctags` returned a non-zero exit code

spec/overcommit/configuration_loader_spec.rb

@@ -4,11 +4,6 @@
   describe '.load_repo_config' do
     subject { described_class.load_repo_config }
 
-    before do
-      # Ensure memoized repo root gets reset
-      Overcommit::Utils.instance_variable_set(:@repo_root, nil)
-    end
-
     context 'when repo does not contain a configuration file' do
       around do |example|
         repo do

spec/overcommit/configuration_spec.rb

@@ -4,10 +4,6 @@
   let(:hash) { {} }
   let(:config) { described_class.new(hash) }
 
-  before do
-    Overcommit::Utils.instance_variable_set(:@repo_root, nil)
-  end
-
   describe '#new' do
     let(:internal_hash) { config.instance_variable_get(:@hash) }
     subject { config }

spec/overcommit/utils_spec.rb

@@ -13,18 +13,64 @@
     let(:repo_dir) { repo }
     subject { described_class.repo_root }
 
-    before do
-      described_class.instance_variable_set(:@repo_root, nil)
+    around do |example|
+      Dir.chdir(repo_dir) do
+        example.run
+      end
+    end
+
+    it 'returns the path to the repository root' do
+      subject.should == repo_dir
     end
 
+    context 'when there is no .git directory' do
+      before do
+        `rm -rf .git`
+      end
+
+      it 'raises an exception' do
+        expect { subject }.to raise_error Overcommit::Exceptions::InvalidGitRepo
+      end
+    end
+  end
+
+  describe '.git_dir' do
+    let(:repo_dir) { repo }
+    subject { described_class.git_dir }
+
     around do |example|
       Dir.chdir(repo_dir) do
         example.run
       end
     end
 
-    it 'returns the path to the repository root' do
-      subject.should end_with repo_dir
+    context 'when .git is a directory' do
+      it 'returns the path to the directory' do
+        subject.should end_with File.join(repo_dir, '.git')
+      end
+    end
+
+    context 'when .git is a file' do
+      before do
+        `rm -rf .git`
+        `echo "gitdir: #{git_dir_path}" > .git`
+      end
+
+      context 'and is a relative path' do
+        let(:git_dir_path) { '../.git' }
+
+        it 'returns the path contained in the file' do
+          subject.should == File.join(File.dirname(repo_dir), '.git')
+        end
+      end
+
+      context 'and is an absolute path' do
+        let(:git_dir_path) { '/some/arbitrary/path/.git' }
+
+        it 'returns the path contained in the file' do
+          subject.should == git_dir_path
+        end
+      end
     end
   end
 

spec/spec_helper.rb

@@ -28,4 +28,12 @@
   config.mock_with :rspec do |c|
     c.syntax = :should
   end
+
+  # Much of Overcommit depends on these helpers, so they are aggressively
+  # cached. Unset them before each example so we always get fresh values.
+  config.before(:each) do
+    %w[git_dir repo_root].each do |var|
+      Overcommit::Utils.instance_variable_set(:"@#{var}", nil)
+    end
+  end
 end