Improve submodule error handling

Add error handling for cases where the specified commit doesn't have a
.gitmodules file, or where the .gitmodules file contains a submodule
with an invalid path.

Author: mr-vinn

lib/gitlab_git/repository.rb

@@ -9,6 +9,7 @@ class Repository
       include Gitlab::Git::Popen
 
       class NoRepository < StandardError; end
+      class InvalidBlobName < StandardError; end
 
       # Default branch in the repository
       attr_accessor :root_ref
@@ -414,7 +415,12 @@ def lookup(oid_or_ref_name)
       def submodules(ref)
         commit = rugged.rev_parse(ref)
 
-        content = blob_content(commit, ".gitmodules")
+        begin
+          content = blob_content(commit, ".gitmodules")
+        rescue InvalidBlobName
+          return {}
+        end
+
         parse_gitmodules(commit, content)
       end
 
@@ -722,6 +728,10 @@ def rev_parse_target(revspec)
       def blob_content(commit, blob_name)
         blob_entry = tree_entry(commit, blob_name)
 
+        unless blob_entry
+          raise InvalidBlobName.new("Invalid blob name: #{blob_name}")
+        end
+
         if blob_entry[:type] == :commit
           blob_entry[:oid]
         else
@@ -740,11 +750,16 @@ def parse_gitmodules(commit, content)
             current = txt.match(/(?<=").*(?=")/)[0]
             results[current] = {}
           else
+            next unless results[current]
             match_data = txt.match(/(\w+)\s*=\s*(.*)/)
             results[current][match_data[1]] = match_data[2]
 
             if match_data[1] == "path"
-              results[current]["id"] = blob_content(commit, match_data[2])
+              begin
+                results[current]["id"] = blob_content(commit, match_data[2])
+              rescue InvalidBlobName
+                results.delete(current)
+              end
             end
           end
         end

spec/repository_spec.rb

@@ -160,14 +160,14 @@
 
   context :submodules do
     let(:repository) { Gitlab::Git::Repository.new(TEST_REPO_PATH) }
-    let(:submodules) { repository.submodules('master') }
 
-    it { submodules.should be_kind_of Hash }
-    it { submodules.empty?.should be_false }
-
-    describe :submodule do
+    context 'where repo has submodules' do
+      let(:submodules) { repository.submodules('master') }
       let(:submodule) { submodules.first }
 
+      it { submodules.should be_kind_of Hash }
+      it { submodules.empty?.should be_false }
+
       it 'should have valid data' do
         submodule.should == [
           "six", {
@@ -191,6 +191,17 @@
         expect(nested['url']).to eq('git://github.com/randx/six.git')
         expect(nested['id']).to eq('24fb71c79fcabc63dfd8832b12ee3bf2bf06b196')
       end
+
+      it 'should not have an entry for an invalid submodule' do
+        expect(submodules).not_to have_key('invalid/path')
+      end
+    end
+
+    context 'where repo doesn\'t have submodules' do
+      let(:submodules) { repository.submodules('6d39438') }
+      it 'should return an empty hash' do
+        expect(submodules).to be_empty
+      end
     end
   end